[FFmpeg-devel] [PATCH] mov auto crop

[Aurelien Jacobs]

On Sun, Oct 03, 2010 at 03:30:08PM -0700, Baptiste Coudurier wrote:
> On 10/3/10 1:04 PM, Aurelien Jacobs wrote:
> > 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.
> 
> Great, then you have no problem exporting _anything_ in AVMetadata, do you ?

I have no problem exporting anything that make sens writting as is in
any destination container supporting "free format" metadata (without
making any assuption how the streams may have been converted between
input and output container).

> >>>> 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 ?
> 
> No equipment supports it ?

I'm pretty sure some equipment do (even if it is not really common).
Anyway, if your audio is encoded with some uncommon codec (TTA1,
WAVPACK4, COOK...), chances are that you don't care about equipment
support... In this case, what's wrong with using Matroska for audio ?
Should it be forbiden to use cover art along with those fringe codecs ?

> [...]
> > 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.
> 
> Ok, let me try to see what that would be:
> ffmpeg -i <file.flac> file.mp3 -newdata -dcodec copy -map_meta_data
> 
> Sorry but this looks like a braindead API to me.

Well, this looks like a natural and efficient API to me, especially when
you want to insert some jpeg files as a covert art into a newly encoded
audio file, or if you want to compress your lossless FLAC+bmp to vorbis+jpeg.
It is also an appropriate API to read/write metadata of the cover art
(eg. an artist tag containing the name of the painter whose painting is
reproduced on the cover).

> > Patches to implement this API in more (de)muxers and apps are welcome.
> 
> Well I'd like to use the metadata API for that,

This looks like a braindead API to me, regarding cover art.
To use the metadata API, you would need to introduce in this API:
  - reading/writting a metadata tag from/to a independent file (or http
  url or any proto) to be able to insert/extract cover art.
  - use avcodec to re-encode the picture (BMP to jpeg for example) while
  copying metadata from input file to output file
  - add support for recursive metadata everywhere, to be able to store
  metadata about the cover art.

> I consider cover art as metadata.

Whether you consider a cover art as a metadata of a song doesn't make it
an appropriate candidate for the ffmpeg metadata API.

Aurel