[FFmpeg-devel] [PATCH] mov auto crop

[Aurelien Jacobs]

On Sat, Oct 02, 2010 at 03:58:32PM -0700, Baptiste Coudurier wrote:
> On 10/2/10 3:46 PM, 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.
> 
> The tables are designed the convert metadata from one format to another,
> not filter out unacceptable metadata.

Well, there are 2 cases I think. Containers supporting "free format"
metadata for which there is no such thing as "unacceptable metadata".
And containers supporting only a limited set of metadata (such as ID3v1)
for which the conversion table should generate this set of metadata and
everything which is not part of the set after the conversion should
simply be discarded by the muxer.

> >> Feel free to add another field in AVFormatContext if you want, but I
> >> won't do it.
> > 
> > Like in attached patch ?
> > 
> >> Btw, when are we going to have cover art support in svn ? I mean I've
> >> had a patch using AVMetadata since January.
> > 
> > Hum... I think it's in svn since quite a long time now.
> > Try this sample for example:
> >   http://samples.mplayerhq.hu/Matroska/ticket-a_aac.mkv
> 
> LOL who use mkv to store audio ?

Enough people that a specific extension was created for this (mka).
What's wrong with using Matroska for audio ?

> I'm talking about mp3,m4a,flac

Why are you restricting yourself to audio ? Videos also have cover art,
and there is no reason to treat those ones differently thant the audio
ones...

> Besides, how can I transcode to mp3 and keep the cover art ?

Appart from the fact that the mp3 muxer currently don't have cover art
implemented (IIRC), it's simply about stream copying the jpeg
attachement. Well, I think ffmpeg don't have support for copying
attachement yet, but at least the API is here and can be used in
applications, at least for formats which implements it.
Patches to implement this API in more (de)muxers and apps are welcome.

> > [...]
> >
> > Index: libavformat/avformat.h
> > ===================================================================
> > --- libavformat/avformat.h	(revision 25319)
> > +++ libavformat/avformat.h	(working copy)
> > @@ -616,6 +616,12 @@ typedef struct AVStream {
> >       * Number of frames that have been demuxed during av_find_stream_info()
> >       */
> >      int codec_info_nb_frames;
> > +
> > +    /**
> > +     * 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;
> >  } AVStream;
> >  
> >  #define AV_PROGRAM_RUNNING 1
> 
> Looks fine to me, but it's missing minor bump and api changes,

Sure ! I would add them before committing.

> and mov patch must be updated as well.

Sure. And other (de)muxers should make use of this API too (I remember
of a ogg/theora sample needing some cropping). It should also be used in
apps (ffplay...).

Aurel