| Message ID | 20200420214551.6318-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | 38d1815cc65dd447de80760895ee008cfc9a0091 |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avcodec/cbs_h265: fix writing extension_data bits | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 4/20/2020 6:45 PM, James Almer wrote: > Fixes ticket #8622 > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > It fixes it assuming the Picture Timing in that sample is not being misparsed > by cbs_h265. > We're compliant with the latest revision of the spec, so any > reserved_payload_extension_data found in a sample created today is almost > guaranteed to be bogus. But the spec states that they should be skipped if > found, for forward compatibility reasons. > Would be worth checking if the nvenc encoder is at fault, writing faulty SEI > messages. > > This patch could for that matter make many potential cases of undiscovered > cbs_h265 SEI misparsing be handled as if the remaining bits were these reserved > bits. > > libavcodec/cbs_h265.h | 1 + > libavcodec/cbs_h265_syntax_template.c | 44 +++++++++++++++++++++++++-- > 2 files changed, 43 insertions(+), 2 deletions(-) > > 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; Added an av_buffer_unref() call in cbs_h265_free_sei_payload() cbs_h2645.c locally. Else the buffer will leak. > } H265RawSEIPayload; > > typedef struct H265RawSEI { > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c > index f978e16549..ef3254d27f 100644 > --- a/libavcodec/cbs_h265_syntax_template.c > +++ b/libavcodec/cbs_h265_syntax_template.c > @@ -2054,11 +2054,43 @@ 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, i; > + > +#ifdef READ > + if (cbs_h265_payload_extension_present(rw, payload_size, cur_pos)) { > + GetBitContext tmp = *rw; > + int payload_zero_bits, bits_left = 8 * payload_size - cur_pos; > + if (bits_left > 8) > + skip_bits_long(&tmp, bits_left - 8); > + payload_zero_bits = ff_ctz(get_bits(&tmp, FFMIN(bits_left, 8))); > + if (payload_zero_bits >= bits_left - 1) > + return AVERROR_INVALIDDATA; > + current->bit_length = bits_left - payload_zero_bits - 1; > + allocate(current->data, (current->bit_length + 7) / 8); > + for (i = 0; i < current->bit_length; i++) { > + uint8_t bit; > + xu(1, reserved_payload_extension_data, bit, 0, 1, 0); > + current->data[i / 8] |= bit << (7 - i % 8); > + } > + } > +#else > + for (i = 0; i < current->bit_length; i++) > + xu(1, reserved_payload_extension_data, > + current->data[i / 8] >> (7 - i % 8) & 1, 0, 1, 0); > +#endif > + > + 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; > > #ifdef READ > start_position = get_bits_count(rw); > @@ -2122,7 +2154,15 @@ static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw, > } > } > > - if (byte_alignment(rw)) { > +#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) || current->extension_data.bit_length) { > +#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); >
James Almer: > Will be reused in the following patch. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/cbs_h2645.c | 9 +++++++++ > libavcodec/cbs_h265_syntax_template.c | 8 +++----- > 2 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c > index d42073cc5a..dffff862e2 100644 > --- a/libavcodec/cbs_h2645.c > +++ b/libavcodec/cbs_h2645.c > @@ -233,6 +233,15 @@ static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc, > return 0; > } > > +static int cbs_h265_payload_extension_present(GetBitContext *gbc, uint32_t payload_size, > + int cur_pos) > +{ > + int bits_left; > + bits_left = payload_size * 8 - cur_pos; > + return (bits_left > 0 && > + (bits_left > 7 || ff_ctz(show_bits(gbc, bits_left)) < bits_left - 1)); > +} > + > #define HEADER(name) do { \ > ff_cbs_trace_header(ctx, name); \ > } while (0) > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c > index fe5ffac80f..f978e16549 100644 > --- a/libavcodec/cbs_h265_syntax_template.c > +++ b/libavcodec/cbs_h265_syntax_template.c > @@ -1568,7 +1568,7 @@ static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw, > int err, i, length; > > #ifdef READ > - int start_pos, end_pos, bits_left; > + int start_pos; > start_pos = get_bits_count(rw); > #endif > > @@ -1649,10 +1649,8 @@ static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw, > #ifdef READ > // payload_extension_present() - true if we are before the last 1-bit > // in the payload structure, which must be in the last byte. > - end_pos = get_bits_count(rw); > - bits_left = *payload_size * 8 - (end_pos - start_pos); > - if (bits_left > 0 && > - (bits_left > 7 || ff_ctz(show_bits(rw, bits_left)) < bits_left - 1)) > + if (cbs_h265_payload_extension_present(rw, *payload_size, > + get_bits_count(rw) - start_pos)) > flag(use_alt_cpb_params_flag); > else > infer(use_alt_cpb_params_flag, 0); > The value of ff_ctz is undefined if the argument is zero. It can be zero for invalid input (namely if all of the bits_left are zero). You should instead use a check like show_bits(gbc, bits_left) & MAX_UINT_BITS(bits_left - 1). (In this situation where you are only reading one bit it doesn't really matter - invalid input will always be detected as such if ff_ctz returns something >= 0. But if we use this later in a scenario where it is about more than just one bit, this can make invalid input slip through.) This is similar to d4035ca849bdb90e95c87e2737a99ea657be0716. - Andreas
James Almer: > Fixes ticket #8622 > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > It fixes it assuming the Picture Timing in that sample is not being misparsed > by cbs_h265. > We're compliant with the latest revision of the spec, so any > reserved_payload_extension_data found in a sample created today is almost > guaranteed to be bogus. But the spec states that they should be skipped if > found, for forward compatibility reasons. > Would be worth checking if the nvenc encoder is at fault, writing faulty SEI > messages. > > This patch could for that matter make many potential cases of undiscovered > cbs_h265 SEI misparsing be handled as if the remaining bits were these reserved > bits. > > libavcodec/cbs_h265.h | 1 + > libavcodec/cbs_h265_syntax_template.c | 44 +++++++++++++++++++++++++-- > 2 files changed, 43 insertions(+), 2 deletions(-) > > 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 f978e16549..ef3254d27f 100644 > --- a/libavcodec/cbs_h265_syntax_template.c > +++ b/libavcodec/cbs_h265_syntax_template.c > @@ -2054,11 +2054,43 @@ 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, i; > + > +#ifdef READ > + if (cbs_h265_payload_extension_present(rw, payload_size, cur_pos)) { > + GetBitContext tmp = *rw; > + int payload_zero_bits, bits_left = 8 * payload_size - cur_pos; > + if (bits_left > 8) > + skip_bits_long(&tmp, bits_left - 8); > + payload_zero_bits = ff_ctz(get_bits(&tmp, FFMIN(bits_left, 8))); Same as in the last mail. Replace the following check with if (!get_bits(&tmp, FFMIN(bits_left, 8))). > + if (payload_zero_bits >= bits_left - 1) > + return AVERROR_INVALIDDATA; > + current->bit_length = bits_left - payload_zero_bits - 1; > + allocate(current->data, (current->bit_length + 7) / 8); > + for (i = 0; i < current->bit_length; i++) { > + uint8_t bit; > + xu(1, reserved_payload_extension_data, bit, 0, 1, 0); > + current->data[i / 8] |= bit << (7 - i % 8); > + } > + } > +#else > + for (i = 0; i < current->bit_length; i++) > + xu(1, reserved_payload_extension_data, > + current->data[i / 8] >> (7 - i % 8) & 1, 0, 1, 0); > +#endif > + > + 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; > > #ifdef READ > start_position = get_bits_count(rw); > @@ -2122,7 +2154,15 @@ static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw, > } > } > > - if (byte_alignment(rw)) { > +#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) || current->extension_data.bit_length) { > +#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); >
James Almer: > Fixes ticket #8622 > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > It fixes it assuming the Picture Timing in that sample is not being misparsed > by cbs_h265. > We're compliant with the latest revision of the spec, so any > reserved_payload_extension_data found in a sample created today is almost > guaranteed to be bogus. But the spec states that they should be skipped if > found, for forward compatibility reasons. > Would be worth checking if the nvenc encoder is at fault, writing faulty SEI > messages. > > This patch could for that matter make many potential cases of undiscovered > cbs_h265 SEI misparsing be handled as if the remaining bits were these reserved > bits. That's unfortunately true. I don't see a way to avoid that. > > libavcodec/cbs_h265.h | 1 + > libavcodec/cbs_h265_syntax_template.c | 44 +++++++++++++++++++++++++-- > 2 files changed, 43 insertions(+), 2 deletions(-) > > 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 f978e16549..ef3254d27f 100644 > --- a/libavcodec/cbs_h265_syntax_template.c > +++ b/libavcodec/cbs_h265_syntax_template.c > @@ -2054,11 +2054,43 @@ 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, i; > + > +#ifdef READ > + if (cbs_h265_payload_extension_present(rw, payload_size, cur_pos)) { > + GetBitContext tmp = *rw; > + int payload_zero_bits, bits_left = 8 * payload_size - cur_pos; > + if (bits_left > 8) > + skip_bits_long(&tmp, bits_left - 8); > + payload_zero_bits = ff_ctz(get_bits(&tmp, FFMIN(bits_left, 8))); > + if (payload_zero_bits >= bits_left - 1) > + return AVERROR_INVALIDDATA; > + current->bit_length = bits_left - payload_zero_bits - 1; > + allocate(current->data, (current->bit_length + 7) / 8); > + for (i = 0; i < current->bit_length; i++) { > + uint8_t bit; > + xu(1, reserved_payload_extension_data, bit, 0, 1, 0); > + current->data[i / 8] |= bit << (7 - i % 8); > + } > + } > +#else > + for (i = 0; i < current->bit_length; i++) > + xu(1, reserved_payload_extension_data, > + current->data[i / 8] >> (7 - i % 8) & 1, 0, 1, 0); Do we really need to write one bit at a time? Why not simply one byte at a time and then output the rest in one go? The specs treat it as a bitfield and not as individual bits, so I think we are free to group it as we please. The same goes for the other extension_data() function, of course. And for reading. And shouldn't i be better size_t? > +#endif > + > + 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; > > #ifdef READ > start_position = get_bits_count(rw); > @@ -2122,7 +2154,15 @@ static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw, > } > } > > - if (byte_alignment(rw)) { > +#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) || current->extension_data.bit_length) { > +#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); >
On 4/22/2020 9:17 PM, Andreas Rheinhardt wrote: > James Almer: >> Fixes ticket #8622 >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> It fixes it assuming the Picture Timing in that sample is not being misparsed >> by cbs_h265. >> We're compliant with the latest revision of the spec, so any >> reserved_payload_extension_data found in a sample created today is almost >> guaranteed to be bogus. But the spec states that they should be skipped if >> found, for forward compatibility reasons. >> Would be worth checking if the nvenc encoder is at fault, writing faulty SEI >> messages. >> >> This patch could for that matter make many potential cases of undiscovered >> cbs_h265 SEI misparsing be handled as if the remaining bits were these reserved >> bits. > > That's unfortunately true. I don't see a way to avoid that. > >> >> libavcodec/cbs_h265.h | 1 + >> libavcodec/cbs_h265_syntax_template.c | 44 +++++++++++++++++++++++++-- >> 2 files changed, 43 insertions(+), 2 deletions(-) >> >> 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 f978e16549..ef3254d27f 100644 >> --- a/libavcodec/cbs_h265_syntax_template.c >> +++ b/libavcodec/cbs_h265_syntax_template.c >> @@ -2054,11 +2054,43 @@ 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, i; >> + >> +#ifdef READ >> + if (cbs_h265_payload_extension_present(rw, payload_size, cur_pos)) { >> + GetBitContext tmp = *rw; >> + int payload_zero_bits, bits_left = 8 * payload_size - cur_pos; >> + if (bits_left > 8) >> + skip_bits_long(&tmp, bits_left - 8); >> + payload_zero_bits = ff_ctz(get_bits(&tmp, FFMIN(bits_left, 8))); >> + if (payload_zero_bits >= bits_left - 1) >> + return AVERROR_INVALIDDATA; >> + current->bit_length = bits_left - payload_zero_bits - 1; >> + allocate(current->data, (current->bit_length + 7) / 8); >> + for (i = 0; i < current->bit_length; i++) { >> + uint8_t bit; >> + xu(1, reserved_payload_extension_data, bit, 0, 1, 0); >> + current->data[i / 8] |= bit << (7 - i % 8); >> + } >> + } >> +#else >> + for (i = 0; i < current->bit_length; i++) >> + xu(1, reserved_payload_extension_data, >> + current->data[i / 8] >> (7 - i % 8) & 1, 0, 1, 0); > > Do we really need to write one bit at a time? Why not simply one byte at > a time and then output the rest in one go? The specs treat it as a > bitfield and not as individual bits, so I think we are free to group it > as we please. Mmh, ok. > > The same goes for the other extension_data() function, of course. And > for reading. Doing it for the other function will affect the existing trace output, so I'm inclined to leave it as is. > > And shouldn't i be better size_t? The correct type would be ptrdiff_t, but we never bothered with it and always used int. > >> +#endif >> + >> + 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; >> >> #ifdef READ >> start_position = get_bits_count(rw); >> @@ -2122,7 +2154,15 @@ static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw, >> } >> } >> >> - if (byte_alignment(rw)) { >> +#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) || current->extension_data.bit_length) { >> +#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); >> > > _______________________________________________ > 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/22/2020 9:17 PM, Andreas Rheinhardt wrote: >> James Almer: >>> Fixes ticket #8622 >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> It fixes it assuming the Picture Timing in that sample is not being misparsed >>> by cbs_h265. >>> We're compliant with the latest revision of the spec, so any >>> reserved_payload_extension_data found in a sample created today is almost >>> guaranteed to be bogus. But the spec states that they should be skipped if >>> found, for forward compatibility reasons. >>> Would be worth checking if the nvenc encoder is at fault, writing faulty SEI >>> messages. >>> >>> This patch could for that matter make many potential cases of undiscovered >>> cbs_h265 SEI misparsing be handled as if the remaining bits were these reserved >>> bits. >> >> That's unfortunately true. I don't see a way to avoid that. >> >>> >>> libavcodec/cbs_h265.h | 1 + >>> libavcodec/cbs_h265_syntax_template.c | 44 +++++++++++++++++++++++++-- >>> 2 files changed, 43 insertions(+), 2 deletions(-) >>> >>> 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 f978e16549..ef3254d27f 100644 >>> --- a/libavcodec/cbs_h265_syntax_template.c >>> +++ b/libavcodec/cbs_h265_syntax_template.c >>> @@ -2054,11 +2054,43 @@ 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, i; >>> + >>> +#ifdef READ >>> + if (cbs_h265_payload_extension_present(rw, payload_size, cur_pos)) { >>> + GetBitContext tmp = *rw; >>> + int payload_zero_bits, bits_left = 8 * payload_size - cur_pos; >>> + if (bits_left > 8) >>> + skip_bits_long(&tmp, bits_left - 8); >>> + payload_zero_bits = ff_ctz(get_bits(&tmp, FFMIN(bits_left, 8))); >>> + if (payload_zero_bits >= bits_left - 1) >>> + return AVERROR_INVALIDDATA; >>> + current->bit_length = bits_left - payload_zero_bits - 1; >>> + allocate(current->data, (current->bit_length + 7) / 8); >>> + for (i = 0; i < current->bit_length; i++) { >>> + uint8_t bit; >>> + xu(1, reserved_payload_extension_data, bit, 0, 1, 0); >>> + current->data[i / 8] |= bit << (7 - i % 8); >>> + } >>> + } >>> +#else >>> + for (i = 0; i < current->bit_length; i++) >>> + xu(1, reserved_payload_extension_data, >>> + current->data[i / 8] >> (7 - i % 8) & 1, 0, 1, 0); >> >> Do we really need to write one bit at a time? Why not simply one byte at >> a time and then output the rest in one go? The specs treat it as a >> bitfield and not as individual bits, so I think we are free to group it >> as we please. > > Mmh, ok. > >> >> The same goes for the other extension_data() function, of course. And >> for reading. > > Doing it for the other function will affect the existing trace output, > so I'm inclined to leave it as is. > >> >> And shouldn't i be better size_t? > > The correct type would be ptrdiff_t, but we never bothered with it and > always used int. > The bit_length field of the extension data struct is size_t. If you presume that extension data will always only be passed through, then you are on the safe side; if not, you are not. >> >>> +#endif >>> + >>> + 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; >>> >>> #ifdef READ >>> start_position = get_bits_count(rw); >>> @@ -2122,7 +2154,15 @@ static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw, >>> } >>> } >>> >>> - if (byte_alignment(rw)) { >>> +#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) || current->extension_data.bit_length) { >>> +#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); >>> >>
On 4/23/2020 12:00 AM, Andreas Rheinhardt wrote: > James Almer: >> On 4/22/2020 9:17 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> Fixes ticket #8622 >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> It fixes it assuming the Picture Timing in that sample is not being misparsed >>>> by cbs_h265. >>>> We're compliant with the latest revision of the spec, so any >>>> reserved_payload_extension_data found in a sample created today is almost >>>> guaranteed to be bogus. But the spec states that they should be skipped if >>>> found, for forward compatibility reasons. >>>> Would be worth checking if the nvenc encoder is at fault, writing faulty SEI >>>> messages. >>>> >>>> This patch could for that matter make many potential cases of undiscovered >>>> cbs_h265 SEI misparsing be handled as if the remaining bits were these reserved >>>> bits. >>> >>> That's unfortunately true. I don't see a way to avoid that. >>> >>>> >>>> libavcodec/cbs_h265.h | 1 + >>>> libavcodec/cbs_h265_syntax_template.c | 44 +++++++++++++++++++++++++-- >>>> 2 files changed, 43 insertions(+), 2 deletions(-) >>>> >>>> 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 f978e16549..ef3254d27f 100644 >>>> --- a/libavcodec/cbs_h265_syntax_template.c >>>> +++ b/libavcodec/cbs_h265_syntax_template.c >>>> @@ -2054,11 +2054,43 @@ 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, i; >>>> + >>>> +#ifdef READ >>>> + if (cbs_h265_payload_extension_present(rw, payload_size, cur_pos)) { >>>> + GetBitContext tmp = *rw; >>>> + int payload_zero_bits, bits_left = 8 * payload_size - cur_pos; >>>> + if (bits_left > 8) >>>> + skip_bits_long(&tmp, bits_left - 8); >>>> + payload_zero_bits = ff_ctz(get_bits(&tmp, FFMIN(bits_left, 8))); >>>> + if (payload_zero_bits >= bits_left - 1) >>>> + return AVERROR_INVALIDDATA; >>>> + current->bit_length = bits_left - payload_zero_bits - 1; >>>> + allocate(current->data, (current->bit_length + 7) / 8); >>>> + for (i = 0; i < current->bit_length; i++) { >>>> + uint8_t bit; >>>> + xu(1, reserved_payload_extension_data, bit, 0, 1, 0); >>>> + current->data[i / 8] |= bit << (7 - i % 8); >>>> + } >>>> + } >>>> +#else >>>> + for (i = 0; i < current->bit_length; i++) >>>> + xu(1, reserved_payload_extension_data, >>>> + current->data[i / 8] >> (7 - i % 8) & 1, 0, 1, 0); >>> >>> Do we really need to write one bit at a time? Why not simply one byte at >>> a time and then output the rest in one go? The specs treat it as a >>> bitfield and not as individual bits, so I think we are free to group it >>> as we please. >> >> Mmh, ok. >> >>> >>> The same goes for the other extension_data() function, of course. And >>> for reading. >> >> Doing it for the other function will affect the existing trace output, >> so I'm inclined to leave it as is. >> >>> >>> And shouldn't i be better size_t? >> >> The correct type would be ptrdiff_t, but we never bothered with it and >> always used int. >> > > The bit_length field of the extension data struct is size_t. If you > presume that extension data will always only be passed through, then you > are on the safe side; if not, you are not. You're not supposed to write extension data in SEI messages to begin with. A passthrough of course does since it keeps the data as it was in the source stream intact. So no bsf or encoder should ever try to fill this manually with custom data, as it would just create potentially invalid files once a SEI message type gets extended in a future revision of the spec, like it happened with Buffering Period. It's what really makes me want to figure out why the nvenc encoder added this blank byte in this sample for the ticket reporter. It's creating files that could just stop being valid in the future. Still, the extension data struct does use size_t, so might as well do the same here. > >>> >>>> +#endif >>>> + >>>> + 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; >>>> >>>> #ifdef READ >>>> start_position = get_bits_count(rw); >>>> @@ -2122,7 +2154,15 @@ static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw, >>>> } >>>> } >>>> >>>> - if (byte_alignment(rw)) { >>>> +#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) || current->extension_data.bit_length) { >>>> +#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); >>>> >>> > _______________________________________________ > 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". >
On 4/20/2020 6:45 PM, James Almer wrote: > We only care about the right most bit. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Fixes handling files like > https://trac.ffmpeg.org/attachment/ticket/7965/puppets_with_alpha_hevc.mov > Without this patch, parsing works but passing the VPS through hevc_metadata_bsf > when writing fails. > > libavcodec/cbs_h265_syntax_template.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c > index 180a045c34..85b952e64c 100644 > --- a/libavcodec/cbs_h265_syntax_template.c > +++ b/libavcodec/cbs_h265_syntax_template.c > @@ -80,7 +80,7 @@ static int FUNC(extension_data)(CodedBitstreamContext *ctx, RWContext *rw, > } > #else > for (k = 0; k < current->bit_length; k++) > - xu(1, extension_data, current->data[k / 8] >> (7 - k % 8), 0, 1, 0); > + xu(1, extension_data, current->data[k / 8] >> (7 - k % 8) & 1, 0, 1, 0); > #endif > return 0; > } I'll apply this soon.
diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c index 180a045c34..85b952e64c 100644 --- a/libavcodec/cbs_h265_syntax_template.c +++ b/libavcodec/cbs_h265_syntax_template.c @@ -80,7 +80,7 @@ static int FUNC(extension_data)(CodedBitstreamContext *ctx, RWContext *rw, } #else for (k = 0; k < current->bit_length; k++) - xu(1, extension_data, current->data[k / 8] >> (7 - k % 8), 0, 1, 0); + xu(1, extension_data, current->data[k / 8] >> (7 - k % 8) & 1, 0, 1, 0); #endif return 0; }
We only care about the right most bit. Signed-off-by: James Almer <jamrial@gmail.com> --- Fixes handling files like https://trac.ffmpeg.org/attachment/ticket/7965/puppets_with_alpha_hevc.mov Without this patch, parsing works but passing the VPS through hevc_metadata_bsf when writing fails. libavcodec/cbs_h265_syntax_template.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)