| Message ID | 20200430215054.3703-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | 3d51d3b42d0eee10301b06aaa814454c5e38eed6 |
| Headers | show |
| Series | None | expand |
On 4/30/2020 6:50 PM, James Almer wrote: > Fixes ticket #8622 > > Signed-off-by: James Almer <jamrial@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; Will apply the set soon.
On 30/04/2020 22:50, James Almer wrote: > Fixes ticket #8622 > > Signed-off-by: James Almer <jamrial@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. - Mark
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@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-request@ffmpeg.org with subject "unsubscribe". >
Mark Thompson: > On 30/04/2020 22:50, James Almer wrote: >> Fixes ticket #8622 >> >> Signed-off-by: James Almer <jamrial@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
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@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
On 5/3/2020 7:22 PM, Mark Thompson wrote: > 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@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 Pushed, thanks.
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;
Fixes ticket #8622 Signed-off-by: James Almer <jamrial@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(-)