Message ID | 20200423233312.2910-1-jamrial@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | None | expand |
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
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))); +} + #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);
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(-)