[FFmpeg-devel] [PATCH] avcodec/h264_sei: fix H.274 film grain parsing

[James Almer]

On 8/2/2021 7:36 AM, Niklas Haas wrote:
> From: Niklas Haas <git at haasn.dev>
> 
> The current code has a number of issues:
> 1. The fg_model_id is specified in H.274 as u(2), not u(8)

Yes, good catch.

> 2. This SEI has no ue(v) "repetition period", but a u(1) persistence flag

This one however isn't. ITU-T H.264 has an ue value with a range 
0..16384 called film_grain_characteristics_repetition_period in the SEI 
message that was replaced by the u(1) persistence flag in H.265, which 
was then adopted by H.274. Everything else in the spec is exactly the 
same otherwise, including how it's applied.

film_grain_characteristics_repetition_period == 0 means the the film 
grain described by the SEI applies only to the current decoded picture. 
Non zero means it persists.

> 
> Additionally, we can get away with using the non-long version of
> get_se_golomb() because the SEI spec as written limits these values to
> 2^(fgBitDepth)-1 where fgBitDepth maxes out at 2^3-1 + 8 = 15 bits.
> 
> Fixes: corrupt bistream values due to wrong number of bits read
> Fixes: 4ff73add5dbe6c319d693355be44df2e17a0b8bf
> 
> Signed-off-by: Niklas Haas <git at haasn.dev>
> ---
>   libavcodec/h264_sei.c   | 6 +++---
>   libavcodec/h264_sei.h   | 2 +-
>   libavcodec/h264_slice.c | 2 +-
>   3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
> index 7797f24650..8de1ce4b90 100644
> --- a/libavcodec/h264_sei.c
> +++ b/libavcodec/h264_sei.c
> @@ -424,7 +424,7 @@ static int decode_film_grain_characteristics(H264SEIFilmGrainCharacteristics *h,
>   
>       if (h->present) {
>           memset(h, 0, sizeof(*h));
> -        h->model_id = get_bits(gb, 8);
> +        h->model_id = get_bits(gb, 2);
>           h->separate_colour_description_present_flag = get_bits1(gb);
>           if (h->separate_colour_description_present_flag) {
>               h->bit_depth_luma = get_bits(gb, 3) + 8;
> @@ -448,11 +448,11 @@ static int decode_film_grain_characteristics(H264SEIFilmGrainCharacteristics *h,
>                       h->intensity_interval_lower_bound[c][i] = get_bits(gb, 8);
>                       h->intensity_interval_upper_bound[c][i] = get_bits(gb, 8);
>                       for (int j = 0; j < h->num_model_values[c]; j++)
> -                        h->comp_model_value[c][i][j] = get_se_golomb_long(gb);
> +                        h->comp_model_value[c][i][j] = get_se_golomb(gb);
>                   }
>               }
>           }
> -        h->repetition_period = get_ue_golomb_long(gb);
> +        h->persistent = get_bits1(gb);
>   
>           h->present = 1;
>       }
> diff --git a/libavcodec/h264_sei.h b/libavcodec/h264_sei.h
> index f9166b45df..dbceb15627 100644
> --- a/libavcodec/h264_sei.h
> +++ b/libavcodec/h264_sei.h
> @@ -183,7 +183,7 @@ typedef struct H264SEIFilmGrainCharacteristics {
>       uint8_t intensity_interval_lower_bound[3][256];
>       uint8_t intensity_interval_upper_bound[3][256];
>       int16_t comp_model_value[3][256][6];
> -    int repetition_period;
> +    int persistent;
>   } H264SEIFilmGrainCharacteristics;
>   
>   typedef struct H264SEIContext {
> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index 41338fbcb6..03606bf504 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -1381,7 +1381,7 @@ static int h264_export_frame_props(H264Context *h)
>           memcpy(&fgp->codec.h274.comp_model_value, &fgc->comp_model_value,
>                  sizeof(fgp->codec.h274.comp_model_value));
>   
> -        fgc->present = !!fgc->repetition_period;
> +        fgc->present = fgc->persistent;
>       }
>   
>       if (h->sei.picture_timing.timecode_cnt > 0) {
>