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

James Almer jamrial at gmail.com
Mon May 4 01:56:58 EEST 2020


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


More information about the ffmpeg-devel mailing list