[FFmpeg-devel] [PATCH] demuxer aspect ratio

[Aurelien Jacobs]

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.

Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sample_aspect_ratio_2.diff
Type: text/x-patch
Size: 5976 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080822/fea57ccc/attachment.bin>