| Message ID | 20201206153603.6257-2-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/hevc_sei: split Dynamic HDR10+ SEI parsing into its own function | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
On Sun, Dec 6, 2020 at 5:44 PM James Almer <jamrial@gmail.com> wrote: > > Create a local one instead from a byte buffer input argument. > > Signed-off-by: James Almer <jamrial@gmail.com> The actual change mentioned in the commit message is LGTM, and the updated sanity checks seem good (since now the alternatives at that point are either 3+3 (2094-40) or 3+4 (closed captions)), but they are pretty separate from the actual change of this commit. So in that sense it might make sense to just separate them into their own fixup commit when pushing. Or at least mention the sanity check fixups in the commit message instead of them being a silent good change. Jan
On Mon, Dec 7, 2020 at 1:23 AM Jan Ekström <jeebjp@gmail.com> wrote: > > On Sun, Dec 6, 2020 at 5:44 PM James Almer <jamrial@gmail.com> wrote: > > > > Create a local one instead from a byte buffer input argument. > > > > Signed-off-by: James Almer <jamrial@gmail.com> > > The actual change mentioned in the commit message is LGTM, and the > updated sanity checks seem good (since now the alternatives at that > point are either 3+3 (2094-40) or 3+4 (closed captions)), but they are > pretty separate from the actual change of this commit. So in that > sense it might make sense to just separate them into their own fixup > commit when pushing. Or at least mention the sanity check fixups in > the commit message instead of them being a silent good change. For the record, I only now grasped after it being mentioned that what I took for just sanity check fixups are actually needed for the gdc changes since otherwise the size variable's value is out-of-sync. So yea, overall LGTM as long as that part of the change set is mentioned, since otherwise it just looked like nice (but unrelated) sanity check fixups. Jan
On 12/6/2020 8:34 PM, Jan Ekström wrote: > On Mon, Dec 7, 2020 at 1:23 AM Jan Ekström <jeebjp@gmail.com> wrote: >> >> On Sun, Dec 6, 2020 at 5:44 PM James Almer <jamrial@gmail.com> wrote: >>> >>> Create a local one instead from a byte buffer input argument. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >> >> The actual change mentioned in the commit message is LGTM, and the >> updated sanity checks seem good (since now the alternatives at that >> point are either 3+3 (2094-40) or 3+4 (closed captions)), but they are >> pretty separate from the actual change of this commit. So in that >> sense it might make sense to just separate them into their own fixup >> commit when pushing. Or at least mention the sanity check fixups in >> the commit message instead of them being a silent good change. > > For the record, I only now grasped after it being mentioned that what > I took for just sanity check fixups are actually needed for the gdc > changes since otherwise the size variable's value is out-of-sync. > > So yea, overall LGTM as long as that part of the change set is > mentioned, since otherwise it just looked like nice (but unrelated) > sanity check fixups. > > Jan Split it in two since it's cleaner that way, and applied it. Thanks.
diff --git a/libavcodec/dynamic_hdr10_plus.c b/libavcodec/dynamic_hdr10_plus.c index 5684bfb2ef..432ef8f6a1 100644 --- a/libavcodec/dynamic_hdr10_plus.c +++ b/libavcodec/dynamic_hdr10_plus.c @@ -17,6 +17,7 @@ */ #include "dynamic_hdr10_plus.h" +#include "get_bits.h" static const uint8_t usa_country_code = 0xB5; static const uint16_t smpte_provider_code = 0x003C; @@ -30,11 +31,19 @@ static const int32_t knee_point_den = 4095; static const int32_t bezier_anchor_den = 1023; static const int32_t saturation_weight_den = 8; -int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(GetBitContext *gb, AVDynamicHDRPlus *s) +int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus *s, const uint8_t *data, + int size) { + GetBitContext gbc, *gb = &gbc; + int ret; + if (!s) return AVERROR(ENOMEM); + ret = init_get_bits8(gb, data, size); + if (ret < 0) + return ret; + s->application_version = get_bits(gb, 8); if (get_bits_left(gb) < 2) @@ -189,7 +198,5 @@ int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(GetBitContext *gb, AVDynamicHDRPlus } } - skip_bits(gb, get_bits_left(gb)); - return 0; } diff --git a/libavcodec/dynamic_hdr10_plus.h b/libavcodec/dynamic_hdr10_plus.h index 06053f88e7..cd7acf0432 100644 --- a/libavcodec/dynamic_hdr10_plus.h +++ b/libavcodec/dynamic_hdr10_plus.h @@ -20,15 +20,16 @@ #define AVCODEC_DYNAMIC_HDR10_PLUS_H #include "libavutil/hdr_dynamic_metadata.h" -#include "get_bits.h" /** * Parse the user data registered ITU-T T.35 to AVbuffer (AVDynamicHDRPlus). - * @param gb The bit content to be decoded. * @param s A pointer containing the decoded AVDynamicHDRPlus structure. + * @param data The byte array containing the raw ITU-T T.35 data. + * @param size Size of the data array in bytes. * * @return 0 if succeed. Otherwise, returns the appropriate AVERROR. */ -int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(GetBitContext *gb, AVDynamicHDRPlus *s); +int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus *s, const uint8_t *data, + int size); #endif /* AVCODEC_DYNAMIC_HDR10_PLUS_H */ diff --git a/libavcodec/hevc_sei.c b/libavcodec/hevc_sei.c index 8af9f9b29d..3b0fa43439 100644 --- a/libavcodec/hevc_sei.c +++ b/libavcodec/hevc_sei.c @@ -216,7 +216,8 @@ static int decode_registered_user_data_dynamic_hdr_plus(HEVCSEIDynamicHDRPlus *s if (!metadata) return AVERROR(ENOMEM); - err = ff_parse_itu_t_t35_to_dynamic_hdr10_plus(gb, metadata); + err = ff_parse_itu_t_t35_to_dynamic_hdr10_plus(metadata, + gb->buffer + get_bits_count(gb) / 8, size); if (err < 0) { av_free(metadata); return err; @@ -229,6 +230,8 @@ static int decode_registered_user_data_dynamic_hdr_plus(HEVCSEIDynamicHDRPlus *s return AVERROR(ENOMEM); } + skip_bits_long(gb, size * 8); + return 0; } @@ -241,9 +244,9 @@ static int decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s, GetBitConte uint8_t country_code = 0; uint16_t provider_code = 0; - if (size < 7) + if (size < 3) return AVERROR(EINVAL); - size -= 7; + size -= 3; country_code = get_bits(gb, 8); if (country_code == 0xFF) { @@ -258,16 +261,27 @@ static int decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s, GetBitConte // A/341 Amendment - 2094-40 const uint16_t smpte2094_40_provider_oriented_code = 0x0001; const uint8_t smpte2094_40_application_identifier = 0x04; + uint16_t provider_oriented_code; + uint8_t application_identifier; - uint16_t provider_oriented_code = get_bits(gb, 16); - uint8_t application_identifier = get_bits(gb, 8); + if (size < 3) + return AVERROR(EINVAL); + size -= 3; + provider_oriented_code = get_bits(gb, 16); + application_identifier = get_bits(gb, 8); if (provider_oriented_code == smpte2094_40_provider_oriented_code && application_identifier == smpte2094_40_application_identifier) { return decode_registered_user_data_dynamic_hdr_plus(&s->dynamic_hdr_plus, gb, size); } } else { - uint32_t user_identifier = get_bits_long(gb, 32); + uint32_t user_identifier; + + if (size < 4) + return AVERROR(EINVAL); + size -= 4; + + user_identifier = get_bits_long(gb, 32); switch (user_identifier) { case MKBETAG('G', 'A', '9', '4'): return decode_registered_user_data_closed_caption(&s->a53_caption, gb, size);
Create a local one instead from a byte buffer input argument. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/dynamic_hdr10_plus.c | 13 ++++++++++--- libavcodec/dynamic_hdr10_plus.h | 7 ++++--- libavcodec/hevc_sei.c | 26 ++++++++++++++++++++------ 3 files changed, 34 insertions(+), 12 deletions(-)