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

From: Niklas Haas <git@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)
2. This SEI has no ue(v) "repetition period", but a u(1) persistence flag

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@haasn.dev>
---
 libavcodec/h264_sei.c   | 6 +++---
 libavcodec/h264_sei.h   | 2 +-
 libavcodec/h264_slice.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

On 8/2/2021 7:36 AM, Niklas Haas wrote:
> From: Niklas Haas <git@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@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) {
>

On Mon, 02 Aug 2021 08:49:49 -0300 James Almer <jamrial@gmail.com> wrote:
> On 8/2/2021 7:36 AM, Niklas Haas wrote:
> > From: Niklas Haas <git@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.

Ah, I see. I was looking at H.274, how confusing for them to have
changed it like that. I'll send a new patch.