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

Fixes ticket #8622

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/cbs_h2645.c                |  1 +
 libavcodec/cbs_h265.h                 |  1 +
 libavcodec/cbs_h265_syntax_template.c | 85 +++++++++++++++++++++------
 3 files changed, 70 insertions(+), 17 deletions(-)
  

On 4/30/2020 6:50 PM, James Almer wrote:
> Fixes ticket #8622
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/cbs_h2645.c                |  1 +
>  libavcodec/cbs_h265.h                 |  1 +
>  libavcodec/cbs_h265_syntax_template.c | 85 +++++++++++++++++++++------
>  3 files changed, 70 insertions(+), 17 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
> 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..d3ac618db6 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;
> +    int more_data = !!current->extension_data.bit_length;
>  
>  #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,24 +2177,23 @@ 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) {
> +#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);
>      }
>  
> -#ifdef READ
> -    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;
> -    }
> -#else
> -    end_position = put_bits_count(rw);
> -    current->payload_size = (end_position - start_position) >> 3;
> +#ifdef WRITE
> +    current->payload_size = (put_bits_count(rw) - start_position) >> 3;
>  #endif
>  
>      return 0;

Will apply the set soon.
  

On 30/04/2020 22:50, James Almer wrote:
> Fixes ticket #8622
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/cbs_h2645.c                |  1 +
>  libavcodec/cbs_h265.h                 |  1 +
>  libavcodec/cbs_h265_syntax_template.c | 85 +++++++++++++++++++++------
>  3 files changed, 70 insertions(+), 17 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
> 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..d3ac618db6 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;
> +    int more_data = !!current->extension_data.bit_length;
>  
>  #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,24 +2177,23 @@ 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) {
> +#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);
>      }
>  
> -#ifdef READ
> -    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;
> -    }
> -#else
> -    end_position = put_bits_count(rw);
> -    current->payload_size = (end_position - start_position) >> 3;
> +#ifdef WRITE
> +    current->payload_size = (put_bits_count(rw) - start_position) >> 3;
>  #endif
>  
>      return 0;
> 

Could we avoid some of the extra code here by making a new GetBitContext in sei_payload containing only the payload bits and passing that to the per-type SEI reading functions?  The functions themselves would know whether any additional bits are present purely from get_bits_left() - buffering_period could then be a normal SEI_TYPE_N and the code in it would be simpler.

- Mark
  

On 5/3/2020 12:39 PM, Mark Thompson wrote:
> On 30/04/2020 22:50, James Almer wrote:
>> Fixes ticket #8622
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/cbs_h2645.c                |  1 +
>>  libavcodec/cbs_h265.h                 |  1 +
>>  libavcodec/cbs_h265_syntax_template.c | 85 +++++++++++++++++++++------
>>  3 files changed, 70 insertions(+), 17 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
>> 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..d3ac618db6 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;
>> +    int more_data = !!current->extension_data.bit_length;
>>  
>>  #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,24 +2177,23 @@ 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) {
>> +#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);
>>      }
>>  
>> -#ifdef READ
>> -    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;
>> -    }
>> -#else
>> -    end_position = put_bits_count(rw);
>> -    current->payload_size = (end_position - start_position) >> 3;
>> +#ifdef WRITE
>> +    current->payload_size = (put_bits_count(rw) - start_position) >> 3;
>>  #endif
>>  
>>      return 0;
>>
> 
> Could we avoid some of the extra code here by making a new GetBitContext in sei_payload containing only the payload bits and passing that to the per-type SEI reading functions?  The functions themselves would know whether any additional bits are present purely from get_bits_left() - buffering_period could then be a normal SEI_TYPE_N and the code in it would be simpler.

The SEI_TYPE_E macro is for the writing scenario, where we can't always
know until the message is finished if we have to write extra bits or not.
For Buffering Period, if we have some extra payload data that we need to
pass through, then use_alt_cpb_params_flag must be written, be it 1 or
0. If we don't have extra payload data that we need to pass through, but
we wrote use_alt_cpb_params_flag because it was 1, then the calling
function cbs_h265_write_sei_payload() needs to know this happened so it
may always write a bit_equal_to_1 and potentially also a bunch of
bit_equal_to_0, even if after use_alt_cpb_params_flag we were byte
aligned, to ensure decoders compliant with any spec revision will not
mistake it for something else.

