[FFmpeg-devel] [PATCH 3/3 v4] avcodec/cbs_h265: add missing support for reserved_payload_extension_data SEI bits
Mark Thompson
sw at jkqxz.net
Mon May 4 01:22:41 EEST 2020
On 03/05/2020 16:55, James Almer wrote:
> On 5/3/2020 12:39 PM, Mark Thompson wrote:
>> 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
Maybe put the { outside the #if so that it matches cleanly?
>>> + 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.
>
> The SEI_TYPE_E macro is for the writing scenario, where we can't always
> know until the message is finished if we have to write extra bits or not.
> For Buffering Period, if we have some extra payload data that we need to
> pass through, then use_alt_cpb_params_flag must be written, be it 1 or
> 0. If we don't have extra payload data that we need to pass through, but
> we wrote use_alt_cpb_params_flag because it was 1, then the calling
> function cbs_h265_write_sei_payload() needs to know this happened so it
> may always write a bit_equal_to_1 and potentially also a bunch of
> bit_equal_to_0, even if after use_alt_cpb_params_flag we were byte
> aligned, to ensure decoders compliant with any spec revision will not
> mistake it for something else.
>
> Making each SEI message have its own GetBitContext in a reading scenario
> will not make a difference for the above.
Ok, that makes sense. (I was hoping that we might be able to make the ugly SEI code cleaner, but apparently not today. Oh well.)
LGTM.
Thanks,
- Mark
More information about the ffmpeg-devel
mailing list