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

[James Almer]

On 4/23/2020 12:00 AM, Andreas Rheinhardt wrote:
> James Almer:
>> On 4/22/2020 9:17 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> Fixes ticket #8622
>>>>
>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>> ---
>>>> It fixes it assuming the Picture Timing in that sample is not being misparsed
>>>> by cbs_h265.
>>>> We're compliant with the latest revision of the spec, so any
>>>> reserved_payload_extension_data found in a sample created today is almost
>>>> guaranteed to be bogus. But the spec states that they should be skipped if
>>>> found, for forward compatibility reasons.
>>>> Would be worth checking if the nvenc encoder is at fault, writing faulty SEI
>>>> messages.
>>>>
>>>> This patch could for that matter make many potential cases of undiscovered
>>>> cbs_h265 SEI misparsing be handled as if the remaining bits were these reserved
>>>> bits.
>>>
>>> That's unfortunately true. I don't see a way to avoid that.
>>>
>>>>
>>>>  libavcodec/cbs_h265.h                 |  1 +
>>>>  libavcodec/cbs_h265_syntax_template.c | 44 +++++++++++++++++++++++++--
>>>>  2 files changed, 43 insertions(+), 2 deletions(-)
>>>>
>>>> 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 f978e16549..ef3254d27f 100644
>>>> --- a/libavcodec/cbs_h265_syntax_template.c
>>>> +++ b/libavcodec/cbs_h265_syntax_template.c
>>>> @@ -2054,11 +2054,43 @@ 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, i;
>>>> +
>>>> +#ifdef READ
>>>> +    if (cbs_h265_payload_extension_present(rw, payload_size, cur_pos)) {
>>>> +        GetBitContext tmp = *rw;
>>>> +        int payload_zero_bits, bits_left = 8 * payload_size - cur_pos;
>>>> +        if (bits_left > 8)
>>>> +            skip_bits_long(&tmp, bits_left - 8);
>>>> +        payload_zero_bits = ff_ctz(get_bits(&tmp, FFMIN(bits_left, 8)));
>>>> +        if (payload_zero_bits >= bits_left - 1)
>>>> +            return AVERROR_INVALIDDATA;
>>>> +        current->bit_length = bits_left - payload_zero_bits - 1;
>>>> +        allocate(current->data, (current->bit_length + 7) / 8);
>>>> +        for (i = 0; i < current->bit_length; i++) {
>>>> +            uint8_t bit;
>>>> +            xu(1, reserved_payload_extension_data, bit, 0, 1, 0);
>>>> +            current->data[i / 8] |= bit << (7 - i % 8);
>>>> +        }
>>>> +    }
>>>> +#else
>>>> +    for (i = 0; i < current->bit_length; i++)
>>>> +        xu(1, reserved_payload_extension_data,
>>>> +           current->data[i / 8] >> (7 - i % 8) & 1, 0, 1, 0);
>>>
>>> Do we really need to write one bit at a time? Why not simply one byte at
>>> a time and then output the rest in one go? The specs treat it as a
>>> bitfield and not as individual bits, so I think we are free to group it
>>> as we please.
>>
>> Mmh, ok.
>>
>>>
>>> The same goes for the other extension_data() function, of course. And
>>> for reading.
>>
>> Doing it for the other function will affect the existing trace output,
>> so I'm inclined to leave it as is.
>>
>>>
>>> And shouldn't i be better size_t?
>>
>> The correct type would be ptrdiff_t, but we never bothered with it and
>> always used int.
>>
> 
> The bit_length field of the extension data struct is size_t. If you
> presume that extension data will always only be passed through, then you
> are on the safe side; if not, you are not.

You're not supposed to write extension data in SEI messages to begin
with. A passthrough of course does since it keeps the data as it was in
the source stream intact. So no bsf or encoder should ever try to fill
this manually with custom data, as it would just create potentially
invalid files once a SEI message type gets extended in a future revision
of the spec, like it happened with Buffering Period.
It's what really makes me want to figure out why the nvenc encoder added
this blank byte in this sample for the ticket reporter. It's creating
files that could just stop being valid in the future.

Still, the extension data struct does use size_t, so might as well do
the same here.

> 
>>>
>>>> +#endif
>>>> +
>>>> +    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;
>>>>  
>>>>  #ifdef READ
>>>>      start_position = get_bits_count(rw);
>>>> @@ -2122,7 +2154,15 @@ static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw,
>>>>          }
>>>>      }
>>>>  
>>>> -    if (byte_alignment(rw)) {
>>>> +#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) || current->extension_data.bit_length) {
>>>> +#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);
>>>>
>>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>