diff mbox series

[FFmpeg-devel] Support HDR10+ metadata for HEVC

Message ID 20200716192347.179352-1-izadi@google.com
State New
Headers show
Series [FFmpeg-devel] Support HDR10+ metadata for HEVC
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Mohammad Izadi July 16, 2020, 7:23 p.m. UTC
From: Mohammad Izadi <moh.izadi@gmail.com>

---
 libavcodec/avpacket.c |   1 +
 libavcodec/decode.c   |   1 +
 libavcodec/hevc_sei.c |  40 +++++++---
 libavcodec/hevc_sei.h |   5 ++
 libavcodec/hevcdec.c  |   7 ++
 libavcodec/internal.h |   9 +++
 libavcodec/packet.h   |   9 +++
 libavcodec/utils.c    | 180 ++++++++++++++++++++++++++++++++++++++++++
 libavcodec/version.h  |   2 +-
 9 files changed, 241 insertions(+), 13 deletions(-)

Comments

Carl Eugen Hoyos July 16, 2020, 7:29 p.m. UTC | #1
Am Do., 16. Juli 2020 um 21:24 Uhr schrieb Mohammad Izadi
<izadi-at-google.com@ffmpeg.org>:

> -    user_identifier = get_bits_long(gb, 32);
> -
> -    switch (user_identifier) {
> -        case MKBETAG('G', 'A', '9', '4'):

Why did you have to change this existing code?

Could you elaborate a little on the use-cases this patch supports (and
does not support) in the commit message?

A micro version bump should be sufficient.

Carl Eugen
James Almer July 16, 2020, 9:34 p.m. UTC | #2
On 7/16/2020 4:23 PM, Mohammad Izadi wrote:
> From: Mohammad Izadi <moh.izadi@gmail.com>
> 
> ---
>  libavcodec/avpacket.c |   1 +
>  libavcodec/decode.c   |   1 +
>  libavcodec/hevc_sei.c |  40 +++++++---
>  libavcodec/hevc_sei.h |   5 ++
>  libavcodec/hevcdec.c  |   7 ++
>  libavcodec/internal.h |   9 +++
>  libavcodec/packet.h   |   9 +++
>  libavcodec/utils.c    | 180 ++++++++++++++++++++++++++++++++++++++++++
>  libavcodec/version.h  |   2 +-
>  9 files changed, 241 insertions(+), 13 deletions(-)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index dce26cb31a..8307032335 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -394,6 +394,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
>      case AV_PKT_DATA_CONTENT_LIGHT_LEVEL:        return "Content light level metadata";
>      case AV_PKT_DATA_SPHERICAL:                  return "Spherical Mapping";
>      case AV_PKT_DATA_A53_CC:                     return "A53 Closed Captions";
> +    case AV_PKT_DATA_DYNAMIC_HDR_PLUS:           return "HDR10+ Dynamic Metadata (SMPTE 2094-40)";
>      case AV_PKT_DATA_ENCRYPTION_INIT_INFO:       return "Encryption initialization data";
>      case AV_PKT_DATA_ENCRYPTION_INFO:            return "Encryption info";
>      case AV_PKT_DATA_AFD:                        return "Active Format Description data";
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index de9c079f9d..cd3286f7fb 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -1698,6 +1698,7 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
>          { AV_PKT_DATA_MASTERING_DISPLAY_METADATA, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA },
>          { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,        AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
>          { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
> +        { AV_PKT_DATA_DYNAMIC_HDR_PLUS,           AV_FRAME_DATA_DYNAMIC_HDR_PLUS },
>          { AV_PKT_DATA_ICC_PROFILE,                AV_FRAME_DATA_ICC_PROFILE },
>      };
>  
> diff --git a/libavcodec/hevc_sei.c b/libavcodec/hevc_sei.c
> index a4ec65dc1a..096c414d91 100644
> --- a/libavcodec/hevc_sei.c
> +++ b/libavcodec/hevc_sei.c
> @@ -25,6 +25,12 @@
>  #include "golomb.h"
>  #include "hevc_ps.h"
>  #include "hevc_sei.h"
> +#include "internal.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;

No global state, please. You can just use the values in question and add
a comment next to them to describe their meaning, like it's done for a
lot other values in this file.

>  
>  static int decode_nal_sei_decoded_picture_hash(HEVCSEIPictureHash *s, GetBitContext *gb)
>  {
> @@ -242,8 +248,8 @@ static int decode_nal_sei_user_data_unregistered(HEVCSEIUnregistered *s, GetBitC
>  static int decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s, GetBitContext *gb,
>                                                           int size)
>  {
> -    uint32_t country_code;
> -    uint32_t user_identifier;
> +    uint8_t country_code;
> +    uint16_t provider_code;
>  
>      if (size < 7)
>          return AVERROR(EINVAL);
> @@ -255,18 +261,27 @@ 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
> +        uint16_t provider_oriented_code = get_bits(gb, 16);
> +        uint8_t application_identifier = get_bits(gb, 8);
> +        if (provider_oriented_code == smpte2094_40_provider_oriented_code &&
> +            application_identifier == smpte2094_40_application_identifier) {
> +            int err = ff_read_itu_t_t35_to_dynamic_hdr_plus(gb, s->dynamic_hdr_plus.info);
> +            if (err < 0 && s->dynamic_hdr_plus.info) {
> +                av_buffer_unref(&s->dynamic_hdr_plus.info);
> +            }
> +            return err;
> +        }
> +    } else {
> +        uint32_t  user_identifier = get_bits_long(gb, 32);
> +        if(user_identifier == MKBETAG('G', 'A', '9', '4'))
>              return decode_registered_user_data_closed_caption(&s->a53_caption, gb, size);
> -        default:
> -            skip_bits_long(gb, size * 8);
> -            break;
>      }
> +    skip_bits_long(gb, size * 8);
>      return 0;
>  }
>  
> @@ -453,4 +468,5 @@ void ff_hevc_reset_sei(HEVCSEI *s)
>          av_buffer_unref(&s->unregistered.buf_ref[i]);
>      s->unregistered.nb_buf_ref = 0;
>      av_freep(&s->unregistered.buf_ref);
> +    av_buffer_unref(&s->dynamic_hdr_plus.info);
>  }
> diff --git a/libavcodec/hevc_sei.h b/libavcodec/hevc_sei.h
> index 5ee7a4796d..e9e2d46ed4 100644
> --- a/libavcodec/hevc_sei.h
> +++ b/libavcodec/hevc_sei.h
> @@ -104,6 +104,10 @@ typedef struct HEVCSEIMasteringDisplay {
>      uint32_t min_luminance;
>  } HEVCSEIMasteringDisplay;
>  
> +typedef struct HEVCSEIDynamicHDRPlus {
> +    AVBufferRef *info;
> +} HEVCSEIDynamicHDRPlus;
> +
>  typedef struct HEVCSEIContentLight {
>      int present;
>      uint16_t max_content_light_level;
> @@ -143,6 +147,7 @@ typedef struct HEVCSEI {
>      HEVCSEIA53Caption a53_caption;
>      HEVCSEIUnregistered unregistered;
>      HEVCSEIMasteringDisplay mastering_display;
> +    HEVCSEIDynamicHDRPlus dynamic_hdr_plus;
>      HEVCSEIContentLight content_light;
>      int active_seq_parameter_set_id;
>      HEVCSEIAlternativeTransfer alternative_transfer;
> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> index b77df8d89f..748233fa32 100644
> --- a/libavcodec/hevcdec.c
> +++ b/libavcodec/hevcdec.c
> @@ -2849,6 +2849,13 @@ 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);
> +
> +        av_frame_new_side_data_from_buf(out, AV_FRAME_DATA_DYNAMIC_HDR_PLUS, info_ref);

Unchecked call.

Also, you should keep this metadata in sync between threads. See
hevc_update_thread_context().

> +    }
>  
>      return 0;
>  }
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index 0a1c0a17ec..744ace534c 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -413,6 +413,15 @@ int ff_int_from_list_or_default(void *ctx, const char * val_name, int val,
>  
>  void ff_dvdsub_parse_palette(uint32_t *palette, const char *p);
>  
> +/**
> + * Reads and decode the user data registered ITU-T T.35 to AVbuffer (AVDynamicHDRPlus).
> + * @param gbc The bit content to be decoded.
> + * @param output A buffer containing the decoded AVDynamicHDRPlus structure.
> + *
> + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR.
> + */
> +int ff_read_itu_t_t35_to_dynamic_hdr_plus(void *gbc, AVBufferRef *output);
> +
>  #if defined(_WIN32) && CONFIG_SHARED && !defined(BUILDING_avcodec)
>  #    define av_export_avcodec __declspec(dllimport)
>  #else
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index 96f237f091..695df15806 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -241,6 +241,15 @@ enum AVPacketSideDataType {
>       */
>      AV_PKT_DATA_A53_CC,
>  
> +
> +    /**
> +     * 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,

New enum values should be at the end, before the NB entry. Otherwise it
will be an ABI break.

> +
>      /**
>       * This side data is encryption initialization data.
>       * The format is not part of ABI, use av_encryption_init_info_* methods to
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 2ece34f921..b537909a3f 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -42,8 +42,10 @@
>  #include "libavutil/samplefmt.h"
>  #include "libavutil/dict.h"
>  #include "libavutil/thread.h"
> +#include "libavutil/hdr_dynamic_metadata.h"
>  #include "avcodec.h"
>  #include "decode.h"
> +#include "get_bits.h"
>  #include "hwconfig.h"
>  #include "libavutil/opt.h"
>  #include "mpegvideo.h"
> @@ -67,6 +69,17 @@
>  const char av_codec_ffversion[] = "FFmpeg version " FFMPEG_VERSION;
>  
>  static AVMutex codec_mutex = AV_MUTEX_INITIALIZER;
> +static const uint8_t usa_country_code = 0xB5;
> +static const uint16_t smpte_provider_code = 0x003C;
> +static const uint16_t smpte2094_40_provider_oriented_code = 0x0001;
> +static const uint16_t smpte2094_40_application_identifier = 0x04;
> +static const int64_t luminance_den = 1;
> +static const int32_t peak_luminance_den = 15;
> +static const int64_t rgb_den = 100000;
> +static const int32_t fraction_pixel_den = 1000;
> +static const int32_t knee_point_den = 4095;
> +static const int32_t bezier_anchor_den = 1023;
> +static const int32_t saturation_weight_den = 8;

Same, no global state.

>  
>  void av_fast_padded_malloc(void *ptr, unsigned int *size, size_t min_size)
>  {
> @@ -2346,3 +2359,170 @@ int ff_int_from_list_or_default(void *ctx, const char * val_name, int val,
>             "%s %d are not supported. Set to default value : %d\n", val_name, val, default_value);
>      return default_value;
>  }
> +
> +int ff_read_itu_t_t35_to_dynamic_hdr_plus(void *gbc,  AVBufferRef *output)

Unless this is used by other decoders, it should not be added to utils.c
Put it in hevc_sei.c

> +{
> +    GetBitContext *gb = (GetBitContext *)gbc;
> +    uint8_t application_version  = get_bits(gbc, 8);
> +    size_t size;
> +    int w, i, j;
> +    AVDynamicHDRPlus *s = av_dynamic_hdr_plus_alloc(&size);
> +    if (!s)
> +        return AVERROR(ENOMEM);
> +
> +    if (output)
> +        av_buffer_unref(&output);
> +
> +    output = av_buffer_create(
> +        (uint8_t *)s, size, av_buffer_default_free, NULL, 0);
> +    if (!output) {
> +        av_freep(&s);
> +        return AVERROR(ENOMEM);
> +    }
> +    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 (w = 1; w < s->num_windows; w++) {
> +        s->params[w].window_upper_left_corner_x.num = get_bits(gb, 16);
> +        s->params[w].window_upper_left_corner_y.num = get_bits(gb, 16);
> +        s->params[w].window_lower_right_corner_x.num = get_bits(gb, 16);
> +        s->params[w].window_lower_right_corner_y.num = get_bits(gb, 16);
> +        // The corners are set to absolute coordinates here. They should be
> +        // converted to the relative coordinates (in [0, 1]) in the decoder.
> +        s->params[w].window_upper_left_corner_x.den = 1;
> +        s->params[w].window_upper_left_corner_y.den = 1;
> +        s->params[w].window_lower_right_corner_x.den = 1;
> +        s->params[w].window_lower_right_corner_y.den = 1;
> +
> +        s->params[w].center_of_ellipse_x = get_bits(gb, 16);
> +        s->params[w].center_of_ellipse_y = get_bits(gb, 16);
> +        s->params[w].rotation_angle = get_bits(gb, 8);
> +        s->params[w].semimajor_axis_internal_ellipse = get_bits(gb, 16);
> +        s->params[w].semimajor_axis_external_ellipse = get_bits(gb, 16);
> +        s->params[w].semiminor_axis_external_ellipse = get_bits(gb, 16);
> +        s->params[w].overlap_process_option = get_bits1(gb);
> +    }
> +
> +    if (get_bits_left(gb) < 28)
> +        return AVERROR(EINVAL);
> +    s->targeted_system_display_maximum_luminance.num = get_bits(gb, 27);
> +    s->targeted_system_display_maximum_luminance.den = luminance_den;
> +    s->targeted_system_display_actual_peak_luminance_flag = get_bits1(gb);
> +
> +    if (s->targeted_system_display_actual_peak_luminance_flag) {
> +        int rows, cols;
> +        if (get_bits_left(gb) < 10)
> +            return AVERROR(EINVAL);
> +        rows = get_bits(gb, 5);
> +        cols = get_bits(gb, 5);
> +        if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25))) {
> +            return AVERROR_INVALIDDATA;
> +        }
> +        s->num_rows_targeted_system_display_actual_peak_luminance = rows;
> +        s->num_cols_targeted_system_display_actual_peak_luminance = cols;
> +
> +        if (get_bits_left(gb) < (rows * cols * 4))
> +            return AVERROR(EINVAL);
> +
> +        for (i = 0; i < rows; i++) {
> +            for (j = 0; j < cols; j++) {
> +                s->targeted_system_display_actual_peak_luminance[i][j].num = get_bits(gb, 4);
> +                s->targeted_system_display_actual_peak_luminance[i][j].den = peak_luminance_den;
> +            }
> +        }
> +    }
> +    for (w = 0; w < s->num_windows; w++) {
> +        if (get_bits_left(gb) < (3 * 17 + 17 + 4))
> +            return AVERROR(EINVAL);
> +        for (i = 0; i < 3; i++) {
> +            s->params[w].maxscl[i].num = get_bits(gb, 17);
> +            s->params[w].maxscl[i].den = rgb_den;
> +        }
> +        s->params[w].average_maxrgb.num = get_bits(gb, 17);
> +        s->params[w].average_maxrgb.den = rgb_den;
> +        s->params[w].num_distribution_maxrgb_percentiles = get_bits(gb, 4);
> +
> +        if (get_bits_left(gb) <
> +            (s->params[w].num_distribution_maxrgb_percentiles * 24))
> +            return AVERROR(EINVAL);
> +        for (i = 0; i < s->params[w].num_distribution_maxrgb_percentiles; i++) {
> +            s->params[w].distribution_maxrgb[i].percentage = get_bits(gb, 7);
> +            s->params[w].distribution_maxrgb[i].percentile.num = get_bits(gb, 17);
> +            s->params[w].distribution_maxrgb[i].percentile.den = rgb_den;
> +        }
> +
> +        if (get_bits_left(gb) < 10)
> +            return AVERROR(EINVAL);
> +        s->params[w].fraction_bright_pixels.num = get_bits(gb, 10);
> +        s->params[w].fraction_bright_pixels.den = fraction_pixel_den;
> +    }
> +    if (get_bits_left(gb) < 1)
> +        return AVERROR(EINVAL);
> +    s->mastering_display_actual_peak_luminance_flag = get_bits1(gb);
> +    if (s->mastering_display_actual_peak_luminance_flag) {
> +        int rows, cols;
> +        if (get_bits_left(gb) < 10)
> +            return AVERROR(EINVAL);
> +        rows = get_bits(gb, 5);
> +        cols = get_bits(gb, 5);
> +        if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25))) {
> +            return AVERROR_INVALIDDATA;
> +        }
> +        s->num_rows_mastering_display_actual_peak_luminance = rows;
> +        s->num_cols_mastering_display_actual_peak_luminance = cols;
> +
> +        if (get_bits_left(gb) < (rows * cols * 4))
> +            return AVERROR(EINVAL);
> +
> +        for (i = 0; i < rows; i++) {
> +            for (j = 0; j < cols; j++) {
> +                s->mastering_display_actual_peak_luminance[i][j].num = get_bits(gb, 4);
> +                s->mastering_display_actual_peak_luminance[i][j].den = peak_luminance_den;
> +            }
> +        }
> +    }
> +
> +    for (w = 0; w < s->num_windows; w++) {
> +        if (get_bits_left(gb) < 1)
> +            return AVERROR(EINVAL);
> +        s->params[w].tone_mapping_flag = get_bits1(gb);
> +        if (s->params[w].tone_mapping_flag) {
> +            if (get_bits_left(gb) < 28)
> +                return AVERROR(EINVAL);
> +            s->params[w].knee_point_x.num = get_bits(gb, 12);
> +            s->params[w].knee_point_x.den = knee_point_den;
> +            s->params[w].knee_point_y.num = get_bits(gb, 12);
> +            s->params[w].knee_point_y.den = knee_point_den;
> +            s->params[w].num_bezier_curve_anchors = get_bits(gb, 4);
> +
> +            if (get_bits_left(gb) < (s->params[w].num_bezier_curve_anchors * 10))
> +                return AVERROR(EINVAL);
> +            for (i = 0; i < s->params[w].num_bezier_curve_anchors; i++) {
> +                s->params[w].bezier_curve_anchors[i].num = get_bits(gb, 10);
> +                s->params[w].bezier_curve_anchors[i].den = bezier_anchor_den;
> +            }
> +        }
> +
> +        if (get_bits_left(gb) < 1)
> +            return AVERROR(EINVAL);
> +        s->params[w].color_saturation_mapping_flag = get_bits1(gb);
> +        if (s->params[w].color_saturation_mapping_flag) {
> +            if (get_bits_left(gb) < 6)
> +                return AVERROR(EINVAL);
> +            s->params[w].color_saturation_weight.num = get_bits(gb, 6);
> +            s->params[w].color_saturation_weight.den = saturation_weight_den;
> +        }
> +    }
> +
> +    skip_bits(gb, get_bits_left(gb));
> +
> +    return 0;
> +}
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index e75891d463..ad0bfd619d 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -28,7 +28,7 @@
>  #include "libavutil/version.h"
>  
>  #define LIBAVCODEC_VERSION_MAJOR  58
> -#define LIBAVCODEC_VERSION_MINOR  95
> +#define LIBAVCODEC_VERSION_MINOR  96
>  #define LIBAVCODEC_VERSION_MICRO 100
>  
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>
James Almer July 16, 2020, 9:38 p.m. UTC | #3
On 7/16/2020 4:29 PM, Carl Eugen Hoyos wrote:
> Am Do., 16. Juli 2020 um 21:24 Uhr schrieb Mohammad Izadi
> <izadi-at-google.com@ffmpeg.org>:
> 
>> -    user_identifier = get_bits_long(gb, 32);
>> -
>> -    switch (user_identifier) {
>> -        case MKBETAG('G', 'A', '9', '4'):
> 
> Why did you have to change this existing code?
> 
> Could you elaborate a little on the use-cases this patch supports (and
> does not support) in the commit message?
> 
> A micro version bump should be sufficient.

It adds a new AVPacketSideDataType enum entry, so minor is correct.

> 
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Steinar H. Gunderson July 16, 2020, 9:47 p.m. UTC | #4
On Thu, Jul 16, 2020 at 06:34:31PM -0300, James Almer wrote:
>>  static AVMutex codec_mutex = AV_MUTEX_INITIALIZER;
>> +static const uint8_t usa_country_code = 0xB5;
>> +static const uint16_t smpte_provider_code = 0x003C;
>> +static const uint16_t smpte2094_40_provider_oriented_code = 0x0001;
>> +static const uint16_t smpte2094_40_application_identifier = 0x04;
>> +static const int64_t luminance_den = 1;
>> +static const int32_t peak_luminance_den = 15;
>> +static const int64_t rgb_den = 100000;
>> +static const int32_t fraction_pixel_den = 1000;
>> +static const int32_t knee_point_den = 4095;
>> +static const int32_t bezier_anchor_den = 1023;
>> +static const int32_t saturation_weight_den = 8;
> Same, no global state.

It's not state if it can't be changed.

/* Steinar */
Mohammad Izadi July 23, 2020, 1:41 a.m. UTC | #5
Please see my answers inline:


On Thu, Jul 16, 2020 at 1:30 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Am Do., 16. Juli 2020 um 21:24 Uhr schrieb Mohammad Izadi
> <izadi-at-google.com@ffmpeg.org>:
>
> > -    user_identifier = get_bits_long(gb, 32);
> > -
> > -    switch (user_identifier) {
> > -        case MKBETAG('G', 'A', '9', '4'):
>
> Why did you have to change this existing code?
>
> We need to read the metadata (A/341 Amendment – 2094-40
<https://www.atsc.org/wp-content/uploads/2018/02/S34-301r2-A341-Amendment-2094-40.pdf>)
from the ITU-T T.35. Here is the point we need to read ITU-T T.35 and
decode it. I will update the commit message to explain it.

> Could you elaborate a little on the use-cases this patch supports (and
> does not support) in the commit message?
>
> Sure, I'll do.

> A micro version bump should be sufficient.
>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Zhao Zhili July 23, 2020, 8:13 a.m. UTC | #6
> On Jul 17, 2020, at 5:47 AM, Steinar H. Gunderson <steinar+ffmpeg@gunderson.no> wrote:
> 
> On Thu, Jul 16, 2020 at 06:34:31PM -0300, James Almer wrote:
>>> static AVMutex codec_mutex = AV_MUTEX_INITIALIZER;
>>> +static const uint8_t usa_country_code = 0xB5;
>>> +static const uint16_t smpte_provider_code = 0x003C;
>>> +static const uint16_t smpte2094_40_provider_oriented_code = 0x0001;
>>> +static const uint16_t smpte2094_40_application_identifier = 0x04;
>>> +static const int64_t luminance_den = 1;
>>> +static const int32_t peak_luminance_den = 15;
>>> +static const int64_t rgb_den = 100000;
>>> +static const int32_t fraction_pixel_den = 1000;
>>> +static const int32_t knee_point_den = 4095;
>>> +static const int32_t bezier_anchor_den = 1023;
>>> +static const int32_t saturation_weight_den = 8;
>> Same, no global state.
> 
> It's not state if it can't be changed.

If there is any chance to reuse these variables, I prefer current style than
use a literal with comments next to it.

> 
> /* Steinar */
> -- 
> Homepage: https://www.sesse.net/
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Timo Rothenpieler July 23, 2020, 10:12 a.m. UTC | #7
On 23/07/2020 10:13, zhilizhao wrote:
> 
> 
>> On Jul 17, 2020, at 5:47 AM, Steinar H. Gunderson <steinar+ffmpeg@gunderson.no> wrote:
>>
>> On Thu, Jul 16, 2020 at 06:34:31PM -0300, James Almer wrote:
>>>> static AVMutex codec_mutex = AV_MUTEX_INITIALIZER;
>>>> +static const uint8_t usa_country_code = 0xB5;
>>>> +static const uint16_t smpte_provider_code = 0x003C;
>>>> +static const uint16_t smpte2094_40_provider_oriented_code = 0x0001;
>>>> +static const uint16_t smpte2094_40_application_identifier = 0x04;
>>>> +static const int64_t luminance_den = 1;
>>>> +static const int32_t peak_luminance_den = 15;
>>>> +static const int64_t rgb_den = 100000;
>>>> +static const int32_t fraction_pixel_den = 1000;
>>>> +static const int32_t knee_point_den = 4095;
>>>> +static const int32_t bezier_anchor_den = 1023;
>>>> +static const int32_t saturation_weight_den = 8;
>>> Same, no global state.
>>
>> It's not state if it can't be changed.
> 
> If there is any chance to reuse these variables, I prefer current style than
> use a literal with comments next to it.

I guess ffmpeg more commonly uses #defines for constants like this, but 
it's still valid and not global state.
Mohammad Izadi July 23, 2020, 10:26 p.m. UTC | #8
Please see my answers inline:

On Thu, Jul 16, 2020 at 2:34 PM James Almer <jamrial@gmail.com> wrote:

> On 7/16/2020 4:23 PM, Mohammad Izadi wrote:
> > From: Mohammad Izadi <moh.izadi@gmail.com>
> >
> > ---
> >  libavcodec/avpacket.c |   1 +
> >  libavcodec/decode.c   |   1 +
> >  libavcodec/hevc_sei.c |  40 +++++++---
> >  libavcodec/hevc_sei.h |   5 ++
> >  libavcodec/hevcdec.c  |   7 ++
> >  libavcodec/internal.h |   9 +++
> >  libavcodec/packet.h   |   9 +++
> >  libavcodec/utils.c    | 180 ++++++++++++++++++++++++++++++++++++++++++
> >  libavcodec/version.h  |   2 +-
> >  9 files changed, 241 insertions(+), 13 deletions(-)
> >
> > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > index dce26cb31a..8307032335 <(830)%20703-2335> 100644
> > --- a/libavcodec/avpacket.c
> > +++ b/libavcodec/avpacket.c
> > @@ -394,6 +394,7 @@ const char *av_packet_side_data_name(enum
> AVPacketSideDataType type)
> >      case AV_PKT_DATA_CONTENT_LIGHT_LEVEL:        return "Content light
> level metadata";
> >      case AV_PKT_DATA_SPHERICAL:                  return "Spherical
> Mapping";
> >      case AV_PKT_DATA_A53_CC:                     return "A53 Closed
> Captions";
> > +    case AV_PKT_DATA_DYNAMIC_HDR_PLUS:           return "HDR10+ Dynamic
> Metadata (SMPTE 2094-40)";
> >      case AV_PKT_DATA_ENCRYPTION_INIT_INFO:       return "Encryption
> initialization data";
> >      case AV_PKT_DATA_ENCRYPTION_INFO:            return "Encryption
> info";
> >      case AV_PKT_DATA_AFD:                        return "Active Format
> Description data";
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index de9c079f9d..cd3286f7fb 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -1698,6 +1698,7 @@ int ff_decode_frame_props(AVCodecContext *avctx,
> AVFrame *frame)
> >          { AV_PKT_DATA_MASTERING_DISPLAY_METADATA,
> AV_FRAME_DATA_MASTERING_DISPLAY_METADATA },
> >          { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,
> AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
> >          { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC
> },
> > +        { AV_PKT_DATA_DYNAMIC_HDR_PLUS,
>  AV_FRAME_DATA_DYNAMIC_HDR_PLUS },
> >          { AV_PKT_DATA_ICC_PROFILE,
> AV_FRAME_DATA_ICC_PROFILE },
> >      };
> >
> > diff --git a/libavcodec/hevc_sei.c b/libavcodec/hevc_sei.c
> > index a4ec65dc1a..096c414d91 100644
> > --- a/libavcodec/hevc_sei.c
> > +++ b/libavcodec/hevc_sei.c
> > @@ -25,6 +25,12 @@
> >  #include "golomb.h"
> >  #include "hevc_ps.h"
> >  #include "hevc_sei.h"
> > +#include "internal.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;
>
> No global state, please. You can just use the values in question and add
> a comment next to them to describe their meaning, like it's done for a
> lot other values in this file.
>
> Done.

> >
> >  static int decode_nal_sei_decoded_picture_hash(HEVCSEIPictureHash *s,
> GetBitContext *gb)
> >  {
> > @@ -242,8 +248,8 @@ static int
> decode_nal_sei_user_data_unregistered(HEVCSEIUnregistered *s, GetBitC
> >  static int decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s,
> GetBitContext *gb,
> >                                                           int size)
> >  {
> > -    uint32_t country_code;
> > -    uint32_t user_identifier;
> > +    uint8_t country_code;
> > +    uint16_t provider_code;
> >
> >      if (size < 7)
> >          return AVERROR(EINVAL);
> > @@ -255,18 +261,27 @@ 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
> > +        uint16_t provider_oriented_code = get_bits(gb, 16);
> > +        uint8_t application_identifier = get_bits(gb, 8);
> > +        if (provider_oriented_code ==
> smpte2094_40_provider_oriented_code &&
> > +            application_identifier ==
> smpte2094_40_application_identifier) {
> > +            int err = ff_read_itu_t_t35_to_dynamic_hdr_plus(gb, s->
> dynamic_hdr_plus.info);
> > +            if (err < 0 && s->dynamic_hdr_plus.info) {
> > +                av_buffer_unref(&s->dynamic_hdr_plus.info);
> > +            }
> > +            return err;
> > +        }
> > +    } else {
> > +        uint32_t  user_identifier = get_bits_long(gb, 32);
> > +        if(user_identifier == MKBETAG('G', 'A', '9', '4'))
> >              return
> decode_registered_user_data_closed_caption(&s->a53_caption, gb, size);
> > -        default:
> > -            skip_bits_long(gb, size * 8);
> > -            break;
> >      }
> > +    skip_bits_long(gb, size * 8);
> >      return 0;
> >  }
> >
> > @@ -453,4 +468,5 @@ void ff_hevc_reset_sei(HEVCSEI *s)
> >          av_buffer_unref(&s->unregistered.buf_ref[i]);
> >      s->unregistered.nb_buf_ref = 0;
> >      av_freep(&s->unregistered.buf_ref);
> > +    av_buffer_unref(&s->dynamic_hdr_plus.info);
> >  }
> > diff --git a/libavcodec/hevc_sei.h b/libavcodec/hevc_sei.h
> > index 5ee7a4796d..e9e2d46ed4 100644
> > --- a/libavcodec/hevc_sei.h
> > +++ b/libavcodec/hevc_sei.h
> > @@ -104,6 +104,10 @@ typedef struct HEVCSEIMasteringDisplay {
> >      uint32_t min_luminance;
> >  } HEVCSEIMasteringDisplay;
> >
> > +typedef struct HEVCSEIDynamicHDRPlus {
> > +    AVBufferRef *info;
> > +} HEVCSEIDynamicHDRPlus;
> > +
> >  typedef struct HEVCSEIContentLight {
> >      int present;
> >      uint16_t max_content_light_level;
> > @@ -143,6 +147,7 @@ typedef struct HEVCSEI {
> >      HEVCSEIA53Caption a53_caption;
> >      HEVCSEIUnregistered unregistered;
> >      HEVCSEIMasteringDisplay mastering_display;
> > +    HEVCSEIDynamicHDRPlus dynamic_hdr_plus;
> >      HEVCSEIContentLight content_light;
> >      int active_seq_parameter_set_id;
> >      HEVCSEIAlternativeTransfer alternative_transfer;
> > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> > index b77df8d89f..748233fa32 100644
> > --- a/libavcodec/hevcdec.c
> > +++ b/libavcodec/hevcdec.c
> > @@ -2849,6 +2849,13 @@ 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);
> > +
> > +        av_frame_new_side_data_from_buf(out,
> AV_FRAME_DATA_DYNAMIC_HDR_PLUS, info_ref);
>
> Unchecked call.
>
> Done.

> Also, you should keep this metadata in sync between threads. See
> hevc_update_thread_context().
>
>  Done

> > +    }
> >
> >      return 0;
> >  }
> > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> > index 0a1c0a17ec..744ace534c 100644
> > --- a/libavcodec/internal.h
> > +++ b/libavcodec/internal.h
> > @@ -413,6 +413,15 @@ int ff_int_from_list_or_default(void *ctx, const
> char * val_name, int val,
> >
> >  void ff_dvdsub_parse_palette(uint32_t *palette, const char *p);
> >
> > +/**
> > + * Reads and decode the user data registered ITU-T T.35 to AVbuffer
> (AVDynamicHDRPlus).
> > + * @param gbc The bit content to be decoded.
> > + * @param output A buffer containing the decoded AVDynamicHDRPlus
> structure.
> > + *
> > + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR.
> > + */
> > +int ff_read_itu_t_t35_to_dynamic_hdr_plus(void *gbc, AVBufferRef
> *output);
> > +
> >  #if defined(_WIN32) && CONFIG_SHARED && !defined(BUILDING_avcodec)
> >  #    define av_export_avcodec __declspec(dllimport)
> >  #else
> > diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> > index 96f237f091..695df15806 100644
> > --- a/libavcodec/packet.h
> > +++ b/libavcodec/packet.h
> > @@ -241,6 +241,15 @@ enum AVPacketSideDataType {
> >       */
> >      AV_PKT_DATA_A53_CC,
> >
> > +
> > +    /**
> > +     * 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,
>
> New enum values should be at the end, before the NB entry. Otherwise it
> will be an ABI break.
>
Done.

>
> > +
> >      /**
> >       * This side data is encryption initialization data.
> >       * The format is not part of ABI, use av_encryption_init_info_*
> methods to
> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > index 2ece34f921..b537909a3f 100644
> > --- a/libavcodec/utils.c
> > +++ b/libavcodec/utils.c
> > @@ -42,8 +42,10 @@
> >  #include "libavutil/samplefmt.h"
> >  #include "libavutil/dict.h"
> >  #include "libavutil/thread.h"
> > +#include "libavutil/hdr_dynamic_metadata.h"
> >  #include "avcodec.h"
> >  #include "decode.h"
> > +#include "get_bits.h"
> >  #include "hwconfig.h"
> >  #include "libavutil/opt.h"
> >  #include "mpegvideo.h"
> > @@ -67,6 +69,17 @@
> >  const char av_codec_ffversion[] = "FFmpeg version " FFMPEG_VERSION;
> >
> >  static AVMutex codec_mutex = AV_MUTEX_INITIALIZER;
> > +static const uint8_t usa_country_code = 0xB5;
> > +static const uint16_t smpte_provider_code = 0x003C;
> > +static const uint16_t smpte2094_40_provider_oriented_code = 0x0001;
> > +static const uint16_t smpte2094_40_application_identifier = 0x04;
> > +static const int64_t luminance_den = 1;
> > +static const int32_t peak_luminance_den = 15;
> > +static const int64_t rgb_den = 100000;
> > +static const int32_t fraction_pixel_den = 1000;
> > +static const int32_t knee_point_den = 4095;
> > +static const int32_t bezier_anchor_den = 1023;
> > +static const int32_t saturation_weight_den = 8;
>
> Same, no global state.
>
Done.

>
> >
> >  void av_fast_padded_malloc(void *ptr, unsigned int *size, size_t
> min_size)
> >  {
> > @@ -2346,3 +2359,170 @@ int ff_int_from_list_or_default(void *ctx, const
> char * val_name, int val,
> >             "%s %d are not supported. Set to default value : %d\n",
> val_name, val, default_value);
> >      return default_value;
> >  }
> > +
> > +int ff_read_itu_t_t35_to_dynamic_hdr_plus(void *gbc,  AVBufferRef
> *output)
>
> Unless this is used by other decoders, it should not be added to utils.c
> Put it in hevc_sei.c
>
Yes, we are going to use it for matroka in avf.

>
> > +{
> > +    GetBitContext *gb = (GetBitContext *)gbc;
> > +    uint8_t application_version  = get_bits(gbc, 8);
> > +    size_t size;
> > +    int w, i, j;
> > +    AVDynamicHDRPlus *s = av_dynamic_hdr_plus_alloc(&size);
> > +    if (!s)
> > +        return AVERROR(ENOMEM);
> > +
> > +    if (output)
> > +        av_buffer_unref(&output);
> > +
> > +    output = av_buffer_create(
> > +        (uint8_t *)s, size, av_buffer_default_free, NULL, 0);
> > +    if (!output) {
> > +        av_freep(&s);
> > +        return AVERROR(ENOMEM);
> > +    }
> > +    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 (w = 1; w < s->num_windows; w++) {
> > +        s->params[w].window_upper_left_corner_x.num = get_bits(gb, 16);
> > +        s->params[w].window_upper_left_corner_y.num = get_bits(gb, 16);
> > +        s->params[w].window_lower_right_corner_x.num = get_bits(gb, 16);
> > +        s->params[w].window_lower_right_corner_y.num = get_bits(gb, 16);
> > +        // The corners are set to absolute coordinates here. They
> should be
> > +        // converted to the relative coordinates (in [0, 1]) in the
> decoder.
> > +        s->params[w].window_upper_left_corner_x.den = 1;
> > +        s->params[w].window_upper_left_corner_y.den = 1;
> > +        s->params[w].window_lower_right_corner_x.den = 1;
> > +        s->params[w].window_lower_right_corner_y.den = 1;
> > +
> > +        s->params[w].center_of_ellipse_x = get_bits(gb, 16);
> > +        s->params[w].center_of_ellipse_y = get_bits(gb, 16);
> > +        s->params[w].rotation_angle = get_bits(gb, 8);
> > +        s->params[w].semimajor_axis_internal_ellipse = get_bits(gb, 16);
> > +        s->params[w].semimajor_axis_external_ellipse = get_bits(gb, 16);
> > +        s->params[w].semiminor_axis_external_ellipse = get_bits(gb, 16);
> > +        s->params[w].overlap_process_option = get_bits1(gb);
> > +    }
> > +
> > +    if (get_bits_left(gb) < 28)
> > +        return AVERROR(EINVAL);
> > +    s->targeted_system_display_maximum_luminance.num = get_bits(gb, 27);
> > +    s->targeted_system_display_maximum_luminance.den = luminance_den;
> > +    s->targeted_system_display_actual_peak_luminance_flag =
> get_bits1(gb);
> > +
> > +    if (s->targeted_system_display_actual_peak_luminance_flag) {
> > +        int rows, cols;
> > +        if (get_bits_left(gb) < 10)
> > +            return AVERROR(EINVAL);
> > +        rows = get_bits(gb, 5);
> > +        cols = get_bits(gb, 5);
> > +        if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25)))
> {
> > +            return AVERROR_INVALIDDATA;
> > +        }
> > +        s->num_rows_targeted_system_display_actual_peak_luminance =
> rows;
> > +        s->num_cols_targeted_system_display_actual_peak_luminance =
> cols;
> > +
> > +        if (get_bits_left(gb) < (rows * cols * 4))
> > +            return AVERROR(EINVAL);
> > +
> > +        for (i = 0; i < rows; i++) {
> > +            for (j = 0; j < cols; j++) {
> > +
> s->targeted_system_display_actual_peak_luminance[i][j].num = get_bits(gb,
> 4);
> > +
> s->targeted_system_display_actual_peak_luminance[i][j].den =
> peak_luminance_den;
> > +            }
> > +        }
> > +    }
> > +    for (w = 0; w < s->num_windows; w++) {
> > +        if (get_bits_left(gb) < (3 * 17 + 17 + 4))
> > +            return AVERROR(EINVAL);
> > +        for (i = 0; i < 3; i++) {
> > +            s->params[w].maxscl[i].num = get_bits(gb, 17);
> > +            s->params[w].maxscl[i].den = rgb_den;
> > +        }
> > +        s->params[w].average_maxrgb.num = get_bits(gb, 17);
> > +        s->params[w].average_maxrgb.den = rgb_den;
> > +        s->params[w].num_distribution_maxrgb_percentiles = get_bits(gb,
> 4);
> > +
> > +        if (get_bits_left(gb) <
> > +            (s->params[w].num_distribution_maxrgb_percentiles * 24))
> > +            return AVERROR(EINVAL);
> > +        for (i = 0; i <
> s->params[w].num_distribution_maxrgb_percentiles; i++) {
> > +            s->params[w].distribution_maxrgb[i].percentage =
> get_bits(gb, 7);
> > +            s->params[w].distribution_maxrgb[i].percentile.num =
> get_bits(gb, 17);
> > +            s->params[w].distribution_maxrgb[i].percentile.den =
> rgb_den;
> > +        }
> > +
> > +        if (get_bits_left(gb) < 10)
> > +            return AVERROR(EINVAL);
> > +        s->params[w].fraction_bright_pixels.num = get_bits(gb, 10);
> > +        s->params[w].fraction_bright_pixels.den = fraction_pixel_den;
> > +    }
> > +    if (get_bits_left(gb) < 1)
> > +        return AVERROR(EINVAL);
> > +    s->mastering_display_actual_peak_luminance_flag = get_bits1(gb);
> > +    if (s->mastering_display_actual_peak_luminance_flag) {
> > +        int rows, cols;
> > +        if (get_bits_left(gb) < 10)
> > +            return AVERROR(EINVAL);
> > +        rows = get_bits(gb, 5);
> > +        cols = get_bits(gb, 5);
> > +        if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25)))
> {
> > +            return AVERROR_INVALIDDATA;
> > +        }
> > +        s->num_rows_mastering_display_actual_peak_luminance = rows;
> > +        s->num_cols_mastering_display_actual_peak_luminance = cols;
> > +
> > +        if (get_bits_left(gb) < (rows * cols * 4))
> > +            return AVERROR(EINVAL);
> > +
> > +        for (i = 0; i < rows; i++) {
> > +            for (j = 0; j < cols; j++) {
> > +                s->mastering_display_actual_peak_luminance[i][j].num =
> get_bits(gb, 4);
> > +                s->mastering_display_actual_peak_luminance[i][j].den =
> peak_luminance_den;
> > +            }
> > +        }
> > +    }
> > +
> > +    for (w = 0; w < s->num_windows; w++) {
> > +        if (get_bits_left(gb) < 1)
> > +            return AVERROR(EINVAL);
> > +        s->params[w].tone_mapping_flag = get_bits1(gb);
> > +        if (s->params[w].tone_mapping_flag) {
> > +            if (get_bits_left(gb) < 28)
> > +                return AVERROR(EINVAL);
> > +            s->params[w].knee_point_x.num = get_bits(gb, 12);
> > +            s->params[w].knee_point_x.den = knee_point_den;
> > +            s->params[w].knee_point_y.num = get_bits(gb, 12);
> > +            s->params[w].knee_point_y.den = knee_point_den;
> > +            s->params[w].num_bezier_curve_anchors = get_bits(gb, 4);
> > +
> > +            if (get_bits_left(gb) <
> (s->params[w].num_bezier_curve_anchors * 10))
> > +                return AVERROR(EINVAL);
> > +            for (i = 0; i < s->params[w].num_bezier_curve_anchors; i++)
> {
> > +                s->params[w].bezier_curve_anchors[i].num = get_bits(gb,
> 10);
> > +                s->params[w].bezier_curve_anchors[i].den =
> bezier_anchor_den;
> > +            }
> > +        }
> > +
> > +        if (get_bits_left(gb) < 1)
> > +            return AVERROR(EINVAL);
> > +        s->params[w].color_saturation_mapping_flag = get_bits1(gb);
> > +        if (s->params[w].color_saturation_mapping_flag) {
> > +            if (get_bits_left(gb) < 6)
> > +                return AVERROR(EINVAL);
> > +            s->params[w].color_saturation_weight.num = get_bits(gb, 6);
> > +            s->params[w].color_saturation_weight.den =
> saturation_weight_den;
> > +        }
> > +    }
> > +
> > +    skip_bits(gb, get_bits_left(gb));
> > +
> > +    return 0;
> > +}
> > diff --git a/libavcodec/version.h b/libavcodec/version.h
> > index e75891d463..ad0bfd619d 100644
> > --- a/libavcodec/version.h
> > +++ b/libavcodec/version.h
> > @@ -28,7 +28,7 @@
> >  #include "libavutil/version.h"
> >
> >  #define LIBAVCODEC_VERSION_MAJOR  58
> > -#define LIBAVCODEC_VERSION_MINOR  95
> > +#define LIBAVCODEC_VERSION_MINOR  96
> >  #define LIBAVCODEC_VERSION_MICRO 100
> >
> >  #define LIBAVCODEC_VERSION_INT
> AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Mohammad Izadi July 23, 2020, 10:29 p.m. UTC | #9
Thanks,
Mohammad


On Thu, Jul 23, 2020 at 1:14 AM zhilizhao <quinkblack@foxmail.com> wrote:

>
>
> > On Jul 17, 2020, at 5:47 AM, Steinar H. Gunderson <
> steinar+ffmpeg@gunderson.no> wrote:
> >
> > On Thu, Jul 16, 2020 at 06:34:31PM -0300, James Almer wrote:
> >>> static AVMutex codec_mutex = AV_MUTEX_INITIALIZER;
> >>> +static const uint8_t usa_country_code = 0xB5;
> >>> +static const uint16_t smpte_provider_code = 0x003C;
> >>> +static const uint16_t smpte2094_40_provider_oriented_code = 0x0001;
> >>> +static const uint16_t smpte2094_40_application_identifier = 0x04;
> >>> +static const int64_t luminance_den = 1;
> >>> +static const int32_t peak_luminance_den = 15;
> >>> +static const int64_t rgb_den = 100000;
> >>> +static const int32_t fraction_pixel_den = 1000;
> >>> +static const int32_t knee_point_den = 4095;
> >>> +static const int32_t bezier_anchor_den = 1023;
> >>> +static const int32_t saturation_weight_den = 8;
> >> Same, no global state.
> >
> > It's not state if it can't be changed.
>
> If there is any chance to reuse these variables, I prefer current style
> than
> use a literal with comments next to it.
>
I am going to use them in both read/write. For now I moved them to the
local function and I will make them global when I added the write func.

>
> >
> > /* Steinar */
> > --
> > Homepage: https://www.sesse.net/
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Mohammad Izadi July 23, 2020, 11:09 p.m. UTC | #10
I am not sure if you received the patch in reply to the thread? I used:
git send-email  0001-Support-HDR10-metadata-for-HEVC.patch --to=
ffmpeg-devel@ffmpeg.org --in-reply-to=
422719c5-f010-6b39-6415-b3bf46dcbf19@rothenpieler.org


It seems it created a new thread. Did you receive it?

Thanks,
Mohammad


On Thu, Jul 23, 2020 at 3:29 PM Mohammad Izadi <izadi@google.com> wrote:

>
> Thanks,
> Mohammad
>
>
> On Thu, Jul 23, 2020 at 1:14 AM zhilizhao <quinkblack@foxmail.com> wrote:
>
>>
>>
>> > On Jul 17, 2020, at 5:47 AM, Steinar H. Gunderson <
>> steinar+ffmpeg@gunderson.no> wrote:
>> >
>> > On Thu, Jul 16, 2020 at 06:34:31PM -0300, James Almer wrote:
>> >>> static AVMutex codec_mutex = AV_MUTEX_INITIALIZER;
>> >>> +static const uint8_t usa_country_code = 0xB5;
>> >>> +static const uint16_t smpte_provider_code = 0x003C;
>> >>> +static const uint16_t smpte2094_40_provider_oriented_code = 0x0001;
>> >>> +static const uint16_t smpte2094_40_application_identifier = 0x04;
>> >>> +static const int64_t luminance_den = 1;
>> >>> +static const int32_t peak_luminance_den = 15;
>> >>> +static const int64_t rgb_den = 100000;
>> >>> +static const int32_t fraction_pixel_den = 1000;
>> >>> +static const int32_t knee_point_den = 4095;
>> >>> +static const int32_t bezier_anchor_den = 1023;
>> >>> +static const int32_t saturation_weight_den = 8;
>> >> Same, no global state.
>> >
>> > It's not state if it can't be changed.
>>
>> If there is any chance to reuse these variables, I prefer current style
>> than
>> use a literal with comments next to it.
>>
> I am going to use them in both read/write. For now I moved them to the
> local function and I will make them global when I added the write func.
>
>>
>> >
>> > /* Steinar */
>> > --
>> > Homepage: https://www.sesse.net/
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel@ffmpeg.org
>> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >
>> > To unsubscribe, visit link above, or email
>> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
>
James Almer July 24, 2020, 2:20 p.m. UTC | #11
On 7/23/2020 7:26 PM, Mohammad Izadi wrote:
>>>  void av_fast_padded_malloc(void *ptr, unsigned int *size, size_t
>> min_size)
>>>  {
>>> @@ -2346,3 +2359,170 @@ int ff_int_from_list_or_default(void *ctx, const
>> char * val_name, int val,
>>>             "%s %d are not supported. Set to default value : %d\n",
>> val_name, val, default_value);
>>>      return default_value;
>>>  }
>>> +
>>> +int ff_read_itu_t_t35_to_dynamic_hdr_plus(void *gbc,  AVBufferRef
>> *output)
>>
>> Unless this is used by other decoders, it should not be added to utils.c
>> Put it in hevc_sei.c
>>
> Yes, we are going to use it for matroka in avf.

If you need to share this function between libraries, then it needs to
use the avpriv prefix, and must not use a pointer to a GetBitContext.

See how avpriv_ac3_parse_header() and ff_ac3_parse_header() are handled
for an example of this. You'd need to do something like:

> int ff_read_itu_t_t35_to_dynamic_hdr_plus(GetBitContext *gbc,  AVBufferRef *output)
> {
> [...]
> }
> 
> int avpriv_read_itu_t_t35_to_dynamic_hdr_plus(const uint8_t *buf, int size, AVBufferRef *output)
> {
>     GetBitContext gb;
>     int ret = init_get_bits8(&gb, buf, size);
>     if (ret < 0)
>         return ret;
>     return ff_read_itu_t_t35_to_dynamic_hdr_plus(&gb, output);
> }

Also, please move this code to its own file within lavc, instead of
adding it to utils.c
diff mbox series

Patch

diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index dce26cb31a..8307032335 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -394,6 +394,7 @@  const char *av_packet_side_data_name(enum AVPacketSideDataType type)
     case AV_PKT_DATA_CONTENT_LIGHT_LEVEL:        return "Content light level metadata";
     case AV_PKT_DATA_SPHERICAL:                  return "Spherical Mapping";
     case AV_PKT_DATA_A53_CC:                     return "A53 Closed Captions";
+    case AV_PKT_DATA_DYNAMIC_HDR_PLUS:           return "HDR10+ Dynamic Metadata (SMPTE 2094-40)";
     case AV_PKT_DATA_ENCRYPTION_INIT_INFO:       return "Encryption initialization data";
     case AV_PKT_DATA_ENCRYPTION_INFO:            return "Encryption info";
     case AV_PKT_DATA_AFD:                        return "Active Format Description data";
diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index de9c079f9d..cd3286f7fb 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -1698,6 +1698,7 @@  int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
         { AV_PKT_DATA_MASTERING_DISPLAY_METADATA, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA },
         { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,        AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
         { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
+        { AV_PKT_DATA_DYNAMIC_HDR_PLUS,           AV_FRAME_DATA_DYNAMIC_HDR_PLUS },
         { AV_PKT_DATA_ICC_PROFILE,                AV_FRAME_DATA_ICC_PROFILE },
     };
 
diff --git a/libavcodec/hevc_sei.c b/libavcodec/hevc_sei.c
index a4ec65dc1a..096c414d91 100644
--- a/libavcodec/hevc_sei.c
+++ b/libavcodec/hevc_sei.c
@@ -25,6 +25,12 @@ 
 #include "golomb.h"
 #include "hevc_ps.h"
 #include "hevc_sei.h"
+#include "internal.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 int decode_nal_sei_decoded_picture_hash(HEVCSEIPictureHash *s, GetBitContext *gb)
 {
@@ -242,8 +248,8 @@  static int decode_nal_sei_user_data_unregistered(HEVCSEIUnregistered *s, GetBitC
 static int decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s, GetBitContext *gb,
                                                          int size)
 {
-    uint32_t country_code;
-    uint32_t user_identifier;
+    uint8_t country_code;
+    uint16_t provider_code;
 
     if (size < 7)
         return AVERROR(EINVAL);
@@ -255,18 +261,27 @@  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
+        uint16_t provider_oriented_code = get_bits(gb, 16);
+        uint8_t application_identifier = get_bits(gb, 8);
+        if (provider_oriented_code == smpte2094_40_provider_oriented_code &&
+            application_identifier == smpte2094_40_application_identifier) {
+            int err = ff_read_itu_t_t35_to_dynamic_hdr_plus(gb, s->dynamic_hdr_plus.info);
+            if (err < 0 && s->dynamic_hdr_plus.info) {
+                av_buffer_unref(&s->dynamic_hdr_plus.info);
+            }
+            return err;
+        }
+    } else {
+        uint32_t  user_identifier = get_bits_long(gb, 32);
+        if(user_identifier == MKBETAG('G', 'A', '9', '4'))
             return decode_registered_user_data_closed_caption(&s->a53_caption, gb, size);
-        default:
-            skip_bits_long(gb, size * 8);
-            break;
     }
+    skip_bits_long(gb, size * 8);
     return 0;
 }
 
@@ -453,4 +468,5 @@  void ff_hevc_reset_sei(HEVCSEI *s)
         av_buffer_unref(&s->unregistered.buf_ref[i]);
     s->unregistered.nb_buf_ref = 0;
     av_freep(&s->unregistered.buf_ref);
+    av_buffer_unref(&s->dynamic_hdr_plus.info);
 }
diff --git a/libavcodec/hevc_sei.h b/libavcodec/hevc_sei.h
index 5ee7a4796d..e9e2d46ed4 100644
--- a/libavcodec/hevc_sei.h
+++ b/libavcodec/hevc_sei.h
@@ -104,6 +104,10 @@  typedef struct HEVCSEIMasteringDisplay {
     uint32_t min_luminance;
 } HEVCSEIMasteringDisplay;
 
+typedef struct HEVCSEIDynamicHDRPlus {
+    AVBufferRef *info;
+} HEVCSEIDynamicHDRPlus;
+
 typedef struct HEVCSEIContentLight {
     int present;
     uint16_t max_content_light_level;
@@ -143,6 +147,7 @@  typedef struct HEVCSEI {
     HEVCSEIA53Caption a53_caption;
     HEVCSEIUnregistered unregistered;
     HEVCSEIMasteringDisplay mastering_display;
+    HEVCSEIDynamicHDRPlus dynamic_hdr_plus;
     HEVCSEIContentLight content_light;
     int active_seq_parameter_set_id;
     HEVCSEIAlternativeTransfer alternative_transfer;
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index b77df8d89f..748233fa32 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -2849,6 +2849,13 @@  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);
+
+        av_frame_new_side_data_from_buf(out, AV_FRAME_DATA_DYNAMIC_HDR_PLUS, info_ref);
+    }
 
     return 0;
 }
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index 0a1c0a17ec..744ace534c 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -413,6 +413,15 @@  int ff_int_from_list_or_default(void *ctx, const char * val_name, int val,
 
 void ff_dvdsub_parse_palette(uint32_t *palette, const char *p);
 
+/**
+ * Reads and decode the user data registered ITU-T T.35 to AVbuffer (AVDynamicHDRPlus).
+ * @param gbc The bit content to be decoded.
+ * @param output A buffer containing the decoded AVDynamicHDRPlus structure.
+ *
+ * @return 0 if succeed. Otherwise, returns the appropriate AVERROR.
+ */
+int ff_read_itu_t_t35_to_dynamic_hdr_plus(void *gbc, AVBufferRef *output);
+
 #if defined(_WIN32) && CONFIG_SHARED && !defined(BUILDING_avcodec)
 #    define av_export_avcodec __declspec(dllimport)
 #else
diff --git a/libavcodec/packet.h b/libavcodec/packet.h
index 96f237f091..695df15806 100644
--- a/libavcodec/packet.h
+++ b/libavcodec/packet.h
@@ -241,6 +241,15 @@  enum AVPacketSideDataType {
      */
     AV_PKT_DATA_A53_CC,
 
+
+    /**
+     * 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,
+
     /**
      * This side data is encryption initialization data.
      * The format is not part of ABI, use av_encryption_init_info_* methods to
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 2ece34f921..b537909a3f 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -42,8 +42,10 @@ 
 #include "libavutil/samplefmt.h"
 #include "libavutil/dict.h"
 #include "libavutil/thread.h"
+#include "libavutil/hdr_dynamic_metadata.h"
 #include "avcodec.h"
 #include "decode.h"
+#include "get_bits.h"
 #include "hwconfig.h"
 #include "libavutil/opt.h"
 #include "mpegvideo.h"
@@ -67,6 +69,17 @@ 
 const char av_codec_ffversion[] = "FFmpeg version " FFMPEG_VERSION;
 
 static AVMutex codec_mutex = AV_MUTEX_INITIALIZER;
+static const uint8_t usa_country_code = 0xB5;
+static const uint16_t smpte_provider_code = 0x003C;
+static const uint16_t smpte2094_40_provider_oriented_code = 0x0001;
+static const uint16_t smpte2094_40_application_identifier = 0x04;
+static const int64_t luminance_den = 1;
+static const int32_t peak_luminance_den = 15;
+static const int64_t rgb_den = 100000;
+static const int32_t fraction_pixel_den = 1000;
+static const int32_t knee_point_den = 4095;
+static const int32_t bezier_anchor_den = 1023;
+static const int32_t saturation_weight_den = 8;
 
 void av_fast_padded_malloc(void *ptr, unsigned int *size, size_t min_size)
 {
@@ -2346,3 +2359,170 @@  int ff_int_from_list_or_default(void *ctx, const char * val_name, int val,
            "%s %d are not supported. Set to default value : %d\n", val_name, val, default_value);
     return default_value;
 }
+
+int ff_read_itu_t_t35_to_dynamic_hdr_plus(void *gbc,  AVBufferRef *output)
+{
+    GetBitContext *gb = (GetBitContext *)gbc;
+    uint8_t application_version  = get_bits(gbc, 8);
+    size_t size;
+    int w, i, j;
+    AVDynamicHDRPlus *s = av_dynamic_hdr_plus_alloc(&size);
+    if (!s)
+        return AVERROR(ENOMEM);
+
+    if (output)
+        av_buffer_unref(&output);
+
+    output = av_buffer_create(
+        (uint8_t *)s, size, av_buffer_default_free, NULL, 0);
+    if (!output) {
+        av_freep(&s);
+        return AVERROR(ENOMEM);
+    }
+    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 (w = 1; w < s->num_windows; w++) {
+        s->params[w].window_upper_left_corner_x.num = get_bits(gb, 16);
+        s->params[w].window_upper_left_corner_y.num = get_bits(gb, 16);
+        s->params[w].window_lower_right_corner_x.num = get_bits(gb, 16);
+        s->params[w].window_lower_right_corner_y.num = get_bits(gb, 16);
+        // The corners are set to absolute coordinates here. They should be
+        // converted to the relative coordinates (in [0, 1]) in the decoder.
+        s->params[w].window_upper_left_corner_x.den = 1;
+        s->params[w].window_upper_left_corner_y.den = 1;
+        s->params[w].window_lower_right_corner_x.den = 1;
+        s->params[w].window_lower_right_corner_y.den = 1;
+
+        s->params[w].center_of_ellipse_x = get_bits(gb, 16);
+        s->params[w].center_of_ellipse_y = get_bits(gb, 16);
+        s->params[w].rotation_angle = get_bits(gb, 8);
+        s->params[w].semimajor_axis_internal_ellipse = get_bits(gb, 16);
+        s->params[w].semimajor_axis_external_ellipse = get_bits(gb, 16);
+        s->params[w].semiminor_axis_external_ellipse = get_bits(gb, 16);
+        s->params[w].overlap_process_option = get_bits1(gb);
+    }
+
+    if (get_bits_left(gb) < 28)
+        return AVERROR(EINVAL);
+    s->targeted_system_display_maximum_luminance.num = get_bits(gb, 27);
+    s->targeted_system_display_maximum_luminance.den = luminance_den;
+    s->targeted_system_display_actual_peak_luminance_flag = get_bits1(gb);
+
+    if (s->targeted_system_display_actual_peak_luminance_flag) {
+        int rows, cols;
+        if (get_bits_left(gb) < 10)
+            return AVERROR(EINVAL);
+        rows = get_bits(gb, 5);
+        cols = get_bits(gb, 5);
+        if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25))) {
+            return AVERROR_INVALIDDATA;
+        }
+        s->num_rows_targeted_system_display_actual_peak_luminance = rows;
+        s->num_cols_targeted_system_display_actual_peak_luminance = cols;
+
+        if (get_bits_left(gb) < (rows * cols * 4))
+            return AVERROR(EINVAL);
+
+        for (i = 0; i < rows; i++) {
+            for (j = 0; j < cols; j++) {
+                s->targeted_system_display_actual_peak_luminance[i][j].num = get_bits(gb, 4);
+                s->targeted_system_display_actual_peak_luminance[i][j].den = peak_luminance_den;
+            }
+        }
+    }
+    for (w = 0; w < s->num_windows; w++) {
+        if (get_bits_left(gb) < (3 * 17 + 17 + 4))
+            return AVERROR(EINVAL);
+        for (i = 0; i < 3; i++) {
+            s->params[w].maxscl[i].num = get_bits(gb, 17);
+            s->params[w].maxscl[i].den = rgb_den;
+        }
+        s->params[w].average_maxrgb.num = get_bits(gb, 17);
+        s->params[w].average_maxrgb.den = rgb_den;
+        s->params[w].num_distribution_maxrgb_percentiles = get_bits(gb, 4);
+
+        if (get_bits_left(gb) <
+            (s->params[w].num_distribution_maxrgb_percentiles * 24))
+            return AVERROR(EINVAL);
+        for (i = 0; i < s->params[w].num_distribution_maxrgb_percentiles; i++) {
+            s->params[w].distribution_maxrgb[i].percentage = get_bits(gb, 7);
+            s->params[w].distribution_maxrgb[i].percentile.num = get_bits(gb, 17);
+            s->params[w].distribution_maxrgb[i].percentile.den = rgb_den;
+        }
+
+        if (get_bits_left(gb) < 10)
+            return AVERROR(EINVAL);
+        s->params[w].fraction_bright_pixels.num = get_bits(gb, 10);
+        s->params[w].fraction_bright_pixels.den = fraction_pixel_den;
+    }
+    if (get_bits_left(gb) < 1)
+        return AVERROR(EINVAL);
+    s->mastering_display_actual_peak_luminance_flag = get_bits1(gb);
+    if (s->mastering_display_actual_peak_luminance_flag) {
+        int rows, cols;
+        if (get_bits_left(gb) < 10)
+            return AVERROR(EINVAL);
+        rows = get_bits(gb, 5);
+        cols = get_bits(gb, 5);
+        if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25))) {
+            return AVERROR_INVALIDDATA;
+        }
+        s->num_rows_mastering_display_actual_peak_luminance = rows;
+        s->num_cols_mastering_display_actual_peak_luminance = cols;
+
+        if (get_bits_left(gb) < (rows * cols * 4))
+            return AVERROR(EINVAL);
+
+        for (i = 0; i < rows; i++) {
+            for (j = 0; j < cols; j++) {
+                s->mastering_display_actual_peak_luminance[i][j].num = get_bits(gb, 4);
+                s->mastering_display_actual_peak_luminance[i][j].den = peak_luminance_den;
+            }
+        }
+    }
+
+    for (w = 0; w < s->num_windows; w++) {
+        if (get_bits_left(gb) < 1)
+            return AVERROR(EINVAL);
+        s->params[w].tone_mapping_flag = get_bits1(gb);
+        if (s->params[w].tone_mapping_flag) {
+            if (get_bits_left(gb) < 28)
+                return AVERROR(EINVAL);
+            s->params[w].knee_point_x.num = get_bits(gb, 12);
+            s->params[w].knee_point_x.den = knee_point_den;
+            s->params[w].knee_point_y.num = get_bits(gb, 12);
+            s->params[w].knee_point_y.den = knee_point_den;
+            s->params[w].num_bezier_curve_anchors = get_bits(gb, 4);
+
+            if (get_bits_left(gb) < (s->params[w].num_bezier_curve_anchors * 10))
+                return AVERROR(EINVAL);
+            for (i = 0; i < s->params[w].num_bezier_curve_anchors; i++) {
+                s->params[w].bezier_curve_anchors[i].num = get_bits(gb, 10);
+                s->params[w].bezier_curve_anchors[i].den = bezier_anchor_den;
+            }
+        }
+
+        if (get_bits_left(gb) < 1)
+            return AVERROR(EINVAL);
+        s->params[w].color_saturation_mapping_flag = get_bits1(gb);
+        if (s->params[w].color_saturation_mapping_flag) {
+            if (get_bits_left(gb) < 6)
+                return AVERROR(EINVAL);
+            s->params[w].color_saturation_weight.num = get_bits(gb, 6);
+            s->params[w].color_saturation_weight.den = saturation_weight_den;
+        }
+    }
+
+    skip_bits(gb, get_bits_left(gb));
+
+    return 0;
+}
diff --git a/libavcodec/version.h b/libavcodec/version.h
index e75891d463..ad0bfd619d 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -28,7 +28,7 @@ 
 #include "libavutil/version.h"
 
 #define LIBAVCODEC_VERSION_MAJOR  58
-#define LIBAVCODEC_VERSION_MINOR  95
+#define LIBAVCODEC_VERSION_MINOR  96
 #define LIBAVCODEC_VERSION_MICRO 100
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \