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

[Andreas Rheinhardt]

James Almer:
> On 4/30/2020 3:50 PM, Andreas Rheinhardt wrote:
>> 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.)
> 
> more_data is used as an argument when calling SEI messages using the new
> SEI_TYPE_E() macro, so that should prevent such warnings. I at least
> haven't seen any in my tests.
> 
But none of these functions will actually use it during reading and
therefore a smart compiler (maybe a future one) might find out that this
variable is write-only.

- Andreas