[FFmpeg-devel] [PATCH] mov auto crop

[Baptiste Coudurier]

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.

It is done blindly, nothing is filtered. MP3 muxer will write everything
as TXXX, as well as the FLV 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 ? I'm talking about mp3,m4a,flac

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

> [...]
>
> 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, and mov
patch must be updated as well.

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