[FFmpeg-devel] [PATCH v7 1/2] avcodec/avutil: move dynamic HDR10+ metadata parsing to libavutil
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Thu Mar 16 22:50:15 EET 2023
Raphaël Zumer:
>
>
>> On Mar 16, 2023, at 16:19, Andreas Rheinhardt <andreas.rheinhardt at outlook.com> wrote:
>>
>> Raphaël Zumer:
>>> Signed-off-by: Raphaël Zumer <rzumer at 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 | 185 +++++++++++++++++++++++++++++
>>> libavutil/hdr_dynamic_metadata.h | 13 ++
>>> 8 files changed, 210 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..5ed903f475 100644
>>> --- a/libavutil/hdr_dynamic_metadata.c
>>> +++ b/libavutil/hdr_dynamic_metadata.c
>>> @@ -20,6 +20,18 @@
>>>
>>> #include "hdr_dynamic_metadata.h"
>>> #include "mem.h"
>>> +#include "libavcodec/get_bits.h"
>>> +#include "libavcodec/put_bits.h"
>>> +
>>> +#define T35_PAYLOAD_MAX_SIZE 907
>>> +
>>> +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 +57,176 @@ 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)
>>> +{
>>> + uint8_t padded_buf[T35_PAYLOAD_MAX_SIZE + AV_INPUT_BUFFER_PADDING_SIZE];
>>> + GetBitContext gbc, *gb = &gbc;
>>> + int ret;
>>> +
>>> + if (!s)
>>> + return AVERROR(ENOMEM);
>>> +
>>> + memcpy(padded_buf, data, size);
>>
>> You are potentially copying more than T35_PAYLOAD_MAX_SIZE bytes.
>> Furthermore, you are not zeroing the padding; this might make Valgrind
>> complain.
>
> Good point on the size if the user sends an incorrect value, we can return AVERROR(EINVAL) in that case. I did run it through Valgrind and did not get an error due to not zeroing the padded buffer. I don’t think that should be necessary since it’s contained within that function and uninitialized bits should never be accessed, is that incorrect?
>
The GetBit API can overread a bit. It mostly reads 32bits at a time, so
that it can read into the padding even when no bit of the padding is
actually used (because it is masked away); but Valgrind does not always
get this.
- Andreas
More information about the ffmpeg-devel
mailing list