Making each SEI message have its own GetBitContext in a reading scenario
will not make a difference for the above.

> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
  

Mark Thompson:
> On 30/04/2020 22:50, James Almer wrote:
>> Fixes ticket #8622
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/cbs_h2645.c                |  1 +
>>  libavcodec/cbs_h265.h                 |  1 +
>>  libavcodec/cbs_h265_syntax_template.c | 85 +++++++++++++++++++++------
>>  3 files changed, 70 insertions(+), 17 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
>> 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..d3ac618db6 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;
>> +    int more_data = !!current->extension_data.bit_length;
>>  
>>  #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,24 +2177,23 @@ 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) {
>> +#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);
>>      }
>>  
>> -#ifdef READ
>> -    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;
>> -    }
>> -#else
>> -    end_position = put_bits_count(rw);
>> -    current->payload_size = (end_position - start_position) >> 3;
>> +#ifdef WRITE
>> +    current->payload_size = (put_bits_count(rw) - start_position) >> 3;
>>  #endif
>>  
>>      return 0;
>>
> 
> Could we avoid some of the extra code here by making a new GetBitContext in sei_payload containing only the payload bits and passing that to the per-type SEI reading functions?  The functions themselves would know whether any additional bits are present purely from get_bits_left() - buffering_period could then be a normal SEI_TYPE_N and the code in it would be simpler.
> 
If by "only the payload bits" you mean only the pits of a single SEI
message, then this will change trace output (the bit count would be
reset for every SEI message, yet the payload size and payload type would
still be parsed via the GetBitContext for the whole SEI RBSP which would
be weird); if one only modifies the end of the GetBitContext, then I
don't see a reason why this should not be possible.
But notice that this will add a check for overreading to the code; right
now it is not an error to overread into the next SEI message. Was there
actually a rationale behind this (in contrast not consuming all bytes
was always an error)?

But this will not make buffering_period a SEI_TYPE_N. This is done so
that one knows whether one has to add a zero bit after having written a
SEI message even when one is byte-aligned (imagine writing a buffering
period SEI with use_alt_cpb_params_flag equal to 1 that is byte aligned
after use_alt_cpb_params_flag; if no 0x80 were added, the
use_alt_cpb_params_flag would be mistaken for the bit_equal_to_one on
reading).

- Andreas
  

On 03/05/2020 16:55, James Almer wrote:
> On 5/3/2020 12:39 PM, Mark Thompson wrote:
>> On 30/04/2020 22:50, James Almer wrote:
>>> Fixes ticket #8622
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>  libavcodec/cbs_h2645.c                |  1 +
>>>  libavcodec/cbs_h265.h                 |  1 +
>>>  libavcodec/cbs_h265_syntax_template.c | 85 +++++++++++++++++++++------
>>>  3 files changed, 70 insertions(+), 17 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
>>> 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..d3ac618db6 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;
>>> +    int more_data = !!current->extension_data.bit_length;
>>>  
>>>  #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,24 +2177,23 @@ 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) {
>>> +#else
>>> +    current_position = put_bits_count(rw) - start_position;
>>> +    if (byte_alignment(rw) || more_data) {
>>> +#endif

Maybe put the { outside the #if so that it matches cleanly?

>>> +        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);
>>>      }
>>>  
>>> -#ifdef READ
>>> -    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;
>>> -    }
>>> -#else
>>> -    end_position = put_bits_count(rw);
>>> -    current->payload_size = (end_position - start_position) >> 3;
>>> +#ifdef WRITE
>>> +    current->payload_size = (put_bits_count(rw) - start_position) >> 3;
>>>  #endif
>>>  
>>>      return 0;
>>>
>>
>> Could we avoid some of the extra code here by making a new GetBitContext in sei_payload containing only the payload bits and passing that to the per-type SEI reading functions?  The functions themselves would know whether any additional bits are present purely from get_bits_left() - buffering_period could then be a normal SEI_TYPE_N and the code in it would be simpler.
> 
> The SEI_TYPE_E macro is for the writing scenario, where we can't always
> know until the message is finished if we have to write extra bits or not.
> For Buffering Period, if we have some extra payload data that we need to
> pass through, then use_alt_cpb_params_flag must be written, be it 1 or
> 0. If we don't have extra payload data that we need to pass through, but
> we wrote use_alt_cpb_params_flag because it was 1, then the calling
> function cbs_h265_write_sei_payload() needs to know this happened so it
> may always write a bit_equal_to_1 and potentially also a bunch of
> bit_equal_to_0, even if after use_alt_cpb_params_flag we were byte
> aligned, to ensure decoders compliant with any spec revision will not
> mistake it for something else.
> 
> Making each SEI message have its own GetBitContext in a reading scenario
> will not make a difference for the above.

