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

Will be reused in the following patch.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/cbs_h2645.c                | 9 +++++++++
 libavcodec/cbs_h265_syntax_template.c | 7 +++----
 2 files changed, 12 insertions(+), 4 deletions(-)

James Almer:
> Will be reused in the following patch.
> 
> Signed-off-by: James Almer <jamrial@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

James Almer:
> Will be reused in the following patch.
> 
> Signed-off-by: James Almer <jamrial@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;
> +    return (bits_left > 0 &&
> +            (bits_left > 7 || show_bits(gbc, bits_left) & MAX_UINT_BITS(bits_left - 1)));

After thinking a bit more about HEVC's reserved_payload_extension_data
mechanism I came to realize that it has two issues:

The first is that it allows to create SEI messages compliant with the
current standard, yet incompliant with the older one. Take the buffering
period SEI. In the current version of the standard it begins with the
syntax elements that said SEI always had followed by this:

if( payload_extension_present( ) )
    use_alt_cpb_params_flag (a one-bit flag)

which is followed by the following generic stuff for all SEI messages:

if( more_data_in_payload( ) ) {
    if( payload_extension_present( ) )
        reserved_payload_extension_data
    payload_bit_equal_to_one /* equal to 1 */
    while( !byte_aligned( ) )
        payload_bit_equal_to_zero /* equal to 0 */
}

The first problem is in the check more_data_in_payload( ). It's
placement after payload_extension_present() implies that it is possible
to create SEI messages that are valid for the current version of the
standard, yet are invalid data for a decoder implementing an older
version of the standard. This is possible if the position of the
bitstream before use_alt_cpb_params_flag() is 7 mod 8. If the next bit
is a bit equal to one and if it is the last bit of the SEI, then
use_alt_cpb_params_flag is not present for both a decoder implementing
the old as well as a decoder implementing the new version of the
standard (this bit will be the payload_bit_equal_to_one for both
decoders); yet if this bit is zero, then payload_extension_present() is
true and the new decoder will therefore infer that
use_alt_cpb_params_flag (with the value zero) is present. It is
perfectly legal according to the new standard to end the SEI message at
this point (for a new decoder, the more_data_in_payload() check will be
false), but the more_data_in_payload() check will be true for an old
decoder (because for it there still is one bit left in the SEI message);
yet the data left does not contain a single bit equal to one, so that
the SEI message is invalid for the old decoder.

In this case this problem is avoidable if the new encoder simply does
not write the use_alt_cpb_params_flag at all (if it is absent, zero
should be inferred anyway); yet this solution depends upon the inferred
value to be equal to zero.

Similarly it is impossible for an older decoder to know whether the bit
it thinks to be the payload_bit_equal_to_one is really the
payload_bit_equal_to_one according to a newer standard (it might be that
a new syntax element has been added to an SEI message in a newer version
of the standard and that the SEI message as seen by the new standard is
automatically byte-aligned, so that no payload_bit_equal_to_one is
needed according to the new standard). Granted, this is not really
important, because the old decoder is supposed to ignore the extension
data anyway.

(We should of course avoid this problem altogether by always writing the
payload_bit_equal_to_one if we have written any field that has been
added in a recent version of the standard to an already existing SEI
message type.)


The second problem is the actual definition of payload_extension_present():

"If the current position in the sei_payload( ) syntax structure is not
the position of the last (least significant, right-most) bit that is
equal to 1 that is less than 8 * payloadSize bits from the beginning of
the syntax structure (i.e., the position of the payload_bit_equal_to_one
syntax element), the return value of payload_extension_present( ) is
equal to TRUE."

So if one is already at the end (i.e. 8 * payloadSize bits from the
beginning of the syntax structure away), then
payload_extension_present() automatically returns true. So a buffering
period SEI message unit without payload_bit_equal_to_one (because what
has been written was already byte-aligned) written by an old encoder
will be considered invalid according to the new standard if one takes
the new standard literally (according to the new standard,
use_alt_cpb_params_flag is present because payload_extension_present()
is true at the end; yet there is no data left).

This also means that the above check in
cbs_h265_payload_extension_present() is spec-incompliant. To make it
spec-compliant, one would need to return 1 if no bits are left. Of
course, we should not make it spec-compliant.

I wonder whether the first issue is an intentional micro-optimization
designed for systems where only new decoders are used or if it was an
oversight. The second issue makes me believe that the first was an
oversight.

- Andreas