[FFmpeg-devel] [PATCH] mov auto crop

[Aurelien Jacobs]

On Sun, Oct 03, 2010 at 11:34:29PM +0200, Michael Niedermayer wrote:
> On Sun, Oct 03, 2010 at 10:14:18PM +0200, Aurelien Jacobs wrote:
> > 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
> >  avcodec.h |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > a5c3d9cb144a5dff8cb7bd3d0fb17fb0d85fd48d  crop_api.diff
> > Index: libavcodec/avcodec.h
> > ===================================================================
> > --- libavcodec/avcodec.h	(revision 25319)
> > +++ libavcodec/avcodec.h	(working copy)
> > @@ -2739,6 +2739,12 @@ typedef struct AVCodecContext {
> >       * - decoding: unused
> >       */
> >      int lpc_passes;
> > +
> > +    /**
> > +     * 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;
> 
> thats a bit underdocumented

Documentation improved.

> also i think the no crop case should be natural consequence of the values
> not a special case with w/h=0

I'm not sure exactly what you mean here ?
Those values will be initialized to 0 anyway (by av_mallocz()).
I guess you suggest that they are set to movie width and height at some
point (somewhere in utils.c).
This just add some code in utils.c, but in this specific situation, I
don't see where is the gain.
Anyway the application will want to treat the "no cropping" case
as a special case, to avoid inserting a useless crop filter.
So whether the apps check for w==0 or w==movie_width, I don't really see
the gain. Unless you think it's OK to always insert a crop filter which
will do nothing for 99% of the files.
So do you really think it's really useful to add some code in utils.c to
set w and h to movie resolution in case the demuxer let them set to 0 ?

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