[FFmpeg-devel] avcodec/vc1: correct aspect ratio calculation

Hi

On Wed, Nov 14, 2018 at 10:57:25AM +0100, Jerome Borsboom wrote:
> According to VC-1 spec:
> * Display size defaults to max coded size when not explicitly set in
> sequence header
> * Aspect ratio in the sequence header refers to the Display size elements.
> Therefore, the aspect ratio for the coded samples (SAR) needs to take into
> account the scaling from coded size to display size, and the aspect ratio
> of the display size elements.
> 
> Signed-off-by: Jerome Borsboom <jerome.borsboom@carpalis.nl>
> ---
> VC-1 spec assumes that the output of the decoder, i.e. the pixel matrix with
> dimensions coded_width x coded_height, is scaled to display size, i.e. a pixel
> matrix of display_horiz_size x display_vert_size. This may introduce part of
> the sample aspect ratio when the sizes of the two pixel matrices are not equal.
> A further part of the sample aspect ratio is optionally specified in the sequence
> header as the aspect ratio of the display size pixels. This patch takes both
> aspect ratios into account and aims to be correct even when the coded image
> includes overscan regions.
> 
>  libavcodec/vc1.c | 38 +++++++++++++++++++++-----------------
>  libavcodec/vc1.h |  2 ++
>  2 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/libavcodec/vc1.c b/libavcodec/vc1.c
> index 3581d87b57..efc6edc4b0 100644
> --- a/libavcodec/vc1.c
> +++ b/libavcodec/vc1.c
> @@ -442,30 +442,24 @@ static int decode_sequence_header_adv(VC1Context *v, GetBitContext *gb)
>      }
>      v->s.max_b_frames = v->s.avctx->max_b_frames = 7;
>      if (get_bits1(gb)) { //Display Info - decoding is not affected by it
> -        int w, h, ar = 0;
> +        int ar = 0;
>          av_log(v->s.avctx, AV_LOG_DEBUG, "Display extended info:\n");
> -        w = get_bits(gb, 14) + 1;
> -        h = get_bits(gb, 14) + 1;
> -        av_log(v->s.avctx, AV_LOG_DEBUG, "Display dimensions: %ix%i\n", w, h);
> +        v->disp_horiz_size = get_bits(gb, 14) + 1;
> +        v->disp_vert_size = get_bits(gb, 14) + 1;
> +        av_log(v->s.avctx, AV_LOG_DEBUG, "Display dimensions: %ix%i\n",
> +               v->disp_horiz_size, v->disp_vert_size);
>          if (get_bits1(gb))
>              ar = get_bits(gb, 4);
>          if (ar && ar < 14) {
> -            v->s.avctx->sample_aspect_ratio = ff_vc1_pixel_aspect[ar];
> +            v->aspect_ratio = ff_vc1_pixel_aspect[ar];
>          } else if (ar == 15) {

> -            w = get_bits(gb, 8) + 1;
> -            h = get_bits(gb, 8) + 1;
> -            v->s.avctx->sample_aspect_ratio = (AVRational){w, h};

> +            v->aspect_ratio = (AVRational){get_bits(gb, 8) + 1, get_bits(gb, 8) + 1};

iam not sure this is valid C and not undefined

but either way this patch breaks fate

TEST    vc1-ism
Test vc1-ism failed. Look at tests/data/fate/vc1-ism.err for details.
make: *** [fate-vc1-ism] Error 1


[...]

> Hi
> 
[...]
> 
> iam not sure this is valid C and not undefined
> 
> but either way this patch breaks fate
> 
> TEST    vc1-ism
> --- ./tests/ref/fate/vc1-ism	2018-11-13 19:52:23.489023763 +0100
> +++ tests/data/fate/vc1-ism	2018-11-14 21:50:11.522992878 +0100
> @@ -2,7 +2,7 @@
>  #media_type 0: video
>  #codec_id 0: rawvideo
>  #dimensions 0: 240x104
> -#sar 0: 156/156
> +#sar 0: 13/30
>  0,          0,          0,        1,    37440, 0xd1bc5235
>  0,          2,          2,        1,    37440, 0x158e6167
>  0,          3,          3,        1,    37440, 0x0faa4481
> Test vc1-ism failed. Look at tests/data/fate/vc1-ism.err for details.
> make: *** [fate-vc1-ism] Error 1
> 

Thank you for the catch. Although 

v->aspect_ratio = (AVRational){get_bits(gb, 8) + 1, get_bits(gb, 8) + 1};

is valid in C99, the order of evaluation of the initialization arguments is
indeed undefined. In C90 the arguments must be constant expressions, so the
previous line in which variables were used was not allowed either according
to the developer documentation.