[FFmpeg-devel] [PATCH 3/3 v3] avcodec/cbs_h265: add missing support for reserved_payload_extension_data SEI bits

[Andreas Rheinhardt]

James Almer:
> Fixes ticket #8622
> 
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
> In writing scenarios, it will now ensure bit_equal_to_one is also always
> written when currently defined extension data (like in Buffering Period) is
> present, and not just when unknown extension data is.
> 
>  libavcodec/cbs_h2645.c                |  1 +
>  libavcodec/cbs_h265.h                 |  1 +
>  libavcodec/cbs_h265_syntax_template.c | 71 +++++++++++++++++++++++++--
>  3 files changed, 68 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index 095e449ddc..b432921ecc 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -553,6 +553,7 @@ static void cbs_h265_free_sei_payload(H265RawSEIPayload *payload)
>          av_buffer_unref(&payload->payload.other.data_ref);
>          break;
>      }
> +    av_buffer_unref(&payload->extension_data.data_ref);
>  }
>  
>  static void cbs_h265_free_sei(void *opaque, uint8_t *content)
> diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h> --- a/libavcodec/cbs_h265.h
> index 2c1e153ad9..73897f77a4 100644
> --- a/libavcodec/cbs_h265.h
> +++ b/libavcodec/cbs_h265.h
> @@ -715,6 +715,7 @@ typedef struct H265RawSEIPayload {
>              AVBufferRef *data_ref;
>          } other;
>      } payload;
> +    H265RawExtensionData extension_data;
>  } H265RawSEIPayload;
>  
>  typedef struct H265RawSEI {
> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> index ed08b06e9c..45838467b7 100644
> --- a/libavcodec/cbs_h265_syntax_template.c
> +++ b/libavcodec/cbs_h265_syntax_template.c
> @@ -1564,7 +1564,8 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
>  
>  static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
>                                        H265RawSEIBufferingPeriod *current,
> -                                      uint32_t *payload_size)
> +                                      uint32_t *payload_size,
> +                                      int *more_data)
>  {
>      CodedBitstreamH265Context *h265 = ctx->priv_data;
>      const H265RawSPS *sps;
> @@ -1658,8 +1659,15 @@ static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
>      else
>          infer(use_alt_cpb_params_flag, 0);
>  #else
> -    if (current->use_alt_cpb_params_flag)
> +    // If unknown extension data exists, then use_alt_cpb_params_flag is
> +    // coded in the bitstream and must be written even if it's 0.
> +    if (current->use_alt_cpb_params_flag || *more_data) {
>          flag(use_alt_cpb_params_flag);
> +        // Ensure this bit is not the last in the payload by making the
> +        // more_data_in_payload() check evaluate to true, so it may not
> +        // be mistaken as something else by decoders.
> +        *more_data = 1;
> +    }
>  #endif
>  
>      return 0;
> @@ -2057,11 +2065,48 @@ static int FUNC(sei_alpha_channel_info)(CodedBitstreamContext *ctx,
>      return 0;
>  }
>  
> +static int FUNC(payload_extension)(CodedBitstreamContext *ctx, RWContext *rw,
> +                                   H265RawExtensionData *current, uint32_t payload_size,
> +                                   int cur_pos)
> +{
> +    int err;
> +    size_t byte_length, k;
> +
> +#ifdef READ
> +    GetBitContext tmp;
> +    int bits_left, payload_zero_bits;
> +
> +    if (!cbs_h265_payload_extension_present(rw, payload_size, cur_pos))
> +        return 0;
> +
> +    bits_left = 8 * payload_size - cur_pos;
> +    tmp = *rw;
> +    if (bits_left > 8)
> +        skip_bits_long(&tmp, bits_left - 8);
> +    payload_zero_bits = get_bits(&tmp, FFMIN(bits_left, 8));
> +    if (!payload_zero_bits)
> +        return AVERROR_INVALIDDATA;
> +    payload_zero_bits = ff_ctz(payload_zero_bits);
> +    current->bit_length = bits_left - payload_zero_bits - 1;
> +    allocate(current->data, (current->bit_length + 7) / 8);
> +#endif
> +
> +    byte_length = (current->bit_length + 7) / 8;
> +    for (k = 0; k < byte_length; k++) {
> +        int length = FFMIN(current->bit_length - k * 8, 8);
> +        xu(length, reserved_payload_extension_data, current->data[k],
> +           0, MAX_UINT_BITS(length), 0);
> +    }
> +
> +    return 0;
> +}
> +
>  static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw,
>                               H265RawSEIPayload *current, int prefix)
>  {
>      int err, i;
> -    int start_position, end_position;
> +    int start_position, current_position, end_position;
> +    int more_data = !!current->extension_data.bit_length;

If I am not mistaken, then more_data is a write-only variable when
reading. Better add av_unused or it might lead to compiler warnings.
(You might even move the definition of more_data into the ifdef block
below and only initialize it during writing.)

>  
>  #ifdef READ
>      start_position = get_bits_count(rw);
> @@ -2093,8 +2138,15 @@ static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw,
>          CHECK(FUNC(sei_ ## name)(ctx, rw, &current->payload.name, \
>                                   &current->payload_size)); \
>          break
> +#define SEI_TYPE_E(type, prefix_valid, suffix_valid, name) \
> +    case HEVC_SEI_TYPE_ ## type: \
> +        SEI_TYPE_CHECK_VALID(name, prefix_valid, suffix_valid); \
> +        CHECK(FUNC(sei_ ## name)(ctx, rw, &current->payload.name, \
> +                                 &current->payload_size, \
> +                                 &more_data)); \
> +        break
>  
> -        SEI_TYPE_S(BUFFERING_PERIOD,         1, 0, buffering_period);
> +        SEI_TYPE_E(BUFFERING_PERIOD,         1, 0, buffering_period);
>          SEI_TYPE_N(PICTURE_TIMING,           1, 0, pic_timing);
>          SEI_TYPE_N(PAN_SCAN_RECT,            1, 0, pan_scan_rect);
>          SEI_TYPE_S(USER_DATA_REGISTERED_ITU_T_T35,
> @@ -2125,7 +2177,16 @@ static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw,
>          }
>      }
>  
> -    if (byte_alignment(rw)) {
> +    // more_data_in_payload()
> +#ifdef READ
> +    current_position = get_bits_count(rw) - start_position;
> +    if (current_position != 8 * current->payload_size) {

The reading process does not use a GetBitContext of its own, so there is
no automatic check that one hasn't already overread into the next SEI
message; so better use < instead of !=.

> +#else
> +    current_position = put_bits_count(rw) - start_position;
> +    if (byte_alignment(rw) || more_data) {
> +#endif
> +        CHECK(FUNC(payload_extension)(ctx, rw, &current->extension_data,
> +                                      current->payload_size, current_position));
>          fixed(1, bit_equal_to_one, 1);
>          while (byte_alignment(rw))
>              fixed(1, bit_equal_to_zero, 0);
> 

During reading the following code follows after this:

    end_position = get_bits_count(rw);
    if (end_position < start_position + 8 * current->payload_size) {
        av_log(ctx->log_ctx, AV_LOG_ERROR, "Incorrect SEI payload length: "
               "header %"PRIu32" bits, actually %d bits.\n",
               8 * current->payload_size,
               end_position - start_position);
        return AVERROR_INVALIDDATA;
    }

Your commit makes this dead code, so you should either remove it or
actually check for overreading.

- Andreas