| Message ID | 20200430003314.1728-3-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/3,v2] avcodec/cbs_h265: rename H265RawPSExtensionData to H265RawExtensionData | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
James Almer: > Fixes ticket #8622 > > Signed-off-by: James Almer <jamrial@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.) > > #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 !=. > +#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. - Andreas
On 4/30/2020 3:50 PM, Andreas Rheinhardt wrote: > James Almer: >> Fixes ticket #8622 >> >> Signed-off-by: James Almer <jamrial@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
James Almer: > On 4/30/2020 3:50 PM, Andreas Rheinhardt wrote: >> James Almer: >>> Fixes ticket #8622 >>> >>> Signed-off-by: James Almer <jamrial@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. > But none of these functions will actually use it during reading and therefore a smart compiler (maybe a future one) might find out that this variable is write-only. - Andreas
On 4/30/2020 4:15 PM, Andreas Rheinhardt wrote: > James Almer: >> On 4/30/2020 3:50 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> Fixes ticket #8622 >>>> >>>> Signed-off-by: James Almer <jamrial@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. >> > But none of these functions will actually use it during reading and > therefore a smart compiler (maybe a future one) might find out that this > variable is write-only. Then we'll take care of it when overtly pedantic compilers start emitting such warnings. Right now, we have several functions in the tree where function arguments are unused, so I wouldn't worry. > > - Andreas > _______________________________________________ > 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". >
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..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; #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) { +#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);
Fixes ticket #8622 Signed-off-by: James Almer <jamrial@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(-)