Thank you Ian for your great detailed comments. Thanks, Mohammad
On Wed, Nov 11, 2020 at 5:10 PM Jan Ekström <jee...@gmail.com> wrote: > On 14.10.2020 2:53, Mohammad Izadi wrote: > > From: Mohammad Izadi <moh.iz...@gmail.com> > > > > HDR10+ is dynamic metadata (A/341 Amendment - SMPTE2094-40) that needs > to be decoded from ITU-T T.35 in HEVC bitstream. The HDR10+ is transferred > to side data packet to be used or passed through. > > > > The fate test file can be found here: > https://drive.google.com/file/d/1Hadzc24-RsgnRqS-B0bxLmzDeTwEOhtE/view?usp=sharing > > > > The video file needs to be copied to fate-suite/mov/ > > --- > > Long overdue initial attempt at a review follows... > > > fftools/ffprobe.c | 16 +++ > > libavcodec/avpacket.c | 1 + > > libavcodec/decode.c | 1 + > > libavcodec/hevc_sei.c | 39 ++++-- > > libavcodec/hevc_sei.h | 5 + > > libavcodec/hevcdec.c | 16 +++ > > libavcodec/internal.h | 9 ++ > > libavcodec/packet.h | 8 ++ > > libavcodec/utils.c | 180 +++++++++++++++++++++++++ > > libavcodec/version.h | 2 +- > > libavfilter/vf_showinfo.c | 28 ++++ > > tests/fate/mov.mak | 3 + > > tests/ref/fate/mov-hdr10-plus-metadata | 55 ++++++++ > > 13 files changed, 350 insertions(+), 13 deletions(-) > > create mode 100644 tests/ref/fate/mov-hdr10-plus-metadata > > > > diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c > > index d4e494f11f..0b80edf842 100644 > > --- a/fftools/ffprobe.c > > +++ b/fftools/ffprobe.c > > @@ -35,6 +35,7 @@ > > #include "libavutil/bprint.h" > > #include "libavutil/display.h" > > #include "libavutil/hash.h" > > +#include "libavutil/hdr_dynamic_metadata.h" > > #include "libavutil/mastering_display_metadata.h" > > #include "libavutil/dovi_meta.h" > > #include "libavutil/opt.h" > > @@ -1925,6 +1926,13 @@ static void print_pkt_side_data(WriterContext *w, > > print_q("min_luminance", metadata->min_luminance, '/'); > > print_q("max_luminance", metadata->max_luminance, '/'); > > } > > + } else if (sd->type == AV_PKT_DATA_DYNAMIC_HDR_PLUS) { > > + AVDynamicHDRPlus *metadata = (AVDynamicHDRPlus *)sd->data; > > + // Partially print HDR10+ metadata. > > + print_int("num_windows", metadata->num_windows); > > + print_q("targeted_system_display_maximum_luminance", > metadata->targeted_system_display_maximum_luminance, '/'); > > + > print_int("targeted_system_display_actual_peak_luminance_flag", > metadata->targeted_system_display_actual_peak_luminance_flag); > > + print_int("mastering_display_actual_peak_luminance_flag", > metadata->mastering_display_actual_peak_luminance_flag); > > } else if (sd->type == AV_PKT_DATA_CONTENT_LIGHT_LEVEL) { > > AVContentLightMetadata *metadata = (AVContentLightMetadata > *)sd->data; > > print_int("max_content", metadata->MaxCLL); > > @@ -2250,6 +2258,14 @@ static void show_frame(WriterContext *w, AVFrame > *frame, AVStream *stream, > > print_q("min_luminance", metadata->min_luminance, > '/'); > > print_q("max_luminance", metadata->max_luminance, > '/'); > > } > > + } else if (sd->type == AV_FRAME_DATA_DYNAMIC_HDR_PLUS) { > > + AVDynamicHDRPlus *metadata = (AVDynamicHDRPlus > *)sd->data; > > + // Partially print HDR10+ metadata. > > + print_int("num_windows", metadata->num_windows); > > + print_q("targeted_system_display_maximum_luminance", > metadata->targeted_system_display_maximum_luminance, '/'); > > + > print_int("targeted_system_display_actual_peak_luminance_flag", > metadata->targeted_system_display_actual_peak_luminance_flag); > > + print_int("mastering_display_actual_peak_luminance_flag", > metadata->mastering_display_actual_peak_luminance_flag); > > + > > Some really minor stuff: > - Empty line at the end > - Tabs vs space, seemingly some tabs ended up here. Copy and paste can > make that happen. I know this has been done in this duplicated fashion so > far, but maybe this should just be in a static function that takes in a > AVDynamicHDRPlus pointer and could just be called from two places? > fixed. > - I am not sure of the whole history of this patch set, but was there any > explanation on why these specific values should be the ones from which > printing should be started since we are not printing the whole > (complicated) structure? I think so far our practice has been to dump all > values from the structures, except for the flags for whether a thing exists > or not (see the example with "has_primaries"/"has_luminance" for mastering > display metadata, and the loop within S12M timecode printing for lists of > things), so even if we limit initial printing to certain values, printing > out flags instead of actual possible values is an interesting selection. > Dumped the whole structure. > > > } else if (sd->type == AV_FRAME_DATA_CONTENT_LIGHT_LEVEL) { > > AVContentLightMetadata *metadata = > (AVContentLightMetadata *)sd->data; > > print_int("max_content", metadata->MaxCLL); > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > > index dce26cb31a..8307032335 <(830)%20703-2335> 100644 > > --- a/libavcodec/avpacket.c > > +++ b/libavcodec/avpacket.c > > @@ -394,6 +394,7 @@ const char *av_packet_side_data_name(enum > AVPacketSideDataType type) > > case AV_PKT_DATA_CONTENT_LIGHT_LEVEL: return "Content light > level metadata"; > > case AV_PKT_DATA_SPHERICAL: return "Spherical > Mapping"; > > case AV_PKT_DATA_A53_CC: return "A53 Closed > Captions"; > > + case AV_PKT_DATA_DYNAMIC_HDR_PLUS: return "HDR10+ Dynamic > Metadata (SMPTE 2094-40)"; > > case AV_PKT_DATA_ENCRYPTION_INIT_INFO: return "Encryption > initialization data"; > > case AV_PKT_DATA_ENCRYPTION_INFO: return "Encryption > info"; > > case AV_PKT_DATA_AFD: return "Active Format > Description data"; diff --git a/libavcodec/decode.c b/libavcodec/decode.c > index de9c079f9d..cd3286f7fb 100644 --- a/libavcodec/decode.c +++ > b/libavcodec/decode.c @@ -1698,6 +1698,7 @@ int > ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame) { > AV_PKT_DATA_MASTERING_DISPLAY_METADATA, > AV_FRAME_DATA_MASTERING_DISPLAY_METADATA }, { > AV_PKT_DATA_CONTENT_LIGHT_LEVEL, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL }, { > AV_PKT_DATA_A53_CC, AV_FRAME_DATA_A53_CC }, + { > AV_PKT_DATA_DYNAMIC_HDR_PLUS, AV_FRAME_DATA_DYNAMIC_HDR_PLUS }, { > AV_PKT_DATA_ICC_PROFILE, AV_FRAME_DATA_ICC_PROFILE }, }; diff --git > a/libavcodec/hevc_sei.c b/libavcodec/hevc_sei.c index > a4ec65dc1a..f2fe47f7f3 100644 --- a/libavcodec/hevc_sei.c +++ > b/libavcodec/hevc_sei.c @@ -25,6 +25,7 @@ #include "golomb.h" > > #include "hevc_ps.h" > > #include "hevc_sei.h" > > +#include "internal.h" > > > > static int decode_nal_sei_decoded_picture_hash(HEVCSEIPictureHash *s, > GetBitContext *gb) > > { > > @@ -242,8 +243,8 @@ static int > decode_nal_sei_user_data_unregistered(HEVCSEIUnregistered *s, GetBitC > > static int decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s, > GetBitContext *gb, > > int size) > > { > > - uint32_t country_code; > > - uint32_t user_identifier; > > + uint8_t country_code; > > + uint16_t provider_code; > > > > if (size < 7) > > return AVERROR(EINVAL); > > @@ -255,18 +256,31 @@ static int > decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s, GetBitConte > > size--; > > } > > > > - skip_bits(gb, 8); > > - skip_bits(gb, 8); > > - > > - user_identifier = get_bits_long(gb, 32); > > - > > - switch (user_identifier) { > > - case MKBETAG('G', 'A', '9', '4'): > > + provider_code = get_bits(gb, 16); > > + > > + const uint8_t usa_country_code = 0xB5; > > + const uint16_t smpte_provider_code = 0x003C; > > + if (country_code == usa_country_code && > > + provider_code == smpte_provider_code) { > > + // A/341 Amendment – 2094-40 > > + uint16_t provider_oriented_code = get_bits(gb, 16); > > + uint8_t application_identifier = get_bits(gb, 8); > > + const uint16_t smpte2094_40_provider_oriented_code = 0x0001; > > + const uint16_t smpte2094_40_application_identifier = 0x04; > > I do not know what the general rule about const integers as an alternative > for #defines is in FFmpeg, > but at the very least I applied the following diff during review, which > did the following things: > > - Alignment of things. - Move the static defines to be first, separate them a bit from the actual > parsed values. > - application_identifier seems to be uint8_t so made > smpte2094_40_application_identifier such as well. > - I prefer always specifically initializing values to something known, > although since the values are always written to first by get_bits > this is relatively minor and in some cases might be considered an > opinion thing. > All fixed. > > --- a/libavcodec/hevc_sei.c > +++ b/libavcodec/hevc_sei.c > @@ -210,8 +210,11 @@ static int > decode_nal_sei_user_data_unregistered(HEVCSEIUnregistered *s, GetBitC > static int decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s, > GetBitContext *gb, > int size) > { > - uint8_t country_code; > - uint16_t provider_code; > + const uint8_t usa_country_code = 0xB5; > + const uint16_t smpte_provider_code = 0x003C; > + > + uint8_t country_code = 0; > + uint16_t provider_code = 0; > > if (size < 7) > return AVERROR(EINVAL); > @@ -225,15 +228,15 @@ static int > decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s, GetBitConte > > provider_code = get_bits(gb, 16); > > - const uint8_t usa_country_code = 0xB5; > - const uint16_t smpte_provider_code = 0x003C; > if (country_code == usa_country_code && > provider_code == smpte_provider_code) { > // A/341 Amendment – 2094-40 > - uint16_t provider_oriented_code = get_bits(gb, 16); > - uint8_t application_identifier = get_bits(gb, 8); > const uint16_t smpte2094_40_provider_oriented_code = 0x0001; > - const uint16_t smpte2094_40_application_identifier = 0x04; > + const uint8_t smpte2094_40_application_identifier = 0x04; > + > + uint16_t provider_oriented_code = get_bits(gb, 16); > + uint8_t application_identifier = get_bits(gb, 8); > + > if (provider_oriented_code == smpte2094_40_provider_oriented_code > && > application_identifier == > smpte2094_40_application_identifier) { > int err = ff_read_itu_t_t35_to_dynamic_hdr_plus(gb, &s-> > dynamic_hdr_plus.info); > @@ -243,7 +246,7 @@ static int > decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s, GetBitConte > return err; > } > } else { > - uint32_t user_identifier = get_bits_long(gb, 32); > + uint32_t user_identifier = get_bits_long(gb, 32); > if(user_identifier == MKBETAG('G', 'A', '9', '4')) > return > decode_registered_user_data_closed_caption(&s->a53_caption, gb, size); > } > > > + if (provider_oriented_code == > smpte2094_40_provider_oriented_code && > > + application_identifier == > smpte2094_40_application_identifier) { > > + int err = ff_read_itu_t_t35_to_dynamic_hdr_plus(gb, &s-> > dynamic_hdr_plus.info); > > + if (err < 0 && s->dynamic_hdr_plus.info) { > > + av_buffer_unref(&s->dynamic_hdr_plus.info); > > + } > > Generally such functions (such as ff_parse_a53_cc) are defined such as > that in case of error, the AVBufferRef pointer is left untouched. > > That is done for exactly the reason that in case of failure, you do not > want to be doing such clean-up on the calling side. > You're right. Fixed. > > > + return err; > > + } > > + } else { > > + uint32_t user_identifier = get_bits_long(gb, 32); > > + if(user_identifier == MKBETAG('G', 'A', '9', '4')) > > return > decode_registered_user_data_closed_caption(&s->a53_caption, gb, size); > > - default: > > - skip_bits_long(gb, size * 8); > > - break; > > } > > + skip_bits_long(gb, size * 8); > > return 0; > > } > > > > @@ -453,4 +467,5 @@ void ff_hevc_reset_sei(HEVCSEI *s) > > av_buffer_unref(&s->unregistered.buf_ref[i]); > > s->unregistered.nb_buf_ref = 0; > > av_freep(&s->unregistered.buf_ref); > > + av_buffer_unref(&s->dynamic_hdr_plus.info); > > } > > diff --git a/libavcodec/hevc_sei.h b/libavcodec/hevc_sei.h > > index 5ee7a4796d..e9e2d46ed4 100644 > > --- a/libavcodec/hevc_sei.h > > +++ b/libavcodec/hevc_sei.h > > @@ -104,6 +104,10 @@ typedef struct HEVCSEIMasteringDisplay { > > uint32_t min_luminance; > > } HEVCSEIMasteringDisplay; > > > > +typedef struct HEVCSEIDynamicHDRPlus { > > + AVBufferRef *info; > > +} HEVCSEIDynamicHDRPlus; > > + > > typedef struct HEVCSEIContentLight { > > int present; > > uint16_t max_content_light_level; > > @@ -143,6 +147,7 @@ typedef struct HEVCSEI { > > HEVCSEIA53Caption a53_caption; > > HEVCSEIUnregistered unregistered; > > HEVCSEIMasteringDisplay mastering_display; > > + HEVCSEIDynamicHDRPlus dynamic_hdr_plus; > > HEVCSEIContentLight content_light; > > int active_seq_parameter_set_id; > > HEVCSEIAlternativeTransfer alternative_transfer; > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > index b77df8d89f..cd794231e8 100644 > > --- a/libavcodec/hevcdec.c > > +++ b/libavcodec/hevcdec.c > > @@ -2849,6 +2849,16 @@ static int set_side_data(HEVCContext *s) > > > > s->sei.timecode.num_clock_ts = 0; > > } > > + if (s->sei.dynamic_hdr_plus.info){ > > + AVBufferRef *info_ref = av_buffer_ref(s-> > sei.dynamic_hdr_plus.info); > > + if (!info_ref) > > + return AVERROR(ENOMEM); > > + > > + if(!av_frame_new_side_data_from_buf(out, > AV_FRAME_DATA_DYNAMIC_HDR_PLUS, info_ref)){ > > + av_buffer_unref(&info_ref); > > + return AVERROR(ENOMEM); > > + } > > + } > > Seems like more mixed tabs and spaces, also 2-space indent instead of 4? > Fixed. > > Also while I do see what is being done with the AVBufferRefs, this is > still fascinating that none of the AVBufferRef utilizing stuff that does > av_frame_new_side_data_from_buf utilizes av_buffer_ref in this fashion in > this function. :) > > Also for this and the update_thread_context part, just add an empty line > before starting the HDR10+ stuff to ease parsing where that bit starts. > Fixed. > > > > > return 0; > > } > > @@ -3530,6 +3540,12 @@ static int > hevc_update_thread_context(AVCodecContext *dst, > > if (!s->sei.a53_caption.buf_ref) > > return AVERROR(ENOMEM); > > } > > + av_buffer_unref(&s->sei.dynamic_hdr_plus.info); > > + if (s0->sei.dynamic_hdr_plus.info) { > > + s->sei.dynamic_hdr_plus.info = av_buffer_ref(s0-> > sei.dynamic_hdr_plus.info); > > + if (!s->sei.dynamic_hdr_plus.info) > > + return AVERROR(ENOMEM); > > + } > > > > s->sei.frame_packing = s0->sei.frame_packing; > > s->sei.display_orientation = s0->sei.display_orientation; > > diff --git a/libavcodec/internal.h b/libavcodec/internal.h > > index 0a1c0a17ec..7ebbc5e4f9 100644 > > --- a/libavcodec/internal.h > > +++ b/libavcodec/internal.h > > @@ -413,6 +413,15 @@ int ff_int_from_list_or_default(void *ctx, const > char * val_name, int val, > > > > void ff_dvdsub_parse_palette(uint32_t *palette, const char *p); > > > > +/** > > + * Reads and decode the user data registered ITU-T T.35 to AVbuffer > (AVDynamicHDRPlus). > > + * @param gbc The bit content to be decoded. > > + * @param output A buffer containing the decoded AVDynamicHDRPlus > structure. > > + * > > + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR. > > + */ > > +int ff_read_itu_t_t35_to_dynamic_hdr_plus(void *gbc, AVBufferRef > **output); > > + > > Taking a look at atsc_a53.{c,h} , is there any reason why this stuff is in > the primary internal.h instead of being in a specific helper file? Also, > why is the GBC being passed on as a void pointer? > > Also for the doxy, I think examples for a bit better wording could be > looked at from the atsc_a53.h header (also as I noted before, I would > prefer such functions to not touch the AVBufferRef * until a structure was > parsed). > It seems James recently moved functions to atsc_a53.{c,h} .Very good point. Synced and created a similar file. > > > #if defined(_WIN32) && CONFIG_SHARED && !defined(BUILDING_avcodec) > > # define av_export_avcodec __declspec(dllimport) > > #else > > diff --git a/libavcodec/packet.h b/libavcodec/packet.h > > index 96f237f091..60fbf2c1e8 100644 > > --- a/libavcodec/packet.h > > +++ b/libavcodec/packet.h > > @@ -282,6 +282,14 @@ enum AVPacketSideDataType { > > */ > > AV_PKT_DATA_DOVI_CONF, > > > > + /** > > + * HDR10+ dynamic metadata associated with a video frame. The > metadata is in > > + * the form of the AVDynamicHDRPlus struct and contains > > + * information for color volume transform - application 4 of > > + * SPMTE 2094-40:2016 standard. > > + */ > > + AV_PKT_DATA_DYNAMIC_HDR_PLUS, > > + > > /** > > * The number of side data types. > > * This is not part of the public API/ABI in the sense that it may > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > > index 2ece34f921..c9435da3b4 100644 > > --- a/libavcodec/utils.c > > +++ b/libavcodec/utils.c > > @@ -42,8 +42,10 @@ > > #include "libavutil/samplefmt.h" > > #include "libavutil/dict.h" > > #include "libavutil/thread.h" > > +#include "libavutil/hdr_dynamic_metadata.h" > > #include "avcodec.h" > > #include "decode.h" > > +#include "get_bits.h" > > #include "hwconfig.h" > > #include "libavutil/opt.h" > > #include "mpegvideo.h" > > @@ -67,6 +69,17 @@ > > const char av_codec_ffversion[] = "FFmpeg version " FFMPEG_VERSION; > > > > static AVMutex codec_mutex = AV_MUTEX_INITIALIZER; > > +static const uint8_t usa_country_code = 0xB5; > > +static const uint16_t smpte_provider_code = 0x003C; > > +static const uint16_t smpte2094_40_provider_oriented_code = 0x0001; > > +static const uint16_t smpte2094_40_application_identifier = 0x04; > > +static const int64_t luminance_den = 1; > > +static const int32_t peak_luminance_den = 15; > > +static const int64_t rgb_den = 100000; > > +static const int32_t fraction_pixel_den = 1000; > > +static const int32_t knee_point_den = 4095; > > +static const int32_t bezier_anchor_den = 1023; > > +static const int32_t saturation_weight_den = 8; > > > > void av_fast_padded_malloc(void *ptr, unsigned int *size, size_t > min_size) > > { > > @@ -2346,3 +2359,170 @@ int ff_int_from_list_or_default(void *ctx, const > char * val_name, int val, > > "%s %d are not supported. Set to default value : %d\n", > val_name, val, default_value); > > return default_value; > > } > > + > > +int ff_read_itu_t_t35_to_dynamic_hdr_plus(void *gbc, AVBufferRef > **output) > > +{ > > + GetBitContext *gb = (GetBitContext *)gbc; > > + uint8_t application_version = get_bits(gbc, 8); > > + size_t size; > > + int w, i, j; > > + AVDynamicHDRPlus *s = av_dynamic_hdr_plus_alloc(&size); > > + if (!s) > > + return AVERROR(ENOMEM); > > + > > + if (*output) > > + av_buffer_unref(output); > > + > > + *output = av_buffer_create( > > + (uint8_t *)s, size, av_buffer_default_free, NULL, 0); > > + if (!(*output)) { > > + av_freep(&s); > > + return AVERROR(ENOMEM); > > + } > > Poking the output pointer should probably be left at the very end, where > the structure "s" is known to be in a good state. That way if the parsing > fails for one reason or another, the given pointer is not touched. > Fixed. > > > + s->application_version = application_version; > > I think application version can be checked against the value 0 as per the > spec? > That's right, as per the spec. Unfortunately, the HDR10 generated by Samsung S10 set application_version to 1. So we cannot check it against 0 and have to pass it through. > > > + if (get_bits_left(gb) < 2) > > + return AVERROR_INVALIDDATA; > > + s->num_windows = get_bits(gb, 2); > > + > > + if (s->num_windows < 1 || s->num_windows > 3) { > > + return AVERROR_INVALIDDATA; > > + } > > + > > + if (get_bits_left(gb) < ((19 * 8 + 1) * (s->num_windows - 1))) > > + return AVERROR_INVALIDDATA; > > You can utilize empty lines between the get_bits_left checks and the > following parsing to split the context better. > Done. > > > + for (w = 1; w < s->num_windows; w++) { > > Thankfully, you can utilize variables initialized within loops, so you do > not have to pre-define a variable that is only utilized within a loop. Most > likely w, i and j here in this function are such :) . > Done. > > > + s->params[w].window_upper_left_corner_x.num = get_bits(gb, 16); > > + s->params[w].window_upper_left_corner_y.num = get_bits(gb, 16); > > + s->params[w].window_lower_right_corner_x.num = get_bits(gb, 16); > > + s->params[w].window_lower_right_corner_y.num = get_bits(gb, 16); > > + // The corners are set to absolute coordinates here. They > should be > > + // converted to the relative coordinates (in [0, 1]) in the > decoder. > > + s->params[w].window_upper_left_corner_x.den = 1; > > + s->params[w].window_upper_left_corner_y.den = 1; > > + s->params[w].window_lower_right_corner_x.den = 1; > > + s->params[w].window_lower_right_corner_y.den = 1; > > + > > These seem to be just X/Y co-ordinates, I'm probably missing something > here but why are these AVRationals? Of course the struct is already defined > and been available since 2018 so me commenting on this is kind of late, I > guess? :D > Good point, but not sure why. I think integer was sufficient. > > I think such structs are usually set in a single set with something like > "(AVRational){get_bits(gb, 16), 1};", that way you do not have to > separately set the numerator and the denumerator. > Thanks for the nice suggestion. Done. > > > + s->params[w].center_of_ellipse_x = get_bits(gb, 16); > > + s->params[w].center_of_ellipse_y = get_bits(gb, 16); > > + s->params[w].rotation_angle = get_bits(gb, 8); > > + s->params[w].semimajor_axis_internal_ellipse = get_bits(gb, 16); > > + s->params[w].semimajor_axis_external_ellipse = get_bits(gb, 16); > > + s->params[w].semiminor_axis_external_ellipse = get_bits(gb, 16); > > + s->params[w].overlap_process_option = get_bits1(gb); > > + } > > + > > + if (get_bits_left(gb) < 28) > > + return AVERROR(EINVAL); > > + s->targeted_system_display_maximum_luminance.num = get_bits(gb, 27); > > + s->targeted_system_display_maximum_luminance.den = luminance_den; > > Ditto regarding AVRational initialization, "(AVRational){get_bits(gb, 27), > luminance_den};" . > Done. > > Although once again, I must wonder how useful a rational is for this if > the denumerator is specified to be 1... > > > + s->targeted_system_display_actual_peak_luminance_flag = > get_bits1(gb); > > + > > + if (s->targeted_system_display_actual_peak_luminance_flag) { > > + int rows, cols; > > + if (get_bits_left(gb) < 10) > > + return AVERROR(EINVAL); > > + rows = get_bits(gb, 5); > > + cols = get_bits(gb, 5); > > + if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25))) > { > > + return AVERROR_INVALIDDATA; > > + } > > + s->num_rows_targeted_system_display_actual_peak_luminance = > rows; > > + s->num_cols_targeted_system_display_actual_peak_luminance = > cols; > > + > > + if (get_bits_left(gb) < (rows * cols * 4)) > > + return AVERROR(EINVAL); > > + > > + for (i = 0; i < rows; i++) { > > + for (j = 0; j < cols; j++) { > > + > s->targeted_system_display_actual_peak_luminance[i][j].num = get_bits(gb, > 4); > > + > s->targeted_system_display_actual_peak_luminance[i][j].den = > peak_luminance_den; > > Once again, regarding the AVRational initialization :) . Now at least the > denumerator is is not 1, yay :) (although it seems like the coded value is > a [0,1] float, just represented in values 0-15). > Hahaha :D > > > + } > > + } > > + } > > + for (w = 0; w < s->num_windows; w++) { > > + if (get_bits_left(gb) < (3 * 17 + 17 + 4)) > > + return AVERROR(EINVAL); > > + for (i = 0; i < 3; i++) { > > + s->params[w].maxscl[i].num = get_bits(gb, 17); > > + s->params[w].maxscl[i].den = rgb_den; > > + } > > + s->params[w].average_maxrgb.num = get_bits(gb, 17); > > + s->params[w].average_maxrgb.den = rgb_den; > > + s->params[w].num_distribution_maxrgb_percentiles = get_bits(gb, > 4); > > + > > + if (get_bits_left(gb) < > > + (s->params[w].num_distribution_maxrgb_percentiles * 24)) > > + return AVERROR(EINVAL); > > + for (i = 0; i < > s->params[w].num_distribution_maxrgb_percentiles; i++) { > > + s->params[w].distribution_maxrgb[i].percentage = > get_bits(gb, 7); > > + s->params[w].distribution_maxrgb[i].percentile.num = > get_bits(gb, 17); > > + s->params[w].distribution_maxrgb[i].percentile.den = > rgb_den; > > + } > > + > > + if (get_bits_left(gb) < 10) > > + return AVERROR(EINVAL); > > + s->params[w].fraction_bright_pixels.num = get_bits(gb, 10); > > + s->params[w].fraction_bright_pixels.den = fraction_pixel_den; > > + } > > Generally the same comment with regards to AVRationals here as well. > > Done. > Additionally, since you can define variables it might probably make sense > to add a AVHDRPlusColorTransformParams *transform_params = ¶ms[w] just > not to duplicate the indexing all the time? Same for the > AVHDRPlusPercentile list being under &s->params[w].distribution_maxrgb[i]. > Done. > > And sorry, at this point it's 02:30 local time and I need to still do some > work tomorrow. I will attempt to do another look at all of this soon. Thank you so much for your time. > > Jan > > > + if (get_bits_left(gb) < 1) > > + return AVERROR(EINVAL); > > + s->mastering_display_actual_peak_luminance_flag = get_bits1(gb); > > + if (s->mastering_display_actual_peak_luminance_flag) { > > + int rows, cols; > > + if (get_bits_left(gb) < 10) > > + return AVERROR(EINVAL); > > + rows = get_bits(gb, 5); > > + cols = get_bits(gb, 5); > > + if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25))) > { > > + return AVERROR_INVALIDDATA; > > + } > > + s->num_rows_mastering_display_actual_peak_luminance = rows; > > + s->num_cols_mastering_display_actual_peak_luminance = cols; > > + > > + if (get_bits_left(gb) < (rows * cols * 4)) > > + return AVERROR(EINVAL); > > + > > + for (i = 0; i < rows; i++) { > > + for (j = 0; j < cols; j++) { > > + s->mastering_display_actual_peak_luminance[i][j].num = > get_bits(gb, 4); > > + s->mastering_display_actual_peak_luminance[i][j].den = > peak_luminance_den; > > + } > > + } > > + } > > + > > + for (w = 0; w < s->num_windows; w++) { > > + if (get_bits_left(gb) < 1) > > + return AVERROR(EINVAL); > > + s->params[w].tone_mapping_flag = get_bits1(gb); > > + if (s->params[w].tone_mapping_flag) { > > + if (get_bits_left(gb) < 28) > > + return AVERROR(EINVAL); > > + s->params[w].knee_point_x.num = get_bits(gb, 12); > > + s->params[w].knee_point_x.den = knee_point_den; > > + s->params[w].knee_point_y.num = get_bits(gb, 12); > > + s->params[w].knee_point_y.den = knee_point_den; > > + s->params[w].num_bezier_curve_anchors = get_bits(gb, 4); > > + > > + if (get_bits_left(gb) < > (s->params[w].num_bezier_curve_anchors * 10)) > > + return AVERROR(EINVAL); > > + for (i = 0; i < s->params[w].num_bezier_curve_anchors; i++) > { > > + s->params[w].bezier_curve_anchors[i].num = get_bits(gb, > 10); > > + s->params[w].bezier_curve_anchors[i].den = > bezier_anchor_den; > > + } > > + } > > + > > + if (get_bits_left(gb) < 1) > > + return AVERROR(EINVAL); > > + s->params[w].color_saturation_mapping_flag = get_bits1(gb); > > + if (s->params[w].color_saturation_mapping_flag) { > > + if (get_bits_left(gb) < 6) > > + return AVERROR(EINVAL); > > + s->params[w].color_saturation_weight.num = get_bits(gb, 6); > > + s->params[w].color_saturation_weight.den = > saturation_weight_den; > > + } > > + } > > + > > + skip_bits(gb, get_bits_left(gb)); > > + > > + return 0; > > +} > > diff --git a/libavcodec/version.h b/libavcodec/version.h > > index e75891d463..ad0bfd619d 100644 > > --- a/libavcodec/version.h > > +++ b/libavcodec/version.h > > @@ -28,7 +28,7 @@ > > #include "libavutil/version.h" > > > > #define LIBAVCODEC_VERSION_MAJOR 58 > > -#define LIBAVCODEC_VERSION_MINOR 95 > > +#define LIBAVCODEC_VERSION_MINOR 96 > > #define LIBAVCODEC_VERSION_MICRO 100 > > > > #define LIBAVCODEC_VERSION_INT > AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ > > diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c > > index 1be939614d..f55df520ca 100644 > > --- a/libavfilter/vf_showinfo.c > > +++ b/libavfilter/vf_showinfo.c > > @@ -37,6 +37,7 @@ > > #include "libavutil/timecode.h" > > #include "libavutil/mastering_display_metadata.h" > > #include "libavutil/video_enc_params.h" > > +#include "libavutil/hdr_dynamic_metadata.h" > > > > #include "avfilter.h" > > #include "internal.h" > > @@ -178,6 +179,30 @@ static void dump_mastering_display(AVFilterContext > *ctx, AVFrameSideData *sd) > > av_q2d(mastering_display->min_luminance), > av_q2d(mastering_display->max_luminance)); > > } > > > > +static void dump_dynamic_hdr_plus(AVFilterContext *ctx, AVFrameSideData > *sd) > > +{ > > + AVDynamicHDRPlus *hdr_plus; > > + > > + av_log(ctx, AV_LOG_INFO, "HDR10+ metadata: "); > > + if (sd->size < sizeof(*hdr_plus)) { > > + av_log(ctx, AV_LOG_ERROR, "invalid data\n"); > > + return; > > + } > > + > > + hdr_plus = (AVDynamicHDRPlus *)sd->data; > > + av_log(ctx, AV_LOG_INFO, "num_windows: %d, ", > > + hdr_plus->num_windows); > > + av_log(ctx, AV_LOG_INFO, > > + "targeted_system_display_maximum_luminance: %8.4f, ", > > + av_q2d(hdr_plus->targeted_system_display_maximum_luminance)); > > + av_log(ctx, AV_LOG_INFO, > > + "targeted_system_display_actual_peak_luminance_flag: %d, ", > > + > hdr_plus->targeted_system_display_actual_peak_luminance_flag); > > + av_log(ctx, AV_LOG_INFO, > > + "mastering_display_actual_peak_luminance_flag: %d", > > + hdr_plus->mastering_display_actual_peak_luminance_flag); > > +} > > + > > static void dump_content_light_metadata(AVFilterContext *ctx, > AVFrameSideData *sd) > > { > > AVContentLightMetadata* metadata = > (AVContentLightMetadata*)sd->data; > > @@ -396,6 +421,9 @@ static int filter_frame(AVFilterLink *inlink, > AVFrame *frame) > > case AV_FRAME_DATA_MASTERING_DISPLAY_METADATA: > > dump_mastering_display(ctx, sd); > > break; > > + case AV_FRAME_DATA_DYNAMIC_HDR_PLUS: > > + dump_dynamic_hdr_plus(ctx, sd); > > + break; > > case AV_FRAME_DATA_CONTENT_LIGHT_LEVEL: > > dump_content_light_metadata(ctx, sd); > > break; > > diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak > > index 7a721d7c95..7cd62041b8 100644 > > --- a/tests/fate/mov.mak > > +++ b/tests/fate/mov.mak > > @@ -29,6 +29,7 @@ FATE_MOV_FFPROBE = fate-mov-neg-firstpts-discard \ > > fate-mov-guess-delay-2 \ > > fate-mov-guess-delay-3 \ > > fate-mov-mp4-with-mov-in24-ver \ > > + fate-mov-hdr10-plus-metadata \ > > > > FATE_MOV_FASTSTART = fate-mov-faststart-4gb-overflow \ > > > > @@ -124,3 +125,5 @@ fate-mov-faststart-4gb-overflow: CMP = oneline > > fate-mov-faststart-4gb-overflow: REF = bc875921f151871e787c4b4023269b29 > > > > fate-mov-mp4-with-mov-in24-ver: CMD = run ffprobe$(PROGSSUF)$(EXESUF) > -show_entries stream=codec_name -select_streams 1 > $(TARGET_SAMPLES)/mov/mp4-with-mov-in24-ver.mp4 > > + > > +fate-mov-hdr10-plus-metadata: CMD = run ffprobe$(PROGSSUF)$(EXESUF) > -show_frames -read_intervals 0%+0.01 -select_streams v -v 0 > $(TARGET_SAMPLES)/mov/hdr10_plus_h265_sample.mp4 > > \ No newline at end of file > > diff --git a/tests/ref/fate/mov-hdr10-plus-metadata > b/tests/ref/fate/mov-hdr10-plus-metadata > > new file mode 100644 > > index 0000000000..8444f77883 > > --- /dev/null > > +++ b/tests/ref/fate/mov-hdr10-plus-metadata > > @@ -0,0 +1,55 @@ > > +[FRAME] > > +media_type=video > > +stream_index=0 > > +key_frame=1 > > +pkt_pts=0 > > +pkt_pts_time=0.000000 > > +pkt_dts=0 > > +pkt_dts_time=0.000000 > > +best_effort_timestamp=0 > > +best_effort_timestamp_time=0.000000 > > +pkt_duration=4119 > > +pkt_duration_time=0.045767 > > +pkt_pos=33496 > > +pkt_size=340318 > > +width=3840 > > +height=2160 > > +pix_fmt=yuv420p10le > > +sample_aspect_ratio=1:1 > > +pict_type=I > > +coded_picture_number=0 > > +display_picture_number=0 > > +interlaced_frame=0 > > +top_field_first=0 > > +repeat_pict=0 > > +color_range=tv > > +color_space=bt2020nc > > +color_primaries=bt2020 > > +color_transfer=smpte2084 > > +chroma_location=left > > +[SIDE_DATA] > > +side_data_type=Mastering display metadata > > +red_x=34000/50000 > > +red_y=16000/50000 > > +green_x=13250/50000 > > +green_y=34500/50000 > > +blue_x=7500/50000 > > +blue_y=3000/50000 > > +white_point_x=15635/50000 > > +white_point_y=16450/50000 > > +min_luminance=50/10000 > > +max_luminance=10000000/10000 > > +[/SIDE_DATA] > > +[SIDE_DATA] > > +side_data_type=Content light level metadata > > +max_content=0 > > +max_average=0 > > +[/SIDE_DATA] > > +[SIDE_DATA] > > +side_data_type=HDR Dynamic Metadata SMPTE2094-40 (HDR10+) > > +num_windows=1 > > +targeted_system_display_maximum_luminance=400/1 > > +targeted_system_display_actual_peak_luminance_flag=0 > > +mastering_display_actual_peak_luminance_flag=0 > > +[/SIDE_DATA] > > +[/FRAME] > > \ No newline at end of file > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".