[FFmpeg-devel] [PATCH 3/3 v3] avcodec/cbs_h265: add missing support for reserved_payload_extension_data SEI bits
James Almer
jamrial at gmail.com
Thu Apr 30 22:08:57 EEST 2020
On 4/30/2020 3:50 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Fixes ticket #8622
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> In writing scenarios, it will now ensure bit_equal_to_one is also always
>> written when currently defined extension data (like in Buffering Period) is
>> present, and not just when unknown extension data is.
>>
>> libavcodec/cbs_h2645.c | 1 +
>> libavcodec/cbs_h265.h | 1 +
>> libavcodec/cbs_h265_syntax_template.c | 71 +++++++++++++++++++++++++--
>> 3 files changed, 68 insertions(+), 5 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> --- a/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..45838467b7 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, end_position;
>> + int more_data = !!current->extension_data.bit_length;
>
> If I am not mistaken, then more_data is a write-only variable when
> reading. Better add av_unused or it might lead to compiler warnings.
> (You might even move the definition of more_data into the ifdef block
> below and only initialize it during writing.)
more_data is used as an argument when calling SEI messages using the new
SEI_TYPE_E() macro, so that should prevent such warnings. I at least
haven't seen any in my tests.
>
>>
>> #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,7 +2177,16 @@ 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) {
>
> The reading process does not use a GetBitContext of its own, so there is
> no automatic check that one hasn't already overread into the next SEI
> message; so better use < instead of !=.
Ok, changing locally.
>
>> +#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);
>>
>
> During reading the following code follows after this:
>
> 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;
> }
>
> Your commit makes this dead code, so you should either remove it or
> actually check for overreading.
You're right, will remove.
>
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list