[FFmpeg-devel] [PATCH 3/4 v2] avcodec/cbs_h265: move the payload_extension_present check into its own function

[Andreas Rheinhardt]

James Almer:
> Will be reused in the following patch.
> 
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
>  libavcodec/cbs_h2645.c                | 9 +++++++++
>  libavcodec/cbs_h265_syntax_template.c | 7 +++----
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index d42073cc5a..a60b3217a0 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -233,6 +233,15 @@ static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
>      return 0;
>  }
>  
> +static int cbs_h265_payload_extension_present(GetBitContext *gbc, uint32_t payload_size,
> +                                              int cur_pos)
> +{
> +    int bits_left;
> +    bits_left = payload_size * 8 - cur_pos;

You could initialize it right away, but that is just a nit.

> +    return (bits_left > 0 &&
> +            (bits_left > 7 || show_bits(gbc, bits_left) & MAX_UINT_BITS(bits_left - 1)));
> +}
> +
>  #define HEADER(name) do { \
>          ff_cbs_trace_header(ctx, name); \
>      } while (0)
> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> index fe5ffac80f..96b9acc1dc 100644
> --- a/libavcodec/cbs_h265_syntax_template.c
> +++ b/libavcodec/cbs_h265_syntax_template.c
> @@ -1568,7 +1568,7 @@ static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
>      int err, i, length;
>  
>  #ifdef READ
> -    int start_pos, end_pos, bits_left;
> +    int start_pos, end_pos;
>      start_pos = get_bits_count(rw);
>  #endif
>  
> @@ -1650,9 +1650,8 @@ static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
>      // payload_extension_present() - true if we are before the last 1-bit
>      // in the payload structure, which must be in the last byte.
>      end_pos = get_bits_count(rw);
> -    bits_left = *payload_size * 8 - (end_pos - start_pos);
> -    if (bits_left > 0 &&
> -        (bits_left > 7 || ff_ctz(show_bits(rw, bits_left)) < bits_left - 1))
> +    if (cbs_h265_payload_extension_present(rw, *payload_size,
> +                                           end_pos - start_pos))
>          flag(use_alt_cpb_params_flag);
>      else
>          infer(use_alt_cpb_params_flag, 0);
> 
I just realized that there is a problem during writing here: If
use_alt_cpb_params_flag is one, it will be written and if the output is
byte-aligned after this, the generic sei_payload code thinks that this
SEI message is finished and will not add any
payload_bit_equal_to_one/zero afterwards (unless there would be even
more extension data present (presuming your patch 4/4 were already in)).

Upon reading this SEI message, the last 1 bit will be treated as if it
were the payload_bit_equal_to_one and the use_alt_cpb_params_flag will
be inferred to be absent and set to its default value (i.e. 0).

And if use_alt_cpb_params_flag is zero and further extension data is
present, use_alt_cpb_params_flag won't be written, yet with your patch
4/4 the further extension data will be written and when this SEI message
is read again, the first bit of the further extension data will be read
as use_alt_cpb_params.

This problem exists for every SEI message for which we parse something
that is extension data for older decoders. In other words: sei_payload()
needs to be informed if some already supported extension data has
already been written so that it always writes the
payload_bit_equal_to_one/zero. And the parsing code for individual SEI
messages needs to always write fields that are extension data when
further extension data is present regardless of whether the values
coincide with the default values.

Finally, we usually write fields upon passthrough even when they are set
to their default values as long as they were present in the source. This
is currently not true for use_alt_cpb_params_flag. Should this be
changed, too?

- Andreas