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 <jamr...@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.
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.
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
