[FFmpeg-devel] [PATCH] demuxer aspect ratio

[Michael Niedermayer]

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

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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080822/c6cb511f/attachment.pgp>