[FFmpeg-devel] [PATCH] mov auto crop

Aurelien Jacobs aurel
Sun Oct 3 22:14:18 CEST 2010


On Sun, Oct 03, 2010 at 07:46:12AM +0200, Michael Niedermayer wrote:
> On Sun, Oct 03, 2010 at 12:46:59AM +0200, 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.
> 
> I think we need to pass the crop parameters through libavfilter so a
> resize or crop filter can update them. Or a future uncrop filter could remove
> them. (thats of course not for this patch to do but a future one but it
> will solve the copying issues ...)

Good idea. That would be ideal. But this is quite independent from
(de)muxing and can be implemented afterward.

> Also there are crop parameters in h264 that we dont support yet IIRC and
> we might want these to use the same API. Id like to think a bit more about
> this but maybe having the 2 crop parameters in AVCodecContext would work
> better than AVStream.

I don't really know yet how we would use h264 crop parameters, but I see
your point. Attached patch moves this API to AVCodecContext instead of
AVStream. It is not harder to use and gives a little bit more
flexibility.
OK to commit ? (with the corresponding minor bump and APIchanges)

Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: crop_api.diff
Type: text/x-diff
Size: 523 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101003/541105c1/attachment.diff>



More information about the ffmpeg-devel mailing list