[FFmpeg-devel] [PATCH] mov auto crop

[Baptiste Coudurier]

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 ?

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

I agree there is no reason, although album cover is by far the most
widespread usage. Videos can always use snapshots and that's what most
OSes do.

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

I already implemented covert art support for mp4 and mp3 for jpg/png/bmp
using AVMetadata.

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

> Patches to implement this API in more (de)muxers and apps are welcome.

Well I'd like to use the metadata API for that, I consider cover art as
metadata.

[...]

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