[FFmpeg-devel] [PATCH 3/3 v4] avcodec/cbs_h265: add missing support for reserved_payload_extension_data SEI bits
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Sun May 3 19:07:27 EEST 2020
Mark Thompson:
> On 30/04/2020 22:50, James Almer wrote:
>> Fixes ticket #8622
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> libavcodec/cbs_h2645.c | 1 +
>> libavcodec/cbs_h265.h | 1 +
>> libavcodec/cbs_h265_syntax_template.c | 85 +++++++++++++++++++++------
>> 3 files changed, 70 insertions(+), 17 deletions(-)
>>
>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> index 095e449ddc..b432921ecc 100644
>> --- a/libavcodec/cbs_h2645.c
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -553,6 +553,7 @@ static void cbs_h265_free_sei_payload(H265RawSEIPayload *payload)
>> av_buffer_unref(&payload->payload.other.data_ref);
>> break;
>> }
>> + av_buffer_unref(&payload->extension_data.data_ref);
>> }
>>
>> static void cbs_h265_free_sei(void *opaque, uint8_t *content)
>> 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 ed08b06e9c..d3ac618db6 100644
>> --- a/libavcodec/cbs_h265_syntax_template.c
>> +++ b/libavcodec/cbs_h265_syntax_template.c
>> @@ -1564,7 +1564,8 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>
>> static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
>> H265RawSEIBufferingPeriod *current,
>> - uint32_t *payload_size)
>> + uint32_t *payload_size,
>> + int *more_data)
>> {
>> CodedBitstreamH265Context *h265 = ctx->priv_data;
>> const H265RawSPS *sps;
>> @@ -1658,8 +1659,15 @@ static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
>> else
>> infer(use_alt_cpb_params_flag, 0);
>> #else
>> - if (current->use_alt_cpb_params_flag)
>> + // If unknown extension data exists, then use_alt_cpb_params_flag is
>> + // coded in the bitstream and must be written even if it's 0.
>> + if (current->use_alt_cpb_params_flag || *more_data) {
>> flag(use_alt_cpb_params_flag);
>> + // Ensure this bit is not the last in the payload by making the
>> + // more_data_in_payload() check evaluate to true, so it may not
>> + // be mistaken as something else by decoders.
>> + *more_data = 1;
>> + }
>> #endif
>>
>> return 0;
>> @@ -2057,11 +2065,48 @@ 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;
>> + size_t byte_length, k;
>> +
>> +#ifdef READ
>> + GetBitContext tmp;
>> + int bits_left, payload_zero_bits;
>> +
>> + if (!cbs_h265_payload_extension_present(rw, payload_size, cur_pos))
>> + return 0;
>> +
>> + bits_left = 8 * payload_size - cur_pos;
>> + tmp = *rw;
>> + if (bits_left > 8)
>> + skip_bits_long(&tmp, bits_left - 8);
>> + payload_zero_bits = get_bits(&tmp, FFMIN(bits_left, 8));
>> + if (!payload_zero_bits)
>> + return AVERROR_INVALIDDATA;
>> + payload_zero_bits = ff_ctz(payload_zero_bits);
>> + current->bit_length = bits_left - payload_zero_bits - 1;
>> + allocate(current->data, (current->bit_length + 7) / 8);
>> +#endif
>> +
>> + byte_length = (current->bit_length + 7) / 8;
>> + for (k = 0; k < byte_length; k++) {
>> + int length = FFMIN(current->bit_length - k * 8, 8);
>> + xu(length, reserved_payload_extension_data, current->data[k],
>> + 0, MAX_UINT_BITS(length), 0);
>> + }
>> +
>> + 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;
>> + int more_data = !!current->extension_data.bit_length;
>>
>> #ifdef READ
>> start_position = get_bits_count(rw);
>> @@ -2093,8 +2138,15 @@ static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw,
>> CHECK(FUNC(sei_ ## name)(ctx, rw, ¤t->payload.name, \
>> ¤t->payload_size)); \
>> break
>> +#define SEI_TYPE_E(type, prefix_valid, suffix_valid, name) \
>> + case HEVC_SEI_TYPE_ ## type: \
>> + SEI_TYPE_CHECK_VALID(name, prefix_valid, suffix_valid); \
>> + CHECK(FUNC(sei_ ## name)(ctx, rw, ¤t->payload.name, \
>> + ¤t->payload_size, \
>> + &more_data)); \
>> + break
>>
>> - SEI_TYPE_S(BUFFERING_PERIOD, 1, 0, buffering_period);
>> + SEI_TYPE_E(BUFFERING_PERIOD, 1, 0, buffering_period);
>> SEI_TYPE_N(PICTURE_TIMING, 1, 0, pic_timing);
>> SEI_TYPE_N(PAN_SCAN_RECT, 1, 0, pan_scan_rect);
>> SEI_TYPE_S(USER_DATA_REGISTERED_ITU_T_T35,
>> @@ -2125,24 +2177,23 @@ static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw,
>> }
>> }
>>
>> - if (byte_alignment(rw)) {
>> + // more_data_in_payload()
>> +#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) || more_data) {
>> +#endif
>> + CHECK(FUNC(payload_extension)(ctx, rw, ¤t->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);
>> }
>>
>> -#ifdef READ
>> - end_position = get_bits_count(rw);
>> - if (end_position < start_position + 8 * current->payload_size) {
>> - av_log(ctx->log_ctx, AV_LOG_ERROR, "Incorrect SEI payload length: "
>> - "header %"PRIu32" bits, actually %d bits.\n",
>> - 8 * current->payload_size,
>> - end_position - start_position);
>> - return AVERROR_INVALIDDATA;
>> - }
>> -#else
>> - end_position = put_bits_count(rw);
>> - current->payload_size = (end_position - start_position) >> 3;
>> +#ifdef WRITE
>> + current->payload_size = (put_bits_count(rw) - start_position) >> 3;
>> #endif
>>
>> return 0;
>>
>
> Could we avoid some of the extra code here by making a new GetBitContext in sei_payload containing only the payload bits and passing that to the per-type SEI reading functions? The functions themselves would know whether any additional bits are present purely from get_bits_left() - buffering_period could then be a normal SEI_TYPE_N and the code in it would be simpler.
>
If by "only the payload bits" you mean only the pits of a single SEI
message, then this will change trace output (the bit count would be
reset for every SEI message, yet the payload size and payload type would
still be parsed via the GetBitContext for the whole SEI RBSP which would
be weird); if one only modifies the end of the GetBitContext, then I
don't see a reason why this should not be possible.
But notice that this will add a check for overreading to the code; right
now it is not an error to overread into the next SEI message. Was there
actually a rationale behind this (in contrast not consuming all bytes
was always an error)?
But this will not make buffering_period a SEI_TYPE_N. This is done so
that one knows whether one has to add a zero bit after having written a
SEI message even when one is byte-aligned (imagine writing a buffering
period SEI with use_alt_cpb_params_flag equal to 1 that is byte aligned
after use_alt_cpb_params_flag; if no 0x80 were added, the
use_alt_cpb_params_flag would be mistaken for the bit_equal_to_one on
reading).
- Andreas
More information about the ffmpeg-devel
mailing list