[MPlayer-dev-eng] [PATCH] fix aspect ratio calculations broken by new av_cmp_q
Reimar Döffinger
Reimar.Doeffinger at gmx.de
Tue Oct 19 21:13:09 CEST 2010
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;
More information about the MPlayer-dev-eng
mailing list