[MPlayer-dev-eng] [PATCH] fix aspect ratio calculations broken by new av_cmp_q

[Reimar Döffinger]

On Tue, Oct 19, 2010 at 10:32:38AM -0400, Andrew Wason wrote:
> On Tue, Oct 19, 2010 at 2:35 AM, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> > In that case the patch is working exactly as intended.
> > The container claims one aspect, the video stream claims another.
> > Currently MPlayer is supposed to go with the value in the video stream,
> > since otherwise resolution/aspect changes won't work right.
> 
> 
> But isn't the opposite happening? In this video the AVStream aspect is
> 16:9, the AVCodecContext aspect is 4:3. MPlayer used to play it as
> 16:9. After av_cmp_q changed MPlayer plays it as 4:3.
> 
> sh->aspect is being computed as 1.777 from the AVStream aspect in
> demux_lavf.c:handle_stream() - here MPlayer is preferring the AVStream
> aspect over the AVCodecContext aspect.
> 
> if(st->sample_aspect_ratio.num)
>     sh_video->aspect = codec->width  * st->sample_aspect_ratio.num
>          / (float)(codec->height * st->sample_aspect_ratio.den);
> else
>     sh_video->aspect=codec->width  * codec->sample_aspect_ratio.num
>          / (float)(codec->height * codec->sample_aspect_ratio.den);

Hm, the wisdom of that code is somewhat questionable.

> In any case, it seems like aspect changes would still work with my
> patch because the first time init_vo is called
> last_sample_aspect_ratio is not a valid aspect (either 0,0 or 0,1) and
> sh->aspect has already been initialized properly in
> demux_lavf.c:handle_stream( - so we only need to worry about
> subsequent changes to avctx->sample_aspect_ratio which will be
> properly detected because we will have set last_sample_aspect_ratio to
> the initial avctx->sample_aspect_ratio we saw. The patch only prevents
> resetting sh->aspect the first time (when last_sample_aspect_ratio has
> not yet been initialized), it still allows last_sample_aspect_ratio to
> be set to the last avctx->sample_aspect_ratio

I basically came to the same conclusion.
However I think your patch is not 100% corect, what do you think about
the patch below?
Index: libmpcodecs/vd_ffmpeg.c
===================================================================
--- libmpcodecs/vd_ffmpeg.c     (revision 32508)
+++ libmpcodecs/vd_ffmpeg.c     (working copy)
@@ -559,9 +559,13 @@
         // sets the value correctly in avcodec_open.
         set_format_params(avctx, avctx->pix_fmt);
         mp_msg(MSGT_DECVIDEO, MSGL_V, "[ffmpeg] aspect_ratio: %f\n", aspect);
-        if (sh->aspect == 0 ||
-            av_cmp_q(avctx->sample_aspect_ratio,
-                     ctx->last_sample_aspect_ratio))
+
+        // Do not overwrite s->aspect on the first call, so that a container
+        // aspect if available is preferred.
+        // But set it even if the sample aspect did not change, since a
+        // resolution change can cause an aspect change even if the
+        // _sample_ aspect is unchanged.
+        if (sh->aspect == 0 || ctx->last_sample_aspect_ratio.den)
             sh->aspect = aspect;
         ctx->last_sample_aspect_ratio = avctx->sample_aspect_ratio;
         sh->disp_w = width;