[FFmpeg-devel] Support HDR10+ metadata for HEVC.

From: Mohammad Izadi <moh.izadi@gmail.com>

HDR10+ is dynamic metadata (A/341 Amendment - SMPTE2094-40) that needs to be decoded from ITU-T T.35 in HEVC bitstream. The HDR10+ is transferred to side data packet to be used or passed through.
---
The fate test file can be found here: https://drive.google.com/file/d/1vcT0ohzpoyVFcxQN32jKIhUrBaBwZweT/view?usp=sharing
The video file needs to be copied to fate-suite/mov/
configure                              |   1 +
 fftools/ffprobe.c                      | 106 +++++++++++++
 libavcodec/Makefile                    |   3 +
 libavcodec/avpacket.c                  |   1 +
 libavcodec/decode.c                    |   1 +
 libavcodec/dynamic_hdr10_plus.c        | 196 +++++++++++++++++++++++++
 libavcodec/dynamic_hdr10_plus.h        |  34 +++++
 libavcodec/hevc_sei.c                  |  62 ++++++--
 libavcodec/hevc_sei.h                  |   5 +
 libavcodec/hevcdec.c                   |  18 +++
 libavcodec/packet.h                    |   8 +
 libavcodec/version.h                   |   2 +-
 libavfilter/vf_showinfo.c              | 106 ++++++++++++-
 tests/fate/mov.mak                     |   3 +
 tests/ref/fate/mov-hdr10-plus-metadata |  90 ++++++++++++
 15 files changed, 621 insertions(+), 15 deletions(-)
 create mode 100644 libavcodec/dynamic_hdr10_plus.c
 create mode 100644 libavcodec/dynamic_hdr10_plus.h
 create mode 100644 tests/ref/fate/mov-hdr10-plus-metadata

Ian, can you please take a look into it? And if it's fine to push it.

Thanks,
Mohammad


On Mon, Nov 23, 2020 at 1:29 PM Mohammad Izadi <izadi@google.com> wrote:

> From: Mohammad Izadi <moh.izadi@gmail.com>
>
> HDR10+ is dynamic metadata (A/341 Amendment - SMPTE2094-40) that needs to
> be decoded from ITU-T T.35 in HEVC bitstream. The HDR10+ is transferred to
> side data packet to be used or passed through.
> ---
> The fate test file can be found here:
> https://drive.google.com/file/d/1vcT0ohzpoyVFcxQN32jKIhUrBaBwZweT/view?usp=sharing
> The video file needs to be copied to fate-suite/mov/
> configure                              |   1 +
>  fftools/ffprobe.c                      | 106 +++++++++++++
>  libavcodec/Makefile                    |   3 +
>  libavcodec/avpacket.c                  |   1 +
>  libavcodec/decode.c                    |   1 +
>  libavcodec/dynamic_hdr10_plus.c        | 196 +++++++++++++++++++++++++
>  libavcodec/dynamic_hdr10_plus.h        |  34 +++++
>  libavcodec/hevc_sei.c                  |  62 ++++++--
>  libavcodec/hevc_sei.h                  |   5 +
>  libavcodec/hevcdec.c                   |  18 +++
>  libavcodec/packet.h                    |   8 +
>  libavcodec/version.h                   |   2 +-
>  libavfilter/vf_showinfo.c              | 106 ++++++++++++-
>  tests/fate/mov.mak                     |   3 +
>  tests/ref/fate/mov-hdr10-plus-metadata |  90 ++++++++++++
>  15 files changed, 621 insertions(+), 15 deletions(-)
>  create mode 100644 libavcodec/dynamic_hdr10_plus.c
>  create mode 100644 libavcodec/dynamic_hdr10_plus.h
>  create mode 100644 tests/ref/fate/mov-hdr10-plus-metadata
>
> diff --git a/configure b/configure
> index 51e43fbf66..a9f12a421e 100755
> --- a/configure
> +++ b/configure
> @@ -2360,6 +2360,7 @@ CONFIG_EXTRA="
>      dirac_parse
>      dnn
>      dvprofile
> +    dynamic_hdr10_plus
>      exif
>      faandct
>      faanidct
> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
> index 86bd23d36d..4cee4e8ec3 100644
> --- a/fftools/ffprobe.c
> +++ b/fftools/ffprobe.c
> @@ -35,6 +35,7 @@
>  #include "libavutil/bprint.h"
>  #include "libavutil/display.h"
>  #include "libavutil/hash.h"
> +#include "libavutil/hdr_dynamic_metadata.h"
>  #include "libavutil/mastering_display_metadata.h"
>  #include "libavutil/dovi_meta.h"
>  #include "libavutil/opt.h"
> @@ -1860,6 +1861,105 @@ static inline int show_tags(WriterContext *w,
> AVDictionary *tags, int section_id
>      return ret;
>  }
>
> +static void print_dynamic_hdr10_plus(WriterContext *w, AVDynamicHDRPlus
> *metadata)
> +{
> +    if (!metadata)
> +        return;
> +    print_int("application version", metadata->application_version);
> +    print_int("num_windows", metadata->num_windows);
> +    for (int n = 1; n < metadata->num_windows; n++) {
> +        AVHDRPlusColorTransformParams *params = &metadata->params[n];
> +        print_q("window_upper_left_corner_x",
> +                params->window_upper_left_corner_x,'/');
> +        print_q("window_upper_left_corner_y",
> +                params->window_upper_left_corner_y,'/');
> +        print_q("window_lower_right_corner_x",
> +                params->window_lower_right_corner_x,'/');
> +        print_q("window_lower_right_corner_y",
> +                params->window_lower_right_corner_y,'/');
> +        print_q("window_upper_left_corner_x",
> +                params->window_upper_left_corner_x,'/');
> +        print_q("window_upper_left_corner_y",
> +                params->window_upper_left_corner_y,'/');
> +        print_int("center_of_ellipse_x",
> +                  params->center_of_ellipse_x ) ;
> +        print_int("center_of_ellipse_y",
> +                  params->center_of_ellipse_y );
> +        print_int("rotation_angle",
> +                  params->rotation_angle);
> +        print_int("semimajor_axis_internal_ellipse",
> +                  params->semimajor_axis_internal_ellipse);
> +        print_int("semimajor_axis_external_ellipse",
> +                  params->semimajor_axis_external_ellipse);
> +        print_int("semiminor_axis_external_ellipse",
> +                  params->semiminor_axis_external_ellipse);
> +        print_int("overlap_process_option",
> +                  params->overlap_process_option);
> +    }
> +    print_q("targeted_system_display_maximum_luminance",
> +            metadata->targeted_system_display_maximum_luminance,'/');
> +    if (metadata->targeted_system_display_actual_peak_luminance_flag) {
> +
> print_int("num_rows_targeted_system_display_actual_peak_luminance",
> +
> metadata->num_rows_targeted_system_display_actual_peak_luminance);
> +
> print_int("num_cols_targeted_system_display_actual_peak_luminance",
> +
> metadata->num_cols_targeted_system_display_actual_peak_luminance);
> +        for (int i = 0; i <
> metadata->num_rows_targeted_system_display_actual_peak_luminance; i++) {
> +            for (int j = 0; j <
> metadata->num_cols_targeted_system_display_actual_peak_luminance; j++) {
> +                print_q("targeted_system_display_actual_peak_luminance",
> +
> metadata->targeted_system_display_actual_peak_luminance[i][j],'/');
> +            }
> +        }
> +    }
> +    for (int n = 0; n < metadata->num_windows; n++) {
> +        AVHDRPlusColorTransformParams *params = &metadata->params[n];
> +        for (int i = 0; i < 3; i++) {
> +            print_q("maxscl",params->maxscl[i],'/');
> +        }
> +        print_q("average_maxrgb",
> +                params->average_maxrgb,'/');
> +        print_int("num_distribution_maxrgb_percentiles",
> +                  params->num_distribution_maxrgb_percentiles);
> +        for (int i = 0; i < params->num_distribution_maxrgb_percentiles;
> i++) {
> +            print_int("distribution_maxrgb_percentage",
> +                      params->distribution_maxrgb[i].percentage);
> +            print_q("distribution_maxrgb_percentile",
> +                    params->distribution_maxrgb[i].percentile,'/');
> +        }
> +        print_q("fraction_bright_pixels",
> +                params->fraction_bright_pixels,'/');
> +    }
> +    if (metadata->mastering_display_actual_peak_luminance_flag) {
> +        print_int("num_rows_mastering_display_actual_peak_luminance",
> +
> metadata->num_rows_mastering_display_actual_peak_luminance);
> +        print_int("num_cols_mastering_display_actual_peak_luminance",
> +
> metadata->num_cols_mastering_display_actual_peak_luminance);
> +        for (int i = 0; i <
> metadata->num_rows_mastering_display_actual_peak_luminance; i++) {
> +            for (int j = 0; j <
> metadata->num_cols_mastering_display_actual_peak_luminance; j++) {
> +                print_q("mastering_display_actual_peak_luminance",
> +
> metadata->mastering_display_actual_peak_luminance[i][j],'/');
> +            }
> +        }
> +    }
> +
> +    for (int n = 0; n < metadata->num_windows; n++) {
> +        AVHDRPlusColorTransformParams *params = &metadata->params[n];
> +        if (params->tone_mapping_flag) {
> +            print_q("knee_point_x", params->knee_point_x,'/');
> +            print_q("knee_point_y", params->knee_point_y,'/');
> +            print_int("num_bezier_curve_anchors",
> +                      params->num_bezier_curve_anchors );
> +            for (int i = 0; i < params->num_bezier_curve_anchors; i++) {
> +                print_q("bezier_curve_anchors",
> +                        params->bezier_curve_anchors[i],'/');
> +            }
> +        }
> +        if (params->color_saturation_mapping_flag) {
> +            print_q("color_saturation_weight",
> +                    params->color_saturation_weight,'/');
> +        }
> +    }
> +}
> +
>  static void print_pkt_side_data(WriterContext *w,
>                                  AVCodecParameters *par,
>                                  const AVPacketSideData *side_data,
> @@ -1925,6 +2025,9 @@ static void print_pkt_side_data(WriterContext *w,
>                  print_q("min_luminance", metadata->min_luminance, '/');
>                  print_q("max_luminance", metadata->max_luminance, '/');
>              }
> +        } else if (sd->type == AV_PKT_DATA_DYNAMIC_HDR_PLUS) {
> +            AVDynamicHDRPlus *metadata = (AVDynamicHDRPlus *)sd->data;
> +            print_dynamic_hdr10_plus(w, metadata);
>          } else if (sd->type == AV_PKT_DATA_CONTENT_LIGHT_LEVEL) {
>              AVContentLightMetadata *metadata = (AVContentLightMetadata
> *)sd->data;
>              print_int("max_content", metadata->MaxCLL);
> @@ -2250,6 +2353,9 @@ static void show_frame(WriterContext *w, AVFrame
> *frame, AVStream *stream,
>                      print_q("min_luminance", metadata->min_luminance,
> '/');
>                      print_q("max_luminance", metadata->max_luminance,
> '/');
>                  }
> +            } else if (sd->type == AV_FRAME_DATA_DYNAMIC_HDR_PLUS) {
> +                AVDynamicHDRPlus *metadata = (AVDynamicHDRPlus *)sd->data;
> +                print_dynamic_hdr10_plus(w, metadata);
>              } else if (sd->type == AV_FRAME_DATA_CONTENT_LIGHT_LEVEL) {
>                  AVContentLightMetadata *metadata =
> (AVContentLightMetadata *)sd->data;
>                  print_int("max_content", metadata->MaxCLL);
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index b4777be4d4..57326c2bd6 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -15,6 +15,7 @@ HEADERS = ac3_parser.h
>                 \
>            dirac.h                                                       \
>            dv_profile.h                                                  \
>            dxva2.h                                                       \
> +          dynamic_hdr10_plus.h                                          \
>            jni.h                                                         \
>            mediacodec.h                                                  \
>            packet.h                                                      \
> @@ -43,6 +44,7 @@ OBJS = ac3_parser.o
>                \
>         dv_profile.o                                                     \
>         encode.o                                                         \
>         imgconvert.o                                                     \
> +       dynamic_hdr10_plus.o                                             \
>         jni.o                                                            \
>         mathtables.o                                                     \
>         mediacodec.o                                                     \
> @@ -78,6 +80,7 @@ OBJS-$(CONFIG_CBS_MPEG2)               += cbs_mpeg2.o
>  OBJS-$(CONFIG_CBS_VP9)                 += cbs_vp9.o
>  OBJS-$(CONFIG_CRYSTALHD)               += crystalhd.o
>  OBJS-$(CONFIG_DCT)                     += dct.o dct32_fixed.o
> dct32_float.o
> +OBJS-$(CONFIG_DYNAMIC_HDR10_PLUS)      += dynamic_hdr10_plus.o
>  OBJS-$(CONFIG_ERROR_RESILIENCE)        += error_resilience.o
>  OBJS-$(CONFIG_EXIF)                    += exif.o tiff_common.o
>  OBJS-$(CONFIG_FAANDCT)                 += faandct.o
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index e4ba403cf6..b18a7b0823 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -394,6 +394,7 @@ const char *av_packet_side_data_name(enum
> AVPacketSideDataType type)
>      case AV_PKT_DATA_CONTENT_LIGHT_LEVEL:        return "Content light
> level metadata";
>      case AV_PKT_DATA_SPHERICAL:                  return "Spherical
> Mapping";
>      case AV_PKT_DATA_A53_CC:                     return "A53 Closed
> Captions";
> +    case AV_PKT_DATA_DYNAMIC_HDR_PLUS:           return "HDR10+ Dynamic
> Metadata (SMPTE 2094-40)";
>      case AV_PKT_DATA_ENCRYPTION_INIT_INFO:       return "Encryption
> initialization data";
>      case AV_PKT_DATA_ENCRYPTION_INFO:            return "Encryption info";
>      case AV_PKT_DATA_AFD:                        return "Active Format
> Description data";
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index 5a1849f944..6da22fa2b9 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -1717,6 +1717,7 @@ int ff_decode_frame_props(AVCodecContext *avctx,
> AVFrame *frame)
>          { AV_PKT_DATA_MASTERING_DISPLAY_METADATA,
> AV_FRAME_DATA_MASTERING_DISPLAY_METADATA },
>          { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,
> AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
>          { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
> +        { AV_PKT_DATA_DYNAMIC_HDR_PLUS,
>  AV_FRAME_DATA_DYNAMIC_HDR_PLUS },
>          { AV_PKT_DATA_ICC_PROFILE,
> AV_FRAME_DATA_ICC_PROFILE },
>          { AV_PKT_DATA_S12M_TIMECODE,
> AV_FRAME_DATA_S12M_TIMECODE },
>      };
> diff --git a/libavcodec/dynamic_hdr10_plus.c
> b/libavcodec/dynamic_hdr10_plus.c
> new file mode 100644
> index 0000000000..ceee66528d
> --- /dev/null
> +++ b/libavcodec/dynamic_hdr10_plus.c
> @@ -0,0 +1,196 @@
> +/*
> + * 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"
> +
> +static const uint8_t usa_country_code = 0xB5;
> +static const uint16_t smpte_provider_code = 0x003C;
> +static const uint16_t smpte2094_40_provider_oriented_code = 0x0001;
> +static const uint16_t smpte2094_40_application_identifier = 0x04;
> +static const int64_t luminance_den = 1;
> +static const int32_t peak_luminance_den = 15;
> +static const int64_t rgb_den = 100000;
> +static const int32_t fraction_pixel_den = 1000;
> +static const int32_t knee_point_den = 4095;
> +static const int32_t bezier_anchor_den = 1023;
> +static const int32_t saturation_weight_den = 8;
> +
> +int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(GetBitContext *gb,
> AVDynamicHDRPlus *s)
> +{
> +    uint8_t application_version  = get_bits(gb, 8);
> +    if (!s)
> +        return AVERROR(ENOMEM);
> +
> +    s->application_version = application_version;
> +
> +    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 (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(EINVAL);
> +
> +    s->targeted_system_display_maximum_luminance =
> +        (AVRational){get_bits(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(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 (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(EINVAL);
> +
> +        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(EINVAL);
> +
> +        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(EINVAL);
> +
> +        params->fraction_bright_pixels = (AVRational){get_bits(gb, 10),
> 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 (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(EINVAL);
> +
> +        params->tone_mapping_flag = get_bits1(gb);
> +        if (params->tone_mapping_flag) {
> +            if (get_bits_left(gb) < 28)
> +                return AVERROR(EINVAL);
> +
> +            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(EINVAL);
> +
> +            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(EINVAL);
> +        params->color_saturation_mapping_flag = get_bits1(gb);
> +        if (params->color_saturation_mapping_flag) {
> +            if (get_bits_left(gb) < 6)
> +                return AVERROR(EINVAL);
> +            params->color_saturation_weight =
> +                (AVRational){get_bits(gb, 6), saturation_weight_den};
> +        }
> +    }
> +
> +    skip_bits(gb, get_bits_left(gb));
> +
> +    return 0;
> +}
> diff --git a/libavcodec/dynamic_hdr10_plus.h
> b/libavcodec/dynamic_hdr10_plus.h
> new file mode 100644
> index 0000000000..2fb9c64edc
> --- /dev/null
> +++ b/libavcodec/dynamic_hdr10_plus.h
> @@ -0,0 +1,34 @@
> +/*
> + * 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 "get_bits.h"
> +#include "libavutil/hdr_dynamic_metadata.h"
> +
> +/**
> + * Parse the user data registered ITU-T T.35 to AVbuffer
> (AVDynamicHDRPlus).
> + * @param gb The bit content to be decoded.
> + * @param s A pointer containing the decoded AVDynamicHDRPlus structure.
> + *
> + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR.
> + */
> +int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(GetBitContext *gb,
> AVDynamicHDRPlus *s);
> +
> +#endif /* AVCODEC_DYNAMIC_HDR10_PLUS_H */
> diff --git a/libavcodec/hevc_sei.c b/libavcodec/hevc_sei.c
> index 06e7a3a10c..211f3ab802 100644
> --- a/libavcodec/hevc_sei.c
> +++ b/libavcodec/hevc_sei.c
> @@ -24,6 +24,7 @@
>
>  #include "atsc_a53.h"
>  #include "golomb.h"
> +#include "dynamic_hdr10_plus.h"
>  #include "hevc_ps.h"
>  #include "hevc_sei.h"
>
> @@ -206,11 +207,15 @@ static int
> decode_nal_sei_user_data_unregistered(HEVCSEIUnregistered *s, GetBitC
>      return 0;
>  }
>
> -static int decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s,
> GetBitContext *gb,
> +static int decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s,
> +                                                         GetBitContext
> *gb,
>                                                           int size)
>  {
> -    uint32_t country_code;
> -    uint32_t user_identifier;
> +    const uint8_t usa_country_code = 0xB5;
> +    const uint16_t smpte_provider_code = 0x003C;
> +
> +    uint8_t country_code = 0;
> +    uint16_t provider_code = 0;
>
>      if (size < 7)
>          return AVERROR(EINVAL);
> @@ -222,18 +227,48 @@ static int
> decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s, GetBitConte
>          size--;
>      }
>
> -    skip_bits(gb, 8);
> -    skip_bits(gb, 8);
> -
> -    user_identifier = get_bits_long(gb, 32);
> -
> -    switch (user_identifier) {
> -        case MKBETAG('G', 'A', '9', '4'):
> +    provider_code = get_bits(gb, 16);
> +
> +    if (country_code == usa_country_code &&
> +        provider_code == smpte_provider_code) {
> +        // A/341 Amendment – 2094-40
> +        const uint16_t smpte2094_40_provider_oriented_code = 0x0001;
> +        const uint8_t smpte2094_40_application_identifier = 0x04;
> +
> +        uint16_t provider_oriented_code = get_bits(gb, 16);
> +        uint8_t application_identifier = get_bits(gb, 8);
> +
> +        if (provider_oriented_code == smpte2094_40_provider_oriented_code
> &&
> +            application_identifier ==
> smpte2094_40_application_identifier) {
> +            int err = 0;
> +            size_t size = 0;
> +            AVDynamicHDRPlus *metadata = av_dynamic_hdr_plus_alloc(&size);
> +            if (!metadata)
> +                return AVERROR(ENOMEM);
> +
> +            err = ff_parse_itu_t_t35_to_dynamic_hdr10_plus(gb, metadata);
> +           if (err < 0) {
> +                av_freep(&metadata);
> +                return err;
> +           }
> +
> +            if (s->dynamic_hdr_plus.info)
> +                av_buffer_unref(&s->dynamic_hdr_plus.info);
> +
> +            s->dynamic_hdr_plus.info = av_buffer_create(
> +                (uint8_t *)metadata, size, av_buffer_default_free, NULL,
> 0);
> +            if (!s->dynamic_hdr_plus.info) {
> +                av_freep(&s);
> +                return AVERROR(ENOMEM);
> +            }
> +            return err;
> +        }
> +    } else {
> +        uint32_t  user_identifier = get_bits_long(gb, 32);
> +        if(user_identifier == MKBETAG('G', 'A', '9', '4'))
>              return
> decode_registered_user_data_closed_caption(&s->a53_caption, gb, size);
> -        default:
> -            skip_bits_long(gb, size * 8);
> -            break;
>      }
> +    skip_bits_long(gb, size * 8);
>      return 0;
>  }
>
> @@ -420,4 +455,5 @@ void ff_hevc_reset_sei(HEVCSEI *s)
>          av_buffer_unref(&s->unregistered.buf_ref[i]);
>      s->unregistered.nb_buf_ref = 0;
>      av_freep(&s->unregistered.buf_ref);
> +    av_buffer_unref(&s->dynamic_hdr_plus.info);
>  }
> diff --git a/libavcodec/hevc_sei.h b/libavcodec/hevc_sei.h
> index 5ee7a4796d..e9e2d46ed4 100644
> --- a/libavcodec/hevc_sei.h
> +++ b/libavcodec/hevc_sei.h
> @@ -104,6 +104,10 @@ typedef struct HEVCSEIMasteringDisplay {
>      uint32_t min_luminance;
>  } HEVCSEIMasteringDisplay;
>
> +typedef struct HEVCSEIDynamicHDRPlus {
> +    AVBufferRef *info;
> +} HEVCSEIDynamicHDRPlus;
> +
>  typedef struct HEVCSEIContentLight {
>      int present;
>      uint16_t max_content_light_level;
> @@ -143,6 +147,7 @@ typedef struct HEVCSEI {
>      HEVCSEIA53Caption a53_caption;
>      HEVCSEIUnregistered unregistered;
>      HEVCSEIMasteringDisplay mastering_display;
> +    HEVCSEIDynamicHDRPlus dynamic_hdr_plus;
>      HEVCSEIContentLight content_light;
>      int active_seq_parameter_set_id;
>      HEVCSEIAlternativeTransfer alternative_transfer;
> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> index 699c13bbcc..e98782b40e 100644
> --- a/libavcodec/hevcdec.c
> +++ b/libavcodec/hevcdec.c
> @@ -2867,6 +2867,17 @@ static int set_side_data(HEVCContext *s)
>          s->sei.timecode.num_clock_ts = 0;
>      }
>
> +    if (s->sei.dynamic_hdr_plus.info){
> +        AVBufferRef *info_ref = av_buffer_ref(s->
> sei.dynamic_hdr_plus.info);
> +        if (!info_ref)
> +            return AVERROR(ENOMEM);
> +
> +        if(!av_frame_new_side_data_from_buf(out,
> AV_FRAME_DATA_DYNAMIC_HDR_PLUS, info_ref)){
> +            av_buffer_unref(&info_ref);
> +            return AVERROR(ENOMEM);
> +        }
> +    }
> +
>      return 0;
>  }
>
> @@ -3560,6 +3571,13 @@ static int
> hevc_update_thread_context(AVCodecContext *dst,
>          }
>      }
>
> +    av_buffer_unref(&s->sei.dynamic_hdr_plus.info);
> +    if (s0->sei.dynamic_hdr_plus.info) {
> +        s->sei.dynamic_hdr_plus.info = av_buffer_ref(s0->
> sei.dynamic_hdr_plus.info);
> +        if (!s->sei.dynamic_hdr_plus.info)
> +            return AVERROR(ENOMEM);
> +    }
> +
>      s->sei.frame_packing        = s0->sei.frame_packing;
>      s->sei.display_orientation  = s0->sei.display_orientation;
>      s->sei.mastering_display    = s0->sei.mastering_display;
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index b9d4c9c2c8..69316096f1 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -290,6 +290,14 @@ enum AVPacketSideDataType {
>       */
>      AV_PKT_DATA_S12M_TIMECODE,
>
> +    /**
> +     * HDR10+ dynamic metadata associated with a video frame. The
> metadata is in
> +     * the form of the AVDynamicHDRPlus struct and contains
> +     * information for color volume transform - application 4 of
> +     * SPMTE 2094-40:2016 standard.
> +     */
> +    AV_PKT_DATA_DYNAMIC_HDR_PLUS,
> +
>      /**
>       * The number of side data types.
>       * This is not part of the public API/ABI in the sense that it may
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 1585d95777..cdd09096a1 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -28,7 +28,7 @@
>  #include "libavutil/version.h"
>
>  #define LIBAVCODEC_VERSION_MAJOR  58
> -#define LIBAVCODEC_VERSION_MINOR 112
> +#define LIBAVCODEC_VERSION_MINOR 113
>  #define LIBAVCODEC_VERSION_MICRO 103
>
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c
> index 66a30c7a32..f9ed54a70c 100644
> --- a/libavfilter/vf_showinfo.c
> +++ b/libavfilter/vf_showinfo.c
> @@ -24,6 +24,7 @@
>
>  #include <inttypes.h>
>
> +#include "libavcodec/dynamic_hdr10_plus.h"
>  #include "libavutil/bswap.h"
>  #include "libavutil/adler32.h"
>  #include "libavutil/display.h"
> @@ -178,7 +179,107 @@ static void dump_mastering_display(AVFilterContext
> *ctx, const AVFrameSideData *
>             av_q2d(mastering_display->min_luminance),
> av_q2d(mastering_display->max_luminance));
>  }
>
> -static void dump_content_light_metadata(AVFilterContext *ctx, const
> AVFrameSideData *sd)
> +static void dump_dynamic_hdr_plus(AVFilterContext *ctx, AVFrameSideData
> *sd)
> +{
> +    AVDynamicHDRPlus *hdr_plus;
> +
> +    av_log(ctx, AV_LOG_INFO, "HDR10+ metadata: ");
> +    if (sd->size < sizeof(*hdr_plus)) {
> +        av_log(ctx, AV_LOG_ERROR, "invalid data\n");
> +        return;
> +    }
> +
> +    hdr_plus = (AVDynamicHDRPlus *)sd->data;
> +    av_log(ctx, AV_LOG_INFO, "application version: %d, ",
> hdr_plus->application_version);
> +    av_log(ctx, AV_LOG_INFO, "num_windows: %d, ", hdr_plus->num_windows);
> +    for (int w = 1; w < hdr_plus->num_windows; w++) {
> +        AVHDRPlusColorTransformParams *params = &hdr_plus->params[w];
> +        av_log(ctx, AV_LOG_INFO, "window %d { ", w);
> +        av_log(ctx, AV_LOG_INFO, "window_upper_left_corner:
> (%5.4f,%5.4f),",
> +               av_q2d(params->window_upper_left_corner_x),
> +               av_q2d(params->window_upper_left_corner_y));
> +        av_log(ctx, AV_LOG_INFO, "window_lower_right_corner:
> (%5.4f,%5.4f), ",
> +               av_q2d(params->window_lower_right_corner_x),
> +               av_q2d(params->window_lower_right_corner_y));
> +        av_log(ctx, AV_LOG_INFO, "window_upper_left_corner: (%5.4f,
> %5.4f), ",
> +               av_q2d(params->window_upper_left_corner_x),
> +               av_q2d(params->window_upper_left_corner_y));
> +        av_log(ctx, AV_LOG_INFO, "center_of_ellipse_x: (%d,%d), ",
> +               params->center_of_ellipse_x,
> +               params->center_of_ellipse_y);
> +        av_log(ctx, AV_LOG_INFO, "rotation_angle: %d, ",
> +               params->rotation_angle);
> +        av_log(ctx, AV_LOG_INFO, "semimajor_axis_internal_ellipse: %d, ",
> +               params->semimajor_axis_internal_ellipse);
> +        av_log(ctx, AV_LOG_INFO, "semimajor_axis_external_ellipse: %d, ",
> +               params->semimajor_axis_external_ellipse);
> +        av_log(ctx, AV_LOG_INFO, "semiminor_axis_external_ellipse: %d, ",
> +               params->semiminor_axis_external_ellipse);
> +        av_log(ctx, AV_LOG_INFO, "overlap_process_option: %d}, ",
> +               params->overlap_process_option);
> +    }
> +    av_log(ctx, AV_LOG_INFO, "targeted_system_display_maximum_luminance:
> %9.4f, ",
> +           av_q2d(hdr_plus->targeted_system_display_maximum_luminance));
> +    if (hdr_plus->targeted_system_display_actual_peak_luminance_flag) {
> +        av_log(ctx, AV_LOG_INFO,
> "targeted_system_display_actual_peak_luminance: {");
> +        for (int i = 0; i <
> hdr_plus->num_rows_targeted_system_display_actual_peak_luminance; i++) {
> +            av_log(ctx, AV_LOG_INFO, "(");
> +            for (int j = 0; j <
> hdr_plus->num_cols_targeted_system_display_actual_peak_luminance; j++) {
> +                av_log(ctx, AV_LOG_INFO, "%5.4f,",
> +
>  av_q2d(hdr_plus->targeted_system_display_actual_peak_luminance[i][j]));
> +            }
> +            av_log(ctx, AV_LOG_INFO, ")");
> +        }
> +        av_log(ctx, AV_LOG_INFO, "}, ");
> +    }
> +
> +    for (int w = 0; w < hdr_plus->num_windows; w++) {
> +        AVHDRPlusColorTransformParams *params = &hdr_plus->params[w];
> +        av_log(ctx, AV_LOG_INFO, "window %d {maxscl: {", w);
> +        for (int i = 0; i < 3; i++) {
> +            av_log(ctx, AV_LOG_INFO, "%5.4f,",av_q2d(params->maxscl[i]));
> +        }
> +        av_log(ctx, AV_LOG_INFO, "} average_maxrgb: %5.4f, ",
> +               av_q2d(params->average_maxrgb));
> +        av_log(ctx, AV_LOG_INFO, "distribution_maxrgb: {");
> +        for (int i = 0; i < params->num_distribution_maxrgb_percentiles;
> i++) {
> +            av_log(ctx, AV_LOG_INFO, "(%d,%5.4f)",
> +                   params->distribution_maxrgb[i].percentage,
> +                   av_q2d(params->distribution_maxrgb[i].percentile));
> +        }
> +        av_log(ctx, AV_LOG_INFO, "} fraction_bright_pixels: %5.4f, ",
> +               av_q2d(params->fraction_bright_pixels));
> +        if (params->tone_mapping_flag) {
> +            av_log(ctx, AV_LOG_INFO, "knee_point: (%5.4f,%5.4f), ",
> av_q2d(params->knee_point_x), av_q2d(params->knee_point_y));
> +            av_log(ctx, AV_LOG_INFO, "bezier_curve_anchors: {");
> +            for (int i = 0; i < params->num_bezier_curve_anchors; i++) {
> +                av_log(ctx, AV_LOG_INFO, "%5.4f,",
> +                       av_q2d(params->bezier_curve_anchors[i]));
> +            }
> +            av_log(ctx, AV_LOG_INFO, "} ");
> +        }
> +        if (params->color_saturation_mapping_flag) {
> +            av_log(ctx, AV_LOG_INFO, "color_saturation_weight: %5.4f",
> +                   av_q2d(params->color_saturation_weight));
> +        }
> +        av_log(ctx, AV_LOG_INFO, "} ");
> +    }
> +
> +    if (hdr_plus->mastering_display_actual_peak_luminance_flag) {
> +        av_log(ctx, AV_LOG_INFO,
> "mastering_display_actual_peak_luminance: {");
> +        for (int i = 0; i <
> hdr_plus->num_rows_mastering_display_actual_peak_luminance; i++) {
> +            av_log(ctx, AV_LOG_INFO, "(");
> +            for (int j = 0; j <
> hdr_plus->num_cols_mastering_display_actual_peak_luminance; j++) {
> +                av_log(ctx, AV_LOG_INFO, " %5.4f,",
> +
>  av_q2d(hdr_plus->mastering_display_actual_peak_luminance[i][j]));
> +            }
> +            av_log(ctx, AV_LOG_INFO, ")");
> +        }
> +        av_log(ctx, AV_LOG_INFO, "} ");
> +    }
> +}
> +
> +static void dump_content_light_metadata(AVFilterContext *ctx,
> AVFrameSideData *sd)
>  {
>      const AVContentLightMetadata *metadata = (const
> AVContentLightMetadata *)sd->data;
>
> @@ -396,6 +497,9 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *frame)
>          case AV_FRAME_DATA_MASTERING_DISPLAY_METADATA:
>              dump_mastering_display(ctx, sd);
>              break;
> +        case AV_FRAME_DATA_DYNAMIC_HDR_PLUS:
> +            dump_dynamic_hdr_plus(ctx, sd);
> +            break;
>          case AV_FRAME_DATA_CONTENT_LIGHT_LEVEL:
>              dump_content_light_metadata(ctx, sd);
>              break;
> diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
> index 0fd20fef96..55228f02fd 100644
> --- a/tests/fate/mov.mak
> +++ b/tests/fate/mov.mak
> @@ -29,6 +29,7 @@ FATE_MOV_FFPROBE = fate-mov-neg-firstpts-discard \
>                     fate-mov-guess-delay-2 \
>                     fate-mov-guess-delay-3 \
>                     fate-mov-mp4-with-mov-in24-ver \
> +                   fate-mov-hdr10-plus-metadata \
>
>  FATE_MOV_FASTSTART = fate-mov-faststart-4gb-overflow \
>
> @@ -124,3 +125,5 @@ fate-mov-faststart-4gb-overflow: CMP = oneline
>  fate-mov-faststart-4gb-overflow: REF = bc875921f151871e787c4b4023269b29
>
>  fate-mov-mp4-with-mov-in24-ver: CMD = run ffprobe$(PROGSSUF)$(EXESUF)
> -show_entries stream=codec_name -select_streams 1
> $(TARGET_SAMPLES)/mov/mp4-with-mov-in24-ver.mp4
> +
> +fate-mov-hdr10-plus-metadata: CMD = run ffprobe$(PROGSSUF)$(EXESUF)
> -show_frames -read_intervals 0%+0.01 -select_streams v -v 0
> $(TARGET_SAMPLES)/mov/hdr10_plus_h265_sample.mp4
> \ No newline at end of file
> diff --git a/tests/ref/fate/mov-hdr10-plus-metadata
> b/tests/ref/fate/mov-hdr10-plus-metadata
> new file mode 100644
> index 0000000000..9f8f5e031b
> --- /dev/null
> +++ b/tests/ref/fate/mov-hdr10-plus-metadata
> @@ -0,0 +1,90 @@
> +[FRAME]
> +media_type=video
> +stream_index=0
> +key_frame=1
> +pkt_pts=0
> +pkt_pts_time=0.000000
> +pkt_dts=0
> +pkt_dts_time=0.000000
> +best_effort_timestamp=0
> +best_effort_timestamp_time=0.000000
> +pkt_duration=3540
> +pkt_duration_time=0.039333
> +pkt_pos=44
> +pkt_size=77634
> +width=1280
> +height=720
> +pix_fmt=yuv420p10le
> +sample_aspect_ratio=1:1
> +pict_type=I
> +coded_picture_number=0
> +display_picture_number=0
> +interlaced_frame=0
> +top_field_first=0
> +repeat_pict=0
> +color_range=tv
> +color_space=bt2020nc
> +color_primaries=bt2020
> +color_transfer=smpte2084
> +chroma_location=left
> +[SIDE_DATA]
> +side_data_type=Mastering display metadata
> +red_x=13250/50000
> +red_y=34500/50000
> +green_x=7500/50000
> +green_y=3000/50000
> +blue_x=34000/50000
> +blue_y=16000/50000
> +white_point_x=15635/50000
> +white_point_y=16450/50000
> +min_luminance=50/10000
> +max_luminance=10000000/10000
> +[/SIDE_DATA]
> +[SIDE_DATA]
> +side_data_type=Content light level metadata
> +max_content=1000
> +max_average=200
> +[/SIDE_DATA]
> +[SIDE_DATA]
> +side_data_type=HDR Dynamic Metadata SMPTE2094-40 (HDR10+)
> +application version=1
> +num_windows=1
> +targeted_system_display_maximum_luminance=400/1
> +maxscl=3340/100000
> +maxscl=2870/100000
> +maxscl=2720/100000
> +average_maxrgb=510/100000
> +num_distribution_maxrgb_percentiles=9
> +distribution_maxrgb_percentage=1
> +distribution_maxrgb_percentile=30/100000
> +distribution_maxrgb_percentage=5
> +distribution_maxrgb_percentile=2940/100000
> +distribution_maxrgb_percentage=10
> +distribution_maxrgb_percentile=255/100000
> +distribution_maxrgb_percentage=25
> +distribution_maxrgb_percentile=70/100000
> +distribution_maxrgb_percentage=50
> +distribution_maxrgb_percentile=1340/100000
> +distribution_maxrgb_percentage=75
> +distribution_maxrgb_percentile=1600/100000
> +distribution_maxrgb_percentage=90
> +distribution_maxrgb_percentile=1850/100000
> +distribution_maxrgb_percentage=95
> +distribution_maxrgb_percentile=1950/100000
> +distribution_maxrgb_percentage=99
> +distribution_maxrgb_percentile=2940/100000
> +fraction_bright_pixels=1/1000
> +knee_point_x=0/4095
> +knee_point_y=0/4095
> +num_bezier_curve_anchors=9
> +bezier_curve_anchors=102/1023
> +bezier_curve_anchors=205/1023
> +bezier_curve_anchors=307/1023
> +bezier_curve_anchors=410/1023
> +bezier_curve_anchors=512/1023
> +bezier_curve_anchors=614/1023
> +bezier_curve_anchors=717/1023
> +bezier_curve_anchors=819/1023
> +bezier_curve_anchors=922/1023
> +[/SIDE_DATA]
> +[/FRAME]
> \ No newline at end of file
> --
> 2.29.2.454.gaff20da3a2-goog
>
>

On 12/1/2020 3:09 PM, Mohammad Izadi wrote:
> Ian, can you please take a look into it? And if it's fine to push it.
> 
> Thanks,
> Mohammad

I removed the packet side data addition since it was not used by any 
module. It can be added when a demuxer needs to propagate it.

Also removed the entry for dynamic_hdr10_plus.h in the installed 
libraries list as it's internal, simplified the test (cut the sample 
into a single frame, 70kb vs 1mb sample) and split the patch in four 
commits before pushing.

Thanks.

On Sat, Dec 5, 2020 at 11:28 PM James Almer <jamrial@gmail.com> wrote:
>
> On 12/1/2020 3:09 PM, Mohammad Izadi wrote:
> > Ian, can you please take a look into it? And if it's fine to push it.
> >
> > Thanks,
> > Mohammad
>
> I removed the packet side data addition since it was not used by any
> module. It can be added when a demuxer needs to propagate it.
>
> Also removed the entry for dynamic_hdr10_plus.h in the installed
> libraries list as it's internal, simplified the test (cut the sample
> into a single frame, 70kb vs 1mb sample) and split the patch in four
> commits before pushing.
>

I looked through the committed patch as I was curious how it compares
to the code i've been using for a bit, and I noticed some concerns
with the GetBitContext handling.

- All SEI parsing uses one shared "gb", not only for one SEI message,
but the entire SEI NALU (this is fine, but important to know for the
other points)
- ff_parse_itu_t_t35_to_dynamic_hdr10_plus will always consume the
*full* GetBitContext with a "skip_bits(gb, get_bits_left(gb));" at its
end, so it may skip over a SEI message following the HDR10+ data?
- The skip in decode_nal_sei_user_data_registered_itu_t_t35 was moved
out of any conditions, so in case of decoding HDR10+ data, it would
skip data despite it already being consumed by parsing?

It seems like the intention of the code was to pass a copy of the
GetBitContex (and not the original) to
ff_parse_itu_t_t35_to_dynamic_hdr10_plus, as it would avoid any of
these issues, but then I don't know what the skip at the end of the
hdr10+ function is for?

- Hendrik

On 12/6/2020 8:13 AM, Hendrik Leppkes wrote:
> On Sat, Dec 5, 2020 at 11:28 PM James Almer <jamrial@gmail.com> wrote:
>>
>> On 12/1/2020 3:09 PM, Mohammad Izadi wrote:
>>> Ian, can you please take a look into it? And if it's fine to push it.
>>>
>>> Thanks,
>>> Mohammad
>>
>> I removed the packet side data addition since it was not used by any
>> module. It can be added when a demuxer needs to propagate it.
>>
>> Also removed the entry for dynamic_hdr10_plus.h in the installed
>> libraries list as it's internal, simplified the test (cut the sample
>> into a single frame, 70kb vs 1mb sample) and split the patch in four
>> commits before pushing.
>>
> 
> I looked through the committed patch as I was curious how it compares
> to the code i've been using for a bit, and I noticed some concerns
> with the GetBitContext handling.
> 
> - All SEI parsing uses one shared "gb", not only for one SEI message,
> but the entire SEI NALU (this is fine, but important to know for the
> other points)

Apparently, h264 creates standalone GetBitContexts for each SEI message, 
whereas hevc doesn't. This was probably written with the former in mind.

> - ff_parse_itu_t_t35_to_dynamic_hdr10_plus will always consume the
> *full* GetBitContext with a "skip_bits(gb, get_bits_left(gb));" at its
> end, so it may skip over a SEI message following the HDR10+ data?
> - The skip in decode_nal_sei_user_data_registered_itu_t_t35 was moved
> out of any conditions, so in case of decoding HDR10+ data, it would
> skip data despite it already being consumed by parsing?
> It seems like the intention of the code was to pass a copy of the
> GetBitContex (and not the original) to
> ff_parse_itu_t_t35_to_dynamic_hdr10_plus, as it would avoid any of
> these issues, but then I don't know what the skip at the end of the
> hdr10+ function is for?

It may be better to make ff_parse_itu_t_t35_to_dynamic_hdr10_plus() 
behave like ff_parse_a53_cc(), taking a buffer as argument and creating 
a GetBitContext internally instead. I'll send a patch for that.