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

[Michael Niedermayer]

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 at 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
--- ./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


[...]
-- 
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: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20181114/de7761f3/attachment.sig>