[FFmpeg-devel] [PATCH] demuxer aspect ratio

[Aurelien Jacobs]

Michael Niedermayer wrote:

> On Fri, Aug 22, 2008 at 06:16:53PM +0200, Aurelien Jacobs wrote:
> > Michael Niedermayer wrote:
> > 
> > > On Fri, Aug 22, 2008 at 01:23:37AM +0200, Aurelien Jacobs wrote:
> > > > Michael Niedermayer wrote:
> > > > 
> > > > > On Fri, Aug 22, 2008 at 12:22:49AM +0200, Aurelien Jacobs wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > Currently, when a video must be rendered with non-square pixels, it is
> > > > > > specified in AVCodecContext.sample_aspect_ratio. The problem is that
> > > > > > this field is set by both decoders and demuxers. In some cases, the
> > > > > > demuxer properly set this field with correct SAR, and then the
> > > > > > decoder overwrite it with dumb values.
> > > > > > I think we need an API so that the caller can chose to use either
> > > > > > the value from the decoder or the demuxer (the demuxer one should
> > > > > > generally be preferred if available).
> > > > > > Attached patch adds a AVStream.display_aspect_ratio dedicated to
> > > > > 
> > > > > sample_aspect_ratio is strongly preferred over display.
> > > > > It is much easier to work with as it does not change when croping or adding
> > > > > borders. Also when rendering a frame into another that can be done when the
> > > > > sample_aspect_ratio matches, it cannot when not without scaling ...
> > > > 
> > > > Oh, indeed. That's something I didn't considered.
> > > > Now, I attached the same patch but using sample_aspect_ratio instead of
> > > > display_aspect_ratio.
> > > > 
> > > [...]
> > > > Index: ffplay.c
> > > > ===================================================================
> > > > --- ffplay.c	(revision 14883)
> > > > +++ ffplay.c	(working copy)
> > > > @@ -657,11 +657,14 @@
> > > >      vp = &is->pictq[is->pictq_rindex];
> > > >      if (vp->bmp) {
> > > >          /* XXX: use variable in the frame */
> > > > -        if (is->video_st->codec->sample_aspect_ratio.num == 0)
> > > > -            aspect_ratio = 0;
> > > > -        else
> > > > +        if (is->video_st->sample_aspect_ratio.num)
> > > > +            aspect_ratio = av_q2d(is->video_st->sample_aspect_ratio)
> > > > +                * is->video_st->codec->width / is->video_st->codec->height;
> > > > +        else if (is->video_st->codec->sample_aspect_ratio.num)
> > > >              aspect_ratio = av_q2d(is->video_st->codec->sample_aspect_ratio)
> > > >                  * is->video_st->codec->width / is->video_st->codec->height;
> > > > +        else
> > > > +            aspect_ratio = 0;
> > > 
> > > ok but 
> > > * is->video_st->codec->width / is->video_st->codec->height;
> > > can be factored out after this
> > 
> > OK, done (both in ffplay.c and ffmpeg.c).
> > 
> > > rest ok, but a few things are missing
> > > * ffmpeg.c should set it for muxers (and dont forget -vcodec copy)
> > 
> > OK. It's now set for muxers. -vcodec copy handle aspect ratio fine.
> > 
> > > * it maybe should be added to the AVOption array
> > 
> > I thought about it initially, but it seems that the AVOption array
> > only allows to change parameters which are stored in AVFormatContext
> > for now. This new sample_aspect_ratio is stream related, and thus,
> > it's stored in AVStream.
> > So unless I missed something, I think it's not possible to do this
> > for now.
> 
> hmm, ok
> 
> and patch ok

Applied.

Aurel