| Message ID | 20200207174628.115201-1-moh.izadi@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] Update HDR10+ metadata structure. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On 07/02/2020 17:46, Mohammad Izadi wrote: > From: Mohammad Izadi <izadi@google.com> > > Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side data in the follow-up CLs. > --- > libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ > libavutil/hdr_dynamic_metadata.h | 12 ++- > libavutil/version.h | 2 +- > 3 files changed, 177 insertions(+), 2 deletions(-) > > diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c > index 0fa1ee82de..a988bcd2d5 100644 > --- a/libavutil/hdr_dynamic_metadata.c > +++ b/libavutil/hdr_dynamic_metadata.c > @@ -19,8 +19,17 @@ > */ > > #include "hdr_dynamic_metadata.h" > +#include "libavcodec/get_bits.h" > #include "mem.h" > > +static const int64_t luminance_den = 1; The int64_t looks very shady - am I missing some special integer promotion behaviour here? (Note that AVRational num/den are int.) > +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; It would probably be clearer just to put these constants inline; there isn't really any use to having the values standalone. > + > AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size) > { > AVDynamicHDRPlus *hdr_plus = av_mallocz(sizeof(AVDynamicHDRPlus)); > @@ -45,3 +54,159 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame) > > return (AVDynamicHDRPlus *)side_data->data; > } > + > +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, > + AVDynamicHDRPlus *s) Why is this function being added to libavutil? It looks like it's meant for decoding UDR SEI messages only, so it should probably be in the relevant place in libavcodec. > +{ > + int w, i, j; > + GetBitContext gb; > + if (!data) > + return AVERROR_INVALIDDATA; > + > + int ret = init_get_bits8(&gb, data, size); > + if (ret < 0) > + return AVERROR_INVALIDDATA; > + > + 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; > + for (w = 1; w < s->num_windows; w++) { > + 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; > + > + 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); You're range-checking some fields here but not others. Should everything be verified against the constraints so that the comments on the structure in the header file are actually true? > + 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); I think these should consistently be INVALIDDATA (the data is incorrect, not the caller's use of the function). > + s->targeted_system_display_maximum_luminance.num = get_bits(&gb, 27); > + s->targeted_system_display_maximum_luminance.den = luminance_den; > + 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; > + } > + } > + } > + 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; > + } > + 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; > + } > + } > + It might be sensible to set the unused members of the structure to something fixed rather than leaving them uninitialised, so that attempting to copy the structure isn't UB. > + return 0; > +} > diff --git a/libavutil/hdr_dynamic_metadata.h b/libavutil/hdr_dynamic_metadata.h > index 2d72de56ae..28c657481f 100644 > --- a/libavutil/hdr_dynamic_metadata.h > +++ b/libavutil/hdr_dynamic_metadata.h > @@ -265,7 +265,7 @@ typedef struct AVDynamicHDRPlus { > > /** > * The nominal maximum display luminance of the targeted system display, > - * in units of 0.0001 candelas per square metre. The value shall be in > + * in units of 1 candelas per square metre. The value shall be in This was a error in the spec itself, I think? It should probably be isolated into its own commit with suitable references explaining what happened. > * the range of 0 to 10000, inclusive. > */ > AVRational targeted_system_display_maximum_luminance; > @@ -340,4 +340,14 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size); > */ > AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame); > > +/** > + * Decode SMPTE ST 2094-40 (the user data registered ITU-T T.35) to AVDynamicHDRPlus. > + * @param data The user data registered ITU-T T.35 (SMPTE ST 2094-40). I think you need to be a bit clearer which part of the data this wants. Is this the entirety of the user_data_registered_itu_t_t35() block, the itu_t_t35_payload_byte[] array, or something else? > + * @param size The size of the user data registered ITU-T T.35 (SMPTE ST 2094-40). > + * @param s The decoded AVDynamicHDRPlus structure. > + * > + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR. > + */ > +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, AVDynamicHDRPlus *s); > + > ... It would help to review if this were included in the decoder so that it can actually be tested. - Mark
On Sun, Feb 9, 2020 at 8:31 AM Mark Thompson <sw@jkqxz.net> wrote: > On 07/02/2020 17:46, Mohammad Izadi wrote: > > From: Mohammad Izadi <izadi@google.com> > > > > Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side > data in the follow-up CLs. > > --- > > libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ > > libavutil/hdr_dynamic_metadata.h | 12 ++- > > libavutil/version.h | 2 +- > > 3 files changed, 177 insertions(+), 2 deletions(-) > > > > diff --git a/libavutil/hdr_dynamic_metadata.c > b/libavutil/hdr_dynamic_metadata.c > > index 0fa1ee82de..a988bcd2d5 100644 > > --- a/libavutil/hdr_dynamic_metadata.c > > +++ b/libavutil/hdr_dynamic_metadata.c > > @@ -19,8 +19,17 @@ > > */ > > > > #include "hdr_dynamic_metadata.h" > > +#include "libavcodec/get_bits.h" > > #include "mem.h" > > > > +static const int64_t luminance_den = 1; > > The int64_t looks very shady - am I missing some special integer promotion > behaviour here? (Note that AVRational num/den are int.) > Done. > > > +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; > > It would probably be clearer just to put these constants inline; there > isn't really any use to having the values standalone. > You are right. Actually, I am going to push the encode function in my next CL and the static vars will be shared between both encode and decode function. > > > + > > AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size) > > { > > AVDynamicHDRPlus *hdr_plus = av_mallocz(sizeof(AVDynamicHDRPlus)); > > @@ -45,3 +54,159 @@ AVDynamicHDRPlus > *av_dynamic_hdr_plus_create_side_data(AVFrame *frame) > > > > return (AVDynamicHDRPlus *)side_data->data; > > } > > + > > +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, > > + AVDynamicHDRPlus *s) > > Why is this function being added to libavutil? It looks like it's meant > for decoding UDR SEI messages only, so it should probably be in the > relevant place in libavcodec. > I have used this function in my local code to decode HDR10+ of SEI message (libavcodec) and also HDR10+ in matroska container (ibavformat). I will push them in my next CLs. > > > +{ > > + int w, i, j; > > + GetBitContext gb; > > + if (!data) > > + return AVERROR_INVALIDDATA; > > + > > + int ret = init_get_bits8(&gb, data, size); > > + if (ret < 0) > > + return AVERROR_INVALIDDATA; > > + > > + 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; > > + for (w = 1; w < s->num_windows; w++) { > > + 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; > > + > > + 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); > > You're range-checking some fields here but not others. Should everything > be verified against the constraints so that the comments on the structure > in the header file are actually true? > The intention is to only pass through HDR10+ from src file to dst file. The interpretation of the data is on the user/caller. I think it is better not to drop the metadata by verification. I did some range checking like row or col in order to simply avoid accessing out of memory (avoid indices that are out of array size). > > > + 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); > > I think these should consistently be INVALIDDATA (the data is incorrect, > not the caller's use of the function). > Done. > > > + s->targeted_system_display_maximum_luminance.num = get_bits(&gb, > 27); > > + s->targeted_system_display_maximum_luminance.den = luminance_den; > > + 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; > > + } > > + } > > + } > > + 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; > > + } > > + 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; > > + } > > + } > > + > > It might be sensible to set the unused members of the structure to > something fixed rather than leaving them uninitialised, so that attempting > to copy the structure isn't UB. > If some members are not set by user and therefore they dont get a value in the decode function, they will never be accessed in the encode function or by user based on the HDR10+ logic. So it really doesn't matter to initialize them. Based on the logic, there is no way to interpret them. Please note that country_code is always unused and should be removed, but we cannot as it breaks API. It would great if you allow to remove it. I wrote the code and I am sure no one used the code yet. I am the only user and you can check it in github. > > > + return 0; > > +} > > diff --git a/libavutil/hdr_dynamic_metadata.h > b/libavutil/hdr_dynamic_metadata.h > > index 2d72de56ae..28c657481f 100644 > > --- a/libavutil/hdr_dynamic_metadata.h > > +++ b/libavutil/hdr_dynamic_metadata.h > > @@ -265,7 +265,7 @@ typedef struct AVDynamicHDRPlus { > > > > /** > > * The nominal maximum display luminance of the targeted system > display, > > - * in units of 0.0001 candelas per square metre. The value shall be > in > > + * in units of 1 candelas per square metre. The value shall be in > > This was a error in the spec itself, I think? It should probably be > isolated into its own commit with suitable references explaining what > happened. > I think it's wrong. It more makes sense now. The spec is updated in March 2019, https://www.atsc.org/wp-content/uploads/2018/02/A341S34-1-582r4-A341-Amendment-2094-40.pdf. I changed it based on the updated version. > > > * the range of 0 to 10000, inclusive. > > */ > > AVRational targeted_system_display_maximum_luminance; > > @@ -340,4 +340,14 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t > *size); > > */ > > AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame); > > > > +/** > > + * Decode SMPTE ST 2094-40 (the user data registered ITU-T T.35) to > AVDynamicHDRPlus. > > + * @param data The user data registered ITU-T T.35 (SMPTE ST 2094-40). > > I think you need to be a bit clearer which part of the data this wants. > Is this the entirety of the user_data_registered_itu_t_t35() block, the > itu_t_t35_payload_byte[] array, or something else? > Corrected the comment. It is the payload. > > > + * @param size The size of the user data registered ITU-T T.35 (SMPTE > ST 2094-40). > > + * @param s The decoded AVDynamicHDRPlus structure. > > + * > > + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR. > > + */ > > +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, > AVDynamicHDRPlus *s); > > + > > ... > > It would help to review if this were included in the decoder so that it > can actually be tested. > Can you please explain a bit more? :) > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 10/02/2020 19:38, Mohammad Izadi wrote: > On Sun, Feb 9, 2020 at 8:31 AM Mark Thompson <sw@jkqxz.net> wrote: > >> On 07/02/2020 17:46, Mohammad Izadi wrote: >>> From: Mohammad Izadi <izadi@google.com> >>> >>> Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side >> data in the follow-up CLs. >>> --- >>> libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ >>> libavutil/hdr_dynamic_metadata.h | 12 ++- >>> libavutil/version.h | 2 +- >>> 3 files changed, 177 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavutil/hdr_dynamic_metadata.c >> b/libavutil/hdr_dynamic_metadata.c >>> index 0fa1ee82de..a988bcd2d5 100644 >>> --- a/libavutil/hdr_dynamic_metadata.c >>> +++ b/libavutil/hdr_dynamic_metadata.c >>> ... >>> +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; >> >> It would probably be clearer just to put these constants inline; there >> isn't really any use to having the values standalone. >> > You are right. Actually, I am going to push the encode function in my next > CL and the static vars will be shared between both encode and decode > function. My point is that separating the constants is actively unhelpful in matching the standard to the code. From the standard you can read in one paragraph: "knee_point_x[ w ] – specifies the x coordinate of the separation point between the linear part and thecurved part of the tone mapping function. The value of knee_point_x[ w ] shall be in the range of 0 to 4,095, inclusive, where 0 maps to 0 cd/m2 and the full range of 4,095 maps to the maximum of the scene maximum luminance and the target peak luminance in cd/m2." But you've split that into two distinct locations by putting knee_point_den as a constant here and then the code below which refers to it when it would be clearer to just put the constant in the code in the same way that the standard does. >>> + >>> AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size) >>> { >>> AVDynamicHDRPlus *hdr_plus = av_mallocz(sizeof(AVDynamicHDRPlus)); >>> @@ -45,3 +54,159 @@ AVDynamicHDRPlus >> *av_dynamic_hdr_plus_create_side_data(AVFrame *frame) >>> >>> return (AVDynamicHDRPlus *)side_data->data; >>> } >>> + >>> +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, >>> + AVDynamicHDRPlus *s) >> >> Why is this function being added to libavutil? It looks like it's meant >> for decoding UDR SEI messages only, so it should probably be in the >> relevant place in libavcodec. >> > I have used this function in my local code to decode HDR10+ of SEI message > (libavcodec) and also HDR10+ in matroska container (ibavformat). I will > push them in my next CLs. Um, so? The parsing code can still go next to its uses in libavcodec. >>> +{ >>> + int w, i, j; >>> + GetBitContext gb; >>> + if (!data) >>> + return AVERROR_INVALIDDATA; >>> + >>> + int ret = init_get_bits8(&gb, data, size); >>> + if (ret < 0) >>> + return AVERROR_INVALIDDATA; >>> + >>> + 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; >>> + for (w = 1; w < s->num_windows; w++) { >>> + 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; >>> + >>> + 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); >> >> You're range-checking some fields here but not others. Should everything >> be verified against the constraints so that the comments on the structure >> in the header file are actually true? >> > The intention is to only pass through HDR10+ from src file to dst file. The > interpretation of the data is on the user/caller. I think it is better not > to drop the metadata by verification. I did some range checking like row or > col in order to simply avoid accessing out of memory (avoid indices that > are out of array size). If this is the intent then you probably need to modify all of the documentation on the AVDynamicHDRPlus to make clear that invalid values are acceptable in that structure and that other code will need to be able to handle them at least to the level of not-UB. >>> + 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) < 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; >>> + } >>> + } >>> + >> >> It might be sensible to set the unused members of the structure to >> something fixed rather than leaving them uninitialised, so that attempting >> to copy the structure isn't UB. >> > If some members are not set by user and therefore they dont get a value in > the decode function, they will never be accessed in the encode function or > by user based on the HDR10+ logic. So it really doesn't matter to > initialize them. Based on the logic, there is no way to interpret them.> Please note that country_code is always unused and should be removed, but > we cannot as it breaks API. It would great if you allow to remove it. I > wrote the code and I am sure no one used the code yet. I am the only user > and you can check it in github. You could mark it deprecated, and then maybe it could be removed on the next major version bump if there are no users. (It can't actually be removed before then because it would change the layout of the structure.) >>> + return 0; >>> +} >>> diff --git a/libavutil/hdr_dynamic_metadata.h >> b/libavutil/hdr_dynamic_metadata.h >>> index 2d72de56ae..28c657481f 100644 >>> --- a/libavutil/hdr_dynamic_metadata.h >>> +++ b/libavutil/hdr_dynamic_metadata.h >>> @@ -265,7 +265,7 @@ typedef struct AVDynamicHDRPlus { >>> >>> /** >>> * The nominal maximum display luminance of the targeted system >> display, >>> - * in units of 0.0001 candelas per square metre. The value shall be >> in >>> + * in units of 1 candelas per square metre. The value shall be in >> >> This was a error in the spec itself, I think? It should probably be >> isolated into its own commit with suitable references explaining what >> happened. >> > I think it's wrong. It more makes sense now. The spec is updated in March > 2019, > https://www.atsc.org/wp-content/uploads/2018/02/A341S34-1-582r4-A341-Amendment-2094-40.pdf. > I changed it based on the updated version. Please turn this into a separate commit including the explanation that it was a typo in the standard. (That can be applied separately without needing anything else using it.) >>> ...>>> + * @param size The size of the user data registered ITU-T T.35 (SMPTE >> ST 2094-40). >>> + * @param s The decoded AVDynamicHDRPlus structure. >>> + * >>> + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR. >>> + */ >>> +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, >> AVDynamicHDRPlus *s); >>> + >>> ... >> >> It would help to review if this were included in the decoder so that it >> can actually be tested. >> > Can you please explain a bit more? :) It is very hard to run a function to observe its behaviour in-use when nothing calls it. Thanks, - Mark
On Wed, Feb 19, 2020 at 4:22 PM Mark Thompson <sw@jkqxz.net> wrote: > On 10/02/2020 19:38, Mohammad Izadi wrote: > > On Sun, Feb 9, 2020 at 8:31 AM Mark Thompson <sw@jkqxz.net> wrote: > > > >> On 07/02/2020 17:46, Mohammad Izadi wrote: > >>> From: Mohammad Izadi <izadi@google.com> > >>> > >>> Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side > >> data in the follow-up CLs. > >>> --- > >>> libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ > >>> libavutil/hdr_dynamic_metadata.h | 12 ++- > >>> libavutil/version.h | 2 +- > >>> 3 files changed, 177 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/libavutil/hdr_dynamic_metadata.c > >> b/libavutil/hdr_dynamic_metadata.c > >>> index 0fa1ee82de..a988bcd2d5 100644 > >>> --- a/libavutil/hdr_dynamic_metadata.c > >>> +++ b/libavutil/hdr_dynamic_metadata.c > >>> ... > >>> +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; > >> > >> It would probably be clearer just to put these constants inline; there > >> isn't really any use to having the values standalone. > >> > > You are right. Actually, I am going to push the encode function in my > next > > CL and the static vars will be shared between both encode and decode > > function. > > My point is that separating the constants is actively unhelpful in > matching the standard to the code. > > From the standard you can read in one paragraph: > > "knee_point_x[ w ] – specifies the x coordinate of the separation point > between the linear part and thecurved part of the tone mapping function. > The value of knee_point_x[ w ] shall be in the range of 0 to 4,095, > inclusive, where 0 maps to 0 cd/m2 and the full range of 4,095 maps to the > maximum of the scene maximum luminance and the target peak luminance in > cd/m2." > > But you've split that into two distinct locations by putting > knee_point_den as a constant here and then the code below which refers to > it when it would be clearer to just put the constant in the code in the > same way that the standard does. > > I totally understand your point. I know it should be close to their local vars and settings. But I am not sure if you get my point. I am going to use the constants in two functions in my next CL. If I move them to the function now, I have to move them back in the next CL. I am using the constants in encode and decode function. I cannot define them separately and locally if I want to follow programming standards. I need to make sure the values are the same for both functions. > >>> + > >>> AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size) > >>> { > >>> AVDynamicHDRPlus *hdr_plus = av_mallocz(sizeof(AVDynamicHDRPlus)); > >>> @@ -45,3 +54,159 @@ AVDynamicHDRPlus > >> *av_dynamic_hdr_plus_create_side_data(AVFrame *frame) > >>> > >>> return (AVDynamicHDRPlus *)side_data->data; > >>> } > >>> + > >>> +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, > >>> + AVDynamicHDRPlus *s) > >> > >> Why is this function being added to libavutil? It looks like it's meant > >> for decoding UDR SEI messages only, so it should probably be in the > >> relevant place in libavcodec. > >> > > I have used this function in my local code to decode HDR10+ of SEI > message > > (libavcodec) and also HDR10+ in matroska container (ibavformat). I will > > push them in my next CLs. > > Um, so? The parsing code can still go next to its uses in libavcodec. > I just wanted to say it would be used in two places libavcodec and libavformat. Never mind, I don't know where is suitable for them. Where do you want to move them? > > >>> +{ > >>> + int w, i, j; > >>> + GetBitContext gb; > >>> + if (!data) > >>> + return AVERROR_INVALIDDATA; > >>> + > >>> + int ret = init_get_bits8(&gb, data, size); > >>> + if (ret < 0) > >>> + return AVERROR_INVALIDDATA; > >>> + > >>> + 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; > >>> + for (w = 1; w < s->num_windows; w++) { > >>> + 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; > >>> + > >>> + 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); > >> > >> You're range-checking some fields here but not others. Should > everything > >> be verified against the constraints so that the comments on the > structure > >> in the header file are actually true? > >> > > The intention is to only pass through HDR10+ from src file to dst file. > The > > interpretation of the data is on the user/caller. I think it is better > not > > to drop the metadata by verification. I did some range checking like row > or > > col in order to simply avoid accessing out of memory (avoid indices that > > are out of array size). > > If this is the intent then you probably need to modify all of the > documentation on the AVDynamicHDRPlus to make clear that invalid values are > acceptable in that structure and that other code will need to be able to > handle them at least to the level of not-UB. > Please refer me to a rule or standard that say such modification is necessary. Thanks > > >>> + 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) < 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; > >>> + } > >>> + } > >>> + > >> > >> It might be sensible to set the unused members of the structure to > >> something fixed rather than leaving them uninitialised, so that > attempting > >> to copy the structure isn't UB. > >> > > If some members are not set by user and therefore they dont get a value > in > > the decode function, they will never be accessed in the encode function > or > > by user based on the HDR10+ logic. So it really doesn't matter to > > initialize them. Based on the logic, there is no way to interpret them.> > Please note that country_code is always unused and should be removed, but > > we cannot as it breaks API. It would great if you allow to remove it. I > > wrote the code and I am sure no one used the code yet. I am the only user > > and you can check it in github. > > You could mark it deprecated, and then maybe it could be removed on the > next major version bump if there are no users. (It can't actually be > removed before then because it would change the layout of the structure.) > Thanks! > > >>> + return 0; > >>> +} > >>> diff --git a/libavutil/hdr_dynamic_metadata.h > >> b/libavutil/hdr_dynamic_metadata.h > >>> index 2d72de56ae..28c657481f 100644 > >>> --- a/libavutil/hdr_dynamic_metadata.h > >>> +++ b/libavutil/hdr_dynamic_metadata.h > >>> @@ -265,7 +265,7 @@ typedef struct AVDynamicHDRPlus { > >>> > >>> /** > >>> * The nominal maximum display luminance of the targeted system > >> display, > >>> - * in units of 0.0001 candelas per square metre. The value shall > be > >> in > >>> + * in units of 1 candelas per square metre. The value shall be in > >> > >> This was a error in the spec itself, I think? It should probably be > >> isolated into its own commit with suitable references explaining what > >> happened. > >> > > I think it's wrong. It more makes sense now. The spec is updated in March > > 2019, > > > https://www.atsc.org/wp-content/uploads/2018/02/A341S34-1-582r4-A341-Amendment-2094-40.pdf > . > > I changed it based on the updated version. > > Please turn this into a separate commit including the explanation that it > was a typo in the standard. (That can be applied separately without > needing anything else using it.) > > Sorry, I prefer to keep it unless you refer me to a rule or standard on that (its a total waste of time to just modify a comment in "a separate CL"; so funny). > >>> ...>>> + * @param size The size of the user data registered ITU-T T.35 > (SMPTE > >> ST 2094-40). > >>> + * @param s The decoded AVDynamicHDRPlus structure. > >>> + * > >>> + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR. > >>> + */ > >>> +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, > >> AVDynamicHDRPlus *s); > >>> + > >>> ... > >> > >> It would help to review if this were included in the decoder so that it > >> can actually be tested. > >> > > Can you please explain a bit more? :) > It is very hard to run a function to observe its behaviour in-use when > nothing calls it. > > Thanks, > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c index 0fa1ee82de..a988bcd2d5 100644 --- a/libavutil/hdr_dynamic_metadata.c +++ b/libavutil/hdr_dynamic_metadata.c @@ -19,8 +19,17 @@ */ #include "hdr_dynamic_metadata.h" +#include "libavcodec/get_bits.h" #include "mem.h" +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; + AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size) { AVDynamicHDRPlus *hdr_plus = av_mallocz(sizeof(AVDynamicHDRPlus)); @@ -45,3 +54,159 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame) return (AVDynamicHDRPlus *)side_data->data; } + +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, + AVDynamicHDRPlus *s) +{ + int w, i, j; + GetBitContext gb; + if (!data) + return AVERROR_INVALIDDATA; + + int ret = init_get_bits8(&gb, data, size); + if (ret < 0) + return AVERROR_INVALIDDATA; + + 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; + for (w = 1; w < s->num_windows; w++) { + 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; + + 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; + 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; + } + } + } + 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; + } + 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; + } + } + + return 0; +} diff --git a/libavutil/hdr_dynamic_metadata.h b/libavutil/hdr_dynamic_metadata.h index 2d72de56ae..28c657481f 100644 --- a/libavutil/hdr_dynamic_metadata.h +++ b/libavutil/hdr_dynamic_metadata.h @@ -265,7 +265,7 @@ typedef struct AVDynamicHDRPlus { /** * The nominal maximum display luminance of the targeted system display, - * in units of 0.0001 candelas per square metre. The value shall be in + * in units of 1 candelas per square metre. The value shall be in * the range of 0 to 10000, inclusive. */ AVRational targeted_system_display_maximum_luminance; @@ -340,4 +340,14 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size); */ AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame); +/** + * Decode SMPTE ST 2094-40 (the user data registered ITU-T T.35) to AVDynamicHDRPlus. + * @param data The user data registered ITU-T T.35 (SMPTE ST 2094-40). + * @param size The size of the user data registered ITU-T T.35 (SMPTE ST 2094-40). + * @param s The decoded AVDynamicHDRPlus structure. + * + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR. + */ +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, AVDynamicHDRPlus *s); + #endif /* AVUTIL_HDR_DYNAMIC_METADATA_H */ diff --git a/libavutil/version.h b/libavutil/version.h index af8f614aff..2bc1b98615 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -79,7 +79,7 @@ */ #define LIBAVUTIL_VERSION_MAJOR 56 -#define LIBAVUTIL_VERSION_MINOR 38 +#define LIBAVUTIL_VERSION_MINOR 39 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
From: Mohammad Izadi <izadi@google.com> Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side data in the follow-up CLs. --- libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ libavutil/hdr_dynamic_metadata.h | 12 ++- libavutil/version.h | 2 +- 3 files changed, 177 insertions(+), 2 deletions(-)