[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;
> +    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