[FFmpeg-devel,v6,1/2] avcodec/avutil: move dynamic HDR10+ metadata parsing to libavutil

Signed-off-by: Raphaël Zumer <rzumer@tebako.net>
---
 libavcodec/Makefile              |   6 +-
 libavcodec/av1dec.c              |   6 +-
 libavcodec/dynamic_hdr10_plus.c  | 198 -------------------------------
 libavcodec/dynamic_hdr10_plus.h  |  35 ------
 libavcodec/h2645_sei.c           |   6 +-
 libavcodec/libdav1d.c            |   6 +-
 libavutil/hdr_dynamic_metadata.c | 180 ++++++++++++++++++++++++++++
 libavutil/hdr_dynamic_metadata.h |  11 ++
 8 files changed, 203 insertions(+), 245 deletions(-)
 delete mode 100644 libavcodec/dynamic_hdr10_plus.c
 delete mode 100644 libavcodec/dynamic_hdr10_plus.h

Raphaël Zumer:
> Signed-off-by: Raphaël Zumer <rzumer@tebako.net>
> ---
>  libavcodec/Makefile              |   6 +-
>  libavcodec/av1dec.c              |   6 +-
>  libavcodec/dynamic_hdr10_plus.c  | 198 -------------------------------
>  libavcodec/dynamic_hdr10_plus.h  |  35 ------
>  libavcodec/h2645_sei.c           |   6 +-
>  libavcodec/libdav1d.c            |   6 +-
>  libavutil/hdr_dynamic_metadata.c | 180 ++++++++++++++++++++++++++++
>  libavutil/hdr_dynamic_metadata.h |  11 ++
>  8 files changed, 203 insertions(+), 245 deletions(-)
>  delete mode 100644 libavcodec/dynamic_hdr10_plus.c
>  delete mode 100644 libavcodec/dynamic_hdr10_plus.h
> 
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index abae4909d2..408ecd1e31 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -104,7 +104,7 @@ OBJS-$(CONFIG_H264_SEI)                += h264_sei.o h2645_sei.o
>  OBJS-$(CONFIG_HEVCPARSE)               += hevc_parse.o hevc_ps.o hevc_data.o \
>                                            h2645data.o h2645_parse.o h2645_vui.o
>  OBJS-$(CONFIG_HEVC_SEI)                += hevc_sei.o h2645_sei.o \
> -                                          dynamic_hdr10_plus.o dynamic_hdr_vivid.o
> +                                          dynamic_hdr_vivid.o
>  OBJS-$(CONFIG_HPELDSP)                 += hpeldsp.o
>  OBJS-$(CONFIG_HUFFMAN)                 += huffman.o
>  OBJS-$(CONFIG_HUFFYUVDSP)              += huffyuvdsp.o
> @@ -250,7 +250,7 @@ OBJS-$(CONFIG_ATRAC3PAL_DECODER)       += atrac3plusdec.o atrac3plus.o \
>  OBJS-$(CONFIG_ATRAC9_DECODER)          += atrac9dec.o
>  OBJS-$(CONFIG_AURA_DECODER)            += cyuv.o
>  OBJS-$(CONFIG_AURA2_DECODER)           += aura.o
> -OBJS-$(CONFIG_AV1_DECODER)             += av1dec.o dynamic_hdr10_plus.o
> +OBJS-$(CONFIG_AV1_DECODER)             += av1dec.o
>  OBJS-$(CONFIG_AV1_CUVID_DECODER)       += cuviddec.o
>  OBJS-$(CONFIG_AV1_MEDIACODEC_DECODER)  += mediacodecdec.o
>  OBJS-$(CONFIG_AV1_NVENC_ENCODER)       += nvenc_av1.o nvenc.o
> @@ -1082,7 +1082,7 @@ OBJS-$(CONFIG_LIBARIBB24_DECODER)         += libaribb24.o ass.o
>  OBJS-$(CONFIG_LIBCELT_DECODER)            += libcelt_dec.o
>  OBJS-$(CONFIG_LIBCODEC2_DECODER)          += libcodec2.o
>  OBJS-$(CONFIG_LIBCODEC2_ENCODER)          += libcodec2.o
> -OBJS-$(CONFIG_LIBDAV1D_DECODER)           += libdav1d.o dynamic_hdr10_plus.o
> +OBJS-$(CONFIG_LIBDAV1D_DECODER)           += libdav1d.o
>  OBJS-$(CONFIG_LIBDAVS2_DECODER)           += libdavs2.o
>  OBJS-$(CONFIG_LIBFDK_AAC_DECODER)         += libfdk-aacdec.o
>  OBJS-$(CONFIG_LIBFDK_AAC_ENCODER)         += libfdk-aacenc.o
> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
> index a80e37e33f..df393fe3d0 100644
> --- a/libavcodec/av1dec.c
> +++ b/libavcodec/av1dec.c
> @@ -20,6 +20,7 @@
>  
>  #include "config_components.h"
>  
> +#include "libavutil/hdr_dynamic_metadata.h"
>  #include "libavutil/film_grain_params.h"
>  #include "libavutil/mastering_display_metadata.h"
>  #include "libavutil/pixdesc.h"
> @@ -30,7 +31,6 @@
>  #include "bytestream.h"
>  #include "codec_internal.h"
>  #include "decode.h"
> -#include "dynamic_hdr10_plus.h"
>  #include "hwconfig.h"
>  #include "profiles.h"
>  #include "thread.h"
> @@ -925,8 +925,8 @@ static int export_itut_t35(AVCodecContext *avctx, AVFrame *frame,
>          if (!hdrplus)
>              return AVERROR(ENOMEM);
>  
> -        ret = ff_parse_itu_t_t35_to_dynamic_hdr10_plus(hdrplus, gb.buffer,
> -                                                       bytestream2_get_bytes_left(&gb));
> +        ret = av_dynamic_hdr_plus_from_t35(hdrplus, gb.buffer,
> +                                           bytestream2_get_bytes_left(&gb));
>          if (ret < 0)
>              return ret;
>          break;
> diff --git a/libavcodec/dynamic_hdr10_plus.c b/libavcodec/dynamic_hdr10_plus.c
> deleted file mode 100644
> index 34a44aac65..0000000000
> --- a/libavcodec/dynamic_hdr10_plus.c
> +++ /dev/null
> @@ -1,198 +0,0 @@
> -/*
> - * This file is part of FFmpeg.
> - *
> - * FFmpeg is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU Lesser General Public
> - * License as published by the Free Software Foundation; either
> - * version 2.1 of the License, or (at your option) any later version.
> - *
> - * FFmpeg is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> - * Lesser General Public License for more details.
> - *
> - * You should have received a copy of the GNU Lesser General Public
> - * License along with FFmpeg; if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> - */
> -
> -#include "dynamic_hdr10_plus.h"
> -#include "get_bits.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;
> -
> -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;
> -
> -    if (get_bits_left(gb) < 10)
> -        return AVERROR_INVALIDDATA;
> -
> -    s->application_version = get_bits(gb, 8);
> -    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 (int w = 1; w < s->num_windows; w++) {
> -        // The corners are set to absolute coordinates here. They should be
> -        // converted to the relative coordinates (in [0, 1]) in the decoder.
> -        AVHDRPlusColorTransformParams *params = &s->params[w];
> -        params->window_upper_left_corner_x =
> -            (AVRational){get_bits(gb, 16), 1};
> -        params->window_upper_left_corner_y =
> -            (AVRational){get_bits(gb, 16), 1};
> -        params->window_lower_right_corner_x =
> -            (AVRational){get_bits(gb, 16), 1};
> -        params->window_lower_right_corner_y =
> -            (AVRational){get_bits(gb, 16), 1};
> -
> -        params->center_of_ellipse_x = get_bits(gb, 16);
> -        params->center_of_ellipse_y = get_bits(gb, 16);
> -        params->rotation_angle = get_bits(gb, 8);
> -        params->semimajor_axis_internal_ellipse = get_bits(gb, 16);
> -        params->semimajor_axis_external_ellipse = get_bits(gb, 16);
> -        params->semiminor_axis_external_ellipse = get_bits(gb, 16);
> -        params->overlap_process_option = get_bits1(gb);
> -    }
> -
> -    if (get_bits_left(gb) < 28)
> -        return AVERROR_INVALIDDATA;
> -
> -    s->targeted_system_display_maximum_luminance =
> -        (AVRational){get_bits_long(gb, 27), 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_INVALIDDATA;
> -        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_INVALIDDATA;
> -
> -        for (int i = 0; i < rows; i++) {
> -            for (int j = 0; j < cols; j++) {
> -                s->targeted_system_display_actual_peak_luminance[i][j] =
> -                    (AVRational){get_bits(gb, 4), peak_luminance_den};
> -            }
> -        }
> -    }
> -    for (int w = 0; w < s->num_windows; w++) {
> -        AVHDRPlusColorTransformParams *params = &s->params[w];
> -        if (get_bits_left(gb) < (3 * 17 + 17 + 4))
> -            return AVERROR_INVALIDDATA;
> -
> -        for (int i = 0; i < 3; i++) {
> -            params->maxscl[i] =
> -                (AVRational){get_bits(gb, 17), rgb_den};
> -        }
> -        params->average_maxrgb =
> -            (AVRational){get_bits(gb, 17), rgb_den};
> -        params->num_distribution_maxrgb_percentiles = get_bits(gb, 4);
> -
> -        if (get_bits_left(gb) <
> -            (params->num_distribution_maxrgb_percentiles * 24))
> -            return AVERROR_INVALIDDATA;
> -
> -        for (int i = 0; i < params->num_distribution_maxrgb_percentiles; i++) {
> -            params->distribution_maxrgb[i].percentage = get_bits(gb, 7);
> -            params->distribution_maxrgb[i].percentile =
> -                (AVRational){get_bits(gb, 17), rgb_den};
> -        }
> -
> -        if (get_bits_left(gb) < 10)
> -            return AVERROR_INVALIDDATA;
> -
> -        params->fraction_bright_pixels = (AVRational){get_bits(gb, 10), fraction_pixel_den};
> -    }
> -    if (get_bits_left(gb) < 1)
> -        return AVERROR_INVALIDDATA;
> -    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_INVALIDDATA;
> -        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_INVALIDDATA;
> -
> -        for (int i = 0; i < rows; i++) {
> -            for (int j = 0; j < cols; j++) {
> -                s->mastering_display_actual_peak_luminance[i][j] =
> -                    (AVRational){get_bits(gb, 4), peak_luminance_den};
> -            }
> -        }
> -    }
> -
> -    for (int w = 0; w < s->num_windows; w++) {
> -        AVHDRPlusColorTransformParams *params = &s->params[w];
> -        if (get_bits_left(gb) < 1)
> -            return AVERROR_INVALIDDATA;
> -
> -        params->tone_mapping_flag = get_bits1(gb);
> -        if (params->tone_mapping_flag) {
> -            if (get_bits_left(gb) < 28)
> -                return AVERROR_INVALIDDATA;
> -
> -            params->knee_point_x =
> -                (AVRational){get_bits(gb, 12), knee_point_den};
> -            params->knee_point_y =
> -                (AVRational){get_bits(gb, 12), knee_point_den};
> -            params->num_bezier_curve_anchors = get_bits(gb, 4);
> -
> -            if (get_bits_left(gb) < (params->num_bezier_curve_anchors * 10))
> -                return AVERROR_INVALIDDATA;
> -
> -            for (int i = 0; i < params->num_bezier_curve_anchors; i++) {
> -                params->bezier_curve_anchors[i] =
> -                    (AVRational){get_bits(gb, 10), bezier_anchor_den};
> -            }
> -        }
> -
> -        if (get_bits_left(gb) < 1)
> -            return AVERROR_INVALIDDATA;
> -        params->color_saturation_mapping_flag = get_bits1(gb);
> -        if (params->color_saturation_mapping_flag) {
> -            if (get_bits_left(gb) < 6)
> -                return AVERROR_INVALIDDATA;
> -            params->color_saturation_weight =
> -                (AVRational){get_bits(gb, 6), saturation_weight_den};
> -        }
> -    }
> -
> -    return 0;
> -}
> diff --git a/libavcodec/dynamic_hdr10_plus.h b/libavcodec/dynamic_hdr10_plus.h
> deleted file mode 100644
> index cd7acf0432..0000000000
> --- a/libavcodec/dynamic_hdr10_plus.h
> +++ /dev/null
> @@ -1,35 +0,0 @@
> -/*
> - * This file is part of FFmpeg.
> - *
> - * FFmpeg is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU Lesser General Public
> - * License as published by the Free Software Foundation; either
> - * version 2.1 of the License, or (at your option) any later version.
> - *
> - * FFmpeg is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> - * Lesser General Public License for more details.
> - *
> - * You should have received a copy of the GNU Lesser General Public
> - * License along with FFmpeg; if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> - */
> -
> -#ifndef AVCODEC_DYNAMIC_HDR10_PLUS_H
> -#define AVCODEC_DYNAMIC_HDR10_PLUS_H
> -
> -#include "libavutil/hdr_dynamic_metadata.h"
> -
> -/**
> - * Parse the user data registered ITU-T T.35 to AVbuffer (AVDynamicHDRPlus).
> - * @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(AVDynamicHDRPlus *s, const uint8_t *data,
> -                                             int size);
> -
> -#endif /* AVCODEC_DYNAMIC_HDR10_PLUS_H */
> diff --git a/libavcodec/h2645_sei.c b/libavcodec/h2645_sei.c
> index 6e4a9a1af2..63ab711bc9 100644
> --- a/libavcodec/h2645_sei.c
> +++ b/libavcodec/h2645_sei.c
> @@ -27,13 +27,13 @@
>  
>  #include "libavutil/ambient_viewing_environment.h"
>  #include "libavutil/display.h"
> +#include "libavutil/hdr_dynamic_metadata.h"
>  #include "libavutil/film_grain_params.h"
>  #include "libavutil/pixdesc.h"
>  #include "libavutil/stereo3d.h"
>  
>  #include "atsc_a53.h"
>  #include "avcodec.h"
> -#include "dynamic_hdr10_plus.h"
>  #include "dynamic_hdr_vivid.h"
>  #include "get_bits.h"
>  #include "golomb.h"
> @@ -52,8 +52,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(metadata, gb->buffer,
> -                                                   bytestream2_get_bytes_left(gb));
> +    err = av_dynamic_hdr_plus_from_t35(metadata, gb->buffer,
> +                                       bytestream2_get_bytes_left(gb));
>      if (err < 0) {
>          av_free(metadata);
>          return err;
> diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
> index eb1225ea1a..50c4ceee40 100644
> --- a/libavcodec/libdav1d.c
> +++ b/libavcodec/libdav1d.c
> @@ -24,6 +24,7 @@
>  #include "libavutil/avassert.h"
>  #include "libavutil/cpu.h"
>  #include "libavutil/film_grain_params.h"
> +#include "libavutil/hdr_dynamic_metadata.h"
>  #include "libavutil/mastering_display_metadata.h"
>  #include "libavutil/imgutils.h"
>  #include "libavutil/opt.h"
> @@ -33,7 +34,6 @@
>  #include "bytestream.h"
>  #include "codec_internal.h"
>  #include "decode.h"
> -#include "dynamic_hdr10_plus.h"
>  #include "internal.h"
>  
>  #define FF_DAV1D_VERSION_AT_LEAST(x,y) \
> @@ -555,8 +555,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
>                  goto fail;
>              }
>  
> -            res = ff_parse_itu_t_t35_to_dynamic_hdr10_plus(hdrplus, gb.buffer,
> -                                                           bytestream2_get_bytes_left(&gb));
> +            res = av_dynamic_hdr_plus_from_t35(hdrplus, gb.buffer,
> +                                               bytestream2_get_bytes_left(&gb));
>              if (res < 0)
>                  goto fail;
>              break;
> diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c
> index 0fa1ee82de..223db2fd66 100644
> --- a/libavutil/hdr_dynamic_metadata.c
> +++ b/libavutil/hdr_dynamic_metadata.c
> @@ -20,6 +20,16 @@
>  
>  #include "hdr_dynamic_metadata.h"
>  #include "mem.h"
> +#include "libavcodec/get_bits.h"
> +#include "libavcodec/put_bits.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)
>  {
> @@ -45,3 +55,173 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame)
>  
>      return (AVDynamicHDRPlus *)side_data->data;
>  }
> +
> +int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data,
> +                                 size_t size)
> +{
> +    GetBitContext gbc, *gb = &gbc;
> +    int ret;
> +
> +    if (!s)
> +        return AVERROR(ENOMEM);
> +
> +    ret = init_get_bits8(gb, data, size);
> +    if (ret < 0)
> +        return ret;
> +
> +    if (get_bits_left(gb) < 10)
> +        return AVERROR_INVALIDDATA;
> +
> +    s->application_version = get_bits(gb, 8);
> +    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 (int w = 1; w < s->num_windows; w++) {
> +        // The corners are set to absolute coordinates here. They should be
> +        // converted to the relative coordinates (in [0, 1]) in the decoder.
> +        AVHDRPlusColorTransformParams *params = &s->params[w];
> +        params->window_upper_left_corner_x =
> +            (AVRational){get_bits(gb, 16), 1};
> +        params->window_upper_left_corner_y =
> +            (AVRational){get_bits(gb, 16), 1};
> +        params->window_lower_right_corner_x =
> +            (AVRational){get_bits(gb, 16), 1};
> +        params->window_lower_right_corner_y =
> +            (AVRational){get_bits(gb, 16), 1};
> +
> +        params->center_of_ellipse_x = get_bits(gb, 16);
> +        params->center_of_ellipse_y = get_bits(gb, 16);
> +        params->rotation_angle = get_bits(gb, 8);
> +        params->semimajor_axis_internal_ellipse = get_bits(gb, 16);
> +        params->semimajor_axis_external_ellipse = get_bits(gb, 16);
> +        params->semiminor_axis_external_ellipse = get_bits(gb, 16);
> +        params->overlap_process_option = get_bits1(gb);
> +    }
> +
> +    if (get_bits_left(gb) < 28)
> +        return AVERROR_INVALIDDATA;
> +
> +    s->targeted_system_display_maximum_luminance =
> +        (AVRational){get_bits_long(gb, 27), 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_INVALIDDATA;
> +        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_INVALIDDATA;
> +
> +        for (int i = 0; i < rows; i++) {
> +            for (int j = 0; j < cols; j++) {
> +                s->targeted_system_display_actual_peak_luminance[i][j] =
> +                    (AVRational){get_bits(gb, 4), peak_luminance_den};
> +            }
> +        }
> +    }
> +    for (int w = 0; w < s->num_windows; w++) {
> +        AVHDRPlusColorTransformParams *params = &s->params[w];
> +        if (get_bits_left(gb) < (3 * 17 + 17 + 4))
> +            return AVERROR_INVALIDDATA;
> +
> +        for (int i = 0; i < 3; i++) {
> +            params->maxscl[i] =
> +                (AVRational){get_bits(gb, 17), rgb_den};
> +        }
> +        params->average_maxrgb =
> +            (AVRational){get_bits(gb, 17), rgb_den};
> +        params->num_distribution_maxrgb_percentiles = get_bits(gb, 4);
> +
> +        if (get_bits_left(gb) <
> +            (params->num_distribution_maxrgb_percentiles * 24))
> +            return AVERROR_INVALIDDATA;
> +
> +        for (int i = 0; i < params->num_distribution_maxrgb_percentiles; i++) {
> +            params->distribution_maxrgb[i].percentage = get_bits(gb, 7);
> +            params->distribution_maxrgb[i].percentile =
> +                (AVRational){get_bits(gb, 17), rgb_den};
> +        }
> +
> +        if (get_bits_left(gb) < 10)
> +            return AVERROR_INVALIDDATA;
> +
> +        params->fraction_bright_pixels = (AVRational){get_bits(gb, 10), fraction_pixel_den};
> +    }
> +    if (get_bits_left(gb) < 1)
> +        return AVERROR_INVALIDDATA;
> +    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_INVALIDDATA;
> +        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_INVALIDDATA;
> +
> +        for (int i = 0; i < rows; i++) {
> +            for (int j = 0; j < cols; j++) {
> +                s->mastering_display_actual_peak_luminance[i][j] =
> +                    (AVRational){get_bits(gb, 4), peak_luminance_den};
> +            }
> +        }
> +    }
> +
> +    for (int w = 0; w < s->num_windows; w++) {
> +        AVHDRPlusColorTransformParams *params = &s->params[w];
> +        if (get_bits_left(gb) < 1)
> +            return AVERROR_INVALIDDATA;
> +
> +        params->tone_mapping_flag = get_bits1(gb);
> +        if (params->tone_mapping_flag) {
> +            if (get_bits_left(gb) < 28)
> +                return AVERROR_INVALIDDATA;
> +
> +            params->knee_point_x =
> +                (AVRational){get_bits(gb, 12), knee_point_den};
> +            params->knee_point_y =
> +                (AVRational){get_bits(gb, 12), knee_point_den};
> +            params->num_bezier_curve_anchors = get_bits(gb, 4);
> +
> +            if (get_bits_left(gb) < (params->num_bezier_curve_anchors * 10))
> +                return AVERROR_INVALIDDATA;
> +
> +            for (int i = 0; i < params->num_bezier_curve_anchors; i++) {
> +                params->bezier_curve_anchors[i] =
> +                    (AVRational){get_bits(gb, 10), bezier_anchor_den};
> +            }
> +        }
> +
> +        if (get_bits_left(gb) < 1)
> +            return AVERROR_INVALIDDATA;
> +        params->color_saturation_mapping_flag = get_bits1(gb);
> +        if (params->color_saturation_mapping_flag) {
> +            if (get_bits_left(gb) < 6)
> +                return AVERROR_INVALIDDATA;
> +            params->color_saturation_weight =
> +                (AVRational){get_bits(gb, 6), saturation_weight_den};
> +        }
> +    }
> +
> +    return 0;
> +}
> diff --git a/libavutil/hdr_dynamic_metadata.h b/libavutil/hdr_dynamic_metadata.h
> index 2d72de56ae..3d327241c1 100644
> --- a/libavutil/hdr_dynamic_metadata.h
> +++ b/libavutil/hdr_dynamic_metadata.h
> @@ -340,4 +340,15 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size);
>   */
>  AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame);
>  
> +/**
> + * Parse the user data registered ITU-T T.35 to AVbuffer (AVDynamicHDRPlus).
> + * @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.

The implementation of this function uses the GetBit-API which requires
the buffer to be padded; yet the documentation does not mention this.

Looking at the calculation in av_dynamic_hdr_plus_to_t35(), it seems
that the maximum bitlength of a valid ITU-T T.35 payload is
48+2×937+27+1+10+25×25×4+3×82+(3×15×24)+(1+10+25×25×4+3×1)+(3×(28+15×10))+3+3×6
= 8855 bits (please double-check this). This means we can just copy that
much into a padded buffer on the stack and ignore the padding. We may
then remove the padding from the serialization function, too.

(The GetBit-API btw actually does not need AV_INPUT_BUFFER_PADDING_SIZE
bytes of padding, but way less (I think 8 bytes or so). I don't really
like using AV_INPUT_BUFFER_PADDING_SIZE here.)

> + *
> + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR.
> + */
> +int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data,
> +                                 size_t size);
> +
>  #endif /* AVUTIL_HDR_DYNAMIC_METADATA_H */

