[FFmpeg-devel] [PATCH] mov auto crop

Michael Niedermayer michaelni
Sun Oct 3 23:34:29 CEST 2010


On Sun, Oct 03, 2010 at 10:14:18PM +0200, Aurelien Jacobs wrote:
> On Sun, Oct 03, 2010 at 07:46:12AM +0200, Michael Niedermayer wrote:
> > On Sun, Oct 03, 2010 at 12:46:59AM +0200, Aurelien Jacobs wrote:
> > > On Sat, Oct 02, 2010 at 01:01:43PM -0700, Baptiste Coudurier wrote:
> > > > On 10/2/10 10:15 AM, Aurelien Jacobs wrote:
> > > > > On Sat, Oct 02, 2010 at 02:57:43PM +0200, Benjamin Larsson wrote:
> > > > >> On 10/02/2010 04:51 AM, Baptiste Coudurier wrote:
> > > > >>> Hi guys,
> > > > >>>
> > > > >>> $subject.
> > > > >>>
> > > > >>> Please keep in mind that this issue has been present for _years_ and
> > > > >>> nobody dared to fix it. I believe the patch is not so hackish/ugly.
> > > > >>>
> > > > >>> I restrict it to codecs not supporting arbitrary resolutions that can be
> > > > >>> stored in .mov by quicktime.
> > > > >>>
> > > > >>> Maybe exporting to "crop", "wxh" is better, I don't know.
> > > > >>>
> > > > >>> I plan to add auto-rotate for iphone 3gs files this way as well :)
> > > > >>>
> > > > >>
> > > > >> Jolly good. But would it be possible to move this code to the api and
> > > > >> just not in the ffmpeg commandline tool?
> > > > > 
> > > > > I agree. Before this patch is committed, I would like to see a proper
> > > > > API patch, whether it is by documenting the metadata API or by adding
> > > > > some fields to the AVFormatContext struct.
> > > > > After we have a clean and documented API for this, it will be trivial
> > > > > and un-controversial to use this API in any muxer/demuxer and tools
> > > > > including all the API users which are not part of the FFmpeg repository.
> > > > > 
> > > > > I have to say that I don't like much the use of the metadata API for
> > > > > this. I forces every existing applications to filter out metadata before
> > > > > copying them to a transcoded file (which might have been resized, so the
> > > > > cropping values don't make any sens anymore). So in some way, it kind of
> > > > > break current public API...
> > > > 
> > > > You are being unreasonable IMHO. No it doesn't break the current public
> > > > API.
> > > 
> > > Unreasonable ? I just gave my feeling about this patch, nothing
> > > more. You can ignore it if you want.
> > > 
> > > > I already said that blindly copying metadata from one container to
> > > > another is a _bad_ idea and we should not do it.
> > > 
> > > We have some conversion tables specifically designed for this, and it
> > > sounds like a very common use case to re-encode for example from mp3 to
> > > vorbis while keeping metadata. In fact, I guess most users want to retain
> > > metadata while transcoding most files.
> > > Still it's not done blindly. You have to use the -map_meta_data option
> > > to trigger this.
> > > And users won't expect -map_meta_data to generate some broken file when
> > > transcoding, let's say from mov to mov, while resising video.
> > 
> > I think we need to pass the crop parameters through libavfilter so a
> > resize or crop filter can update them. Or a future uncrop filter could remove
> > them. (thats of course not for this patch to do but a future one but it
> > will solve the copying issues ...)
> 
> Good idea. That would be ideal. But this is quite independent from
> (de)muxing and can be implemented afterward.
> 
> > Also there are crop parameters in h264 that we dont support yet IIRC and
> > we might want these to use the same API. Id like to think a bit more about
> > this but maybe having the 2 crop parameters in AVCodecContext would work
> > better than AVStream.
> 
> I don't really know yet how we would use h264 crop parameters, but I see
> your point. Attached patch moves this API to AVCodecContext instead of
> AVStream. It is not harder to use and gives a little bit more
> flexibility.
> OK to commit ? (with the corresponding minor bump and APIchanges)
> 
> Aurel
>  avcodec.h |    6 ++++++
>  1 file changed, 6 insertions(+)
> a5c3d9cb144a5dff8cb7bd3d0fb17fb0d85fd48d  crop_api.diff
> Index: libavcodec/avcodec.h
> ===================================================================
> --- libavcodec/avcodec.h	(revision 25319)
> +++ libavcodec/avcodec.h	(working copy)
> @@ -2739,6 +2739,12 @@ typedef struct AVCodecContext {
>       * - decoding: unused
>       */
>      int lpc_passes;
> +
> +    /**
> +     * Video cropping parameters suggested by the container.
> +     * width=0 or height=0 means no cropping in the corresponding direction.
> +     */
> +    struct { int x, y, width, height; } crop;

thats a bit underdocumented
also i think the no crop case should be natural consequence of the values
not a special case with w/h=0


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101003/08c94bf0/attachment.pgp>



More information about the ffmpeg-devel mailing list