[FFmpeg-devel] [PATCH] mov auto crop

Fri Oct 8 04:18:11 CEST 2010

On Thu, Oct 07, 2010 at 11:27:28PM +0200, Aurelien Jacobs wrote:
> 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).

they could be crop_top/bottom/... then 0 is natural no crop, its the opposite
of what the user parameters of vf_crop are though, feels odd but it seems
more natural to do it like this here. Also its closer to what is stored
in h264

anyway this is largely bikeshed and i dont care strongly

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101008/5a7124b5/attachment.pgp>