On 3/13/23 20:44, Andreas Rheinhardt wrote:
>> diff --git a/libavutil/hdr_dynamic_metadata.h b/libavutil/hdr_dynamic_metadata.h
>> index 2d72de56ae..3d327241c1 100644
>> --- a/libavutil/hdr_dynamic_metadata.h
>> +++ b/libavutil/hdr_dynamic_metadata.h
>> @@ -340,4 +340,15 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size);
>>   */
>>  AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame);
>>  
>> +/**
>> + * Parse the user data registered ITU-T T.35 to AVbuffer (AVDynamicHDRPlus).
>> + * @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.
> The implementation of this function uses the GetBit-API which requires
> the buffer to be padded; yet the documentation does not mention this.
>
> Looking at the calculation in av_dynamic_hdr_plus_to_t35(), it seems
> that the maximum bitlength of a valid ITU-T T.35 payload is
> 48+2×937+27+1+10+25×25×4+3×82+(3×15×24)+(1+10+25×25×4+3×1)+(3×(28+15×10))+3+3×6
> = 8855 bits (please double-check this). This means we can just copy that
> much into a padded buffer on the stack and ignore the padding. We may
> then remove the padding from the serialization function, too.
>
> (The GetBit-API btw actually does not need AV_INPUT_BUFFER_PADDING_SIZE
> bytes of padding, but way less (I think 8 bytes or so). I don't really
> like using AV_INPUT_BUFFER_PADDING_SIZE here.)

Hi,

From get_bits.h (init_get_bits()): "buffer, must be AV_INPUT_BUFFER_PADDING_SIZE bytes larger than the actual read bits". Is this wrong? If so, what is the correct value and is it defined anywhere? What about "other places where one needed a padded buffer" as mentioned in your previous comment?

Why is copying the buffer to the stack in the parsing function preferable to padding it when serializing? The vast majority of HDR10+ payloads will be well below the max size.

RZ

Raphaël Zumer:
> On 3/13/23 20:44, Andreas Rheinhardt wrote:
>>> diff --git a/libavutil/hdr_dynamic_metadata.h b/libavutil/hdr_dynamic_metadata.h
>>> index 2d72de56ae..3d327241c1 100644
>>> --- a/libavutil/hdr_dynamic_metadata.h
>>> +++ b/libavutil/hdr_dynamic_metadata.h
>>> @@ -340,4 +340,15 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size);
>>>   */
>>>  AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame);
>>>  
>>> +/**
>>> + * Parse the user data registered ITU-T T.35 to AVbuffer (AVDynamicHDRPlus).
>>> + * @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.
>> The implementation of this function uses the GetBit-API which requires
>> the buffer to be padded; yet the documentation does not mention this.
>>
>> Looking at the calculation in av_dynamic_hdr_plus_to_t35(), it seems
>> that the maximum bitlength of a valid ITU-T T.35 payload is
>> 48+2×937+27+1+10+25×25×4+3×82+(3×15×24)+(1+10+25×25×4+3×1)+(3×(28+15×10))+3+3×6
>> = 8855 bits (please double-check this). This means we can just copy that
>> much into a padded buffer on the stack and ignore the padding. We may
>> then remove the padding from the serialization function, too.
>>
>> (The GetBit-API btw actually does not need AV_INPUT_BUFFER_PADDING_SIZE
>> bytes of padding, but way less (I think 8 bytes or so). I don't really
>> like using AV_INPUT_BUFFER_PADDING_SIZE here.)
> 
> Hi,
> 
> From get_bits.h (init_get_bits()): "buffer, must be AV_INPUT_BUFFER_PADDING_SIZE bytes larger than the actual read bits". Is this wrong? If so, what is the correct value and is it defined anywhere? What about "other places where one needed a padded buffer" as mentioned in your previous comment?

It is overtly pessimistic. In fact, AV_INPUT_BUFFER_PADDING_SIZE has
been bumped several times with no changes to the GetBit API.
The correct value is not defined anywhere; I'd need to take a closer
look at it to find it out.

> 
> Why is copying the buffer to the stack in the parsing function preferable to padding it when serializing? The vast majority of HDR10+ payloads will be well below the max size.
> 

If the parsing function does not require padding, then it can be used
with any buffer, not only those created by the serialization function.

- Andreas

On 3/13/23 20:44, Andreas Rheinhardt wrote:
> Looking at the calculation in av_dynamic_hdr_plus_to_t35(), it seems
> that the maximum bitlength of a valid ITU-T T.35 payload is
> 48+2×937+27+1+10+25×25×4+3×82+(3×15×24)+(1+10+25×25×4+3×1)+(3×(28+15×10))+3+3×6
> = 8855 bits (please double-check this). This means we can just copy that
> much into a padded buffer on the stack and ignore the padding. We may
> then remove the padding from the serialization function, too.

I went over the calculations again and found a couple of places where size was overestimated due to fields that can fit larger values than their specified maximum. 937 also seems like a typo on my part (unsure how that happened, maybe multiplied instead of added some number), it should be 153. I re-ran the numbers for everything and rewrote/expanded the size calculation code somewhat to make reading alongside the spec or block comment easier.

I also removed another 40 bits from the header to match the parsing function, which begins parsing at the application mode, rather than at the terminal provider code. That means we exclude 48 bits in total from the header, and the calculation should be as follows:

8 + 2 +
2 * 153 +
27 +
1 + 10 + 25 * 25 * 4 +
3 * (72 + 15 * 24 + 10) +
1 + 10 + 25 * 25 * 4 +
3 * (1 + 28 + 15 * 10) +
3 * (1 + 6) =

7249 bits maximum (907 bytes rounded up).

Can we correct the padding amount along with get_bits.h in a separate patch? I would like to avoid scope creep.

Will send a new version with the changes mentioned above. I've tested the round trip (HEVC decode -> parse -> serialize -> HEVC encode).

RZ