Ok, that makes sense.  (I was hoping that we might be able to make the ugly SEI code cleaner, but apparently not today.  Oh well.)

LGTM.

Thanks,

- Mark
  

On 5/3/2020 7:22 PM, Mark Thompson wrote:
> On 03/05/2020 16:55, James Almer wrote:
>> On 5/3/2020 12:39 PM, Mark Thompson wrote:
>>> On 30/04/2020 22:50, James Almer wrote:
>>>> Fixes ticket #8622
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>  libavcodec/cbs_h2645.c                |  1 +
>>>>  libavcodec/cbs_h265.h                 |  1 +
>>>>  libavcodec/cbs_h265_syntax_template.c | 85 +++++++++++++++++++++------
>>>>  3 files changed, 70 insertions(+), 17 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
>>>> 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..d3ac618db6 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;
>>>> +    int more_data = !!current->extension_data.bit_length;
>>>>  
>>>>  #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,24 +2177,23 @@ 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) {
>>>> +#else
>>>> +    current_position = put_bits_count(rw) - start_position;
>>>> +    if (byte_alignment(rw) || more_data) {
>>>> +#endif
> 
> Maybe put the { outside the #if so that it matches cleanly?
> 
>>>> +        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);
>>>>      }
>>>>  
>>>> -#ifdef READ
>>>> -    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;
>>>> -    }
>>>> -#else
>>>> -    end_position = put_bits_count(rw);
>>>> -    current->payload_size = (end_position - start_position) >> 3;
>>>> +#ifdef WRITE
>>>> +    current->payload_size = (put_bits_count(rw) - start_position) >> 3;
>>>>  #endif
>>>>  
>>>>      return 0;
>>>>
>>>
>>> Could we avoid some of the extra code here by making a new GetBitContext in sei_payload containing only the payload bits and passing that to the per-type SEI reading functions?  The functions themselves would know whether any additional bits are present purely from get_bits_left() - buffering_period could then be a normal SEI_TYPE_N and the code in it would be simpler.
>>
>> The SEI_TYPE_E macro is for the writing scenario, where we can't always
>> know until the message is finished if we have to write extra bits or not.
>> For Buffering Period, if we have some extra payload data that we need to
>> pass through, then use_alt_cpb_params_flag must be written, be it 1 or
>> 0. If we don't have extra payload data that we need to pass through, but
>> we wrote use_alt_cpb_params_flag because it was 1, then the calling
>> function cbs_h265_write_sei_payload() needs to know this happened so it
>> may always write a bit_equal_to_1 and potentially also a bunch of
>> bit_equal_to_0, even if after use_alt_cpb_params_flag we were byte
>> aligned, to ensure decoders compliant with any spec revision will not
>> mistake it for something else.
>>
>> Making each SEI message have its own GetBitContext in a reading scenario
>> will not make a difference for the above.
> 
> Ok, that makes sense.  (I was hoping that we might be able to make the ugly SEI code cleaner, but apparently not today.  Oh well.)
> 
> LGTM.
> 
> Thanks,
> 
> - Mark

Pushed, thanks.