[FFmpeg-devel] [PATCH 4/4] 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>
> ---
> 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.

The same goes for the other extension_data() function, of course. And
for reading.

And shouldn't i be better size_t?

> +#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);
>