[FFmpeg-devel] [PATCH] mov auto crop

[Baptiste Coudurier]

On 10/07/2010 03:02 PM, Aurelien Jacobs wrote:
> 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).

I just don't understand that. In particular I don't understand why 
writing has anything to do with exporting.

>>>>>> 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 ?

Covert art is meant to be seen IMHO, I guess some people might wish to 
see the covert art on their computer...
What I care about is ipod/iphone/itunes.

>> [...]
>>> 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).

We will have to agree to disagree on this one then.

>>> 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.

Done

>    - use avcodec to re-encode the picture (BMP to jpeg for example) while
>    copying metadata from input file to output file

No need for that. Every format that I know of support bmp/jpeg/png.

>    - add support for recursive metadata everywhere, to be able to store
>    metadata about the cover art.

No need for recursive metadata, only type.

>> 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.

You ffmpeg's metadata API, not mine. I was always against crippling it to
char*/char*

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org