[FFmpeg-devel] Pass the HDR10+ metadata to the packet side data in VP9 encoder

HDR10+ metadata is stored in the bit stream for HEVC. The story is different for VP9 and cannot store the metadata in the bit stream. HDR10+ should be passed to packet side data an stored in the container (mkv) for VP9.

This CL is taking HDR10+ from AVFrame side data in libvpxenc and is passing it to the AVPacket side data.
---
 libavcodec/avpacket.c  |  1 +
 libavcodec/decode.c    |  1 +
 libavcodec/libvpxenc.c | 88 +++++++++++++++++++++++++++++++++++++++++-
 libavcodec/packet.h    |  8 ++++
 4 files changed, 96 insertions(+), 2 deletions(-)

Any more comments or looks good to go?
Thanks,
Mohammad


On Mon, Apr 26, 2021 at 6:54 PM Mohammad Izadi <izadi@google.com> wrote:

> HDR10+ metadata is stored in the bit stream for HEVC. The story is
> different for VP9 and cannot store the metadata in the bit stream. HDR10+
> should be passed to packet side data an stored in the container (mkv) for
> VP9.
>
> This CL is taking HDR10+ from AVFrame side data in libvpxenc and is
> passing it to the AVPacket side data.
> ---
>  libavcodec/avpacket.c  |  1 +
>  libavcodec/decode.c    |  1 +
>  libavcodec/libvpxenc.c | 88 +++++++++++++++++++++++++++++++++++++++++-
>  libavcodec/packet.h    |  8 ++++
>  4 files changed, 96 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index b5bac5c5f2..7a3b0a73e3 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -416,6 +416,7 @@ const char *av_packet_side_data_name(enum
> AVPacketSideDataType type)
>      case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
>      case AV_PKT_DATA_DOVI_CONF:                  return "DOVI
> configuration record";
>      case AV_PKT_DATA_S12M_TIMECODE:              return "SMPTE ST
> 12-1:2014 timecode";
> +    case AV_PKT_DATA_DYNAMIC_HDR10_PLUS:         return "HDR10+ Dynamic
> Metadata (SMPTE 2094-40)";
>      }
>      return NULL;
>  }
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index 0956a6ac6f..bf5fbcca97 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -1736,6 +1736,7 @@ int ff_decode_frame_props(AVCodecContext *avctx,
> AVFrame *frame)
>          { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
>          { AV_PKT_DATA_ICC_PROFILE,
> AV_FRAME_DATA_ICC_PROFILE },
>          { AV_PKT_DATA_S12M_TIMECODE,
> AV_FRAME_DATA_S12M_TIMECODE },
> +        { AV_PKT_DATA_DYNAMIC_HDR10_PLUS,
>  AV_FRAME_DATA_DYNAMIC_HDR_PLUS },
>      };
>
>      if (IS_EMPTY(pkt) && av_fifo_size(avctx->internal->pkt_props) >=
> sizeof(*pkt))
> diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> index 3f36943c12..2096c08437 100644
> --- a/libavcodec/libvpxenc.c
> +++ b/libavcodec/libvpxenc.c
> @@ -63,6 +63,12 @@ struct FrameListData {
>      struct FrameListData *next;
>  };
>
> +typedef struct FrameHDR10PlusList {
> +    int64_t pts;
> +    AVBufferRef *hdr10_plus;
> +    struct FrameHDR10PlusList *next;
> +} FrameHDR10PlusList;
> +
>  typedef struct VPxEncoderContext {
>      AVClass *class;
>      struct vpx_codec_ctx encoder;
> @@ -120,6 +126,8 @@ typedef struct VPxEncoderContext {
>      int tune_content;
>      int corpus_complexity;
>      int tpl_model;
> +    int discard_hdr10_plus;
> +    struct FrameHDR10PlusList *hdr10_plus_list;
>      /**
>       * If the driver does not support ROI then warn the first time we
>       * encounter a frame with ROI side data.
> @@ -315,6 +323,53 @@ static av_cold void free_frame_list(struct
> FrameListData *list)
>      }
>  }
>
> +
> +static void add_hdr10_plus(void *list, struct FrameHDR10PlusList *data)
> +{
> +    struct FrameHDR10PlusList **p = list;
> +    while (*p)
> +        p = &(*p)->next;
> +    *p = data;
> +    data->next = NULL;
> +}
> +
> +static av_cold void free_hdr10_plus(struct FrameHDR10PlusList *p)
> +{
> +    av_buffer_unref(&p->hdr10_plus);
> +    av_free(p);
> +}
> +
> +static av_cold void free_hdr10_plus_list(struct FrameHDR10PlusList *list)
> +{
> +    struct FrameHDR10PlusList *p = list;
> +    while (p) {
> +        list = list->next;
> +        free_hdr10_plus(p);
> +        p = list;
> +    }
> +}
> +
> +static int copy_hdr10_plus_to_pkt(void *list, AVPacket *pkt)
> +{
> +    struct FrameHDR10PlusList **p = list;
> +    struct FrameHDR10PlusList *head = *p;
> +
> +    if (head && pkt && head->hdr10_plus && head->pts == pkt->pts) {
> +        uint8_t *data;
> +        *p = (*p)->next;
> +        data = av_packet_new_side_data(pkt,
> AV_PKT_DATA_DYNAMIC_HDR10_PLUS, head->hdr10_plus->size);
> +
> +        if (!data) {
> +            free_hdr10_plus(head);
> +            return AVERROR(ENOMEM);
> +        }
> +        memcpy(data, head->hdr10_plus->data, head->hdr10_plus->size);
> +        free_hdr10_plus(head);
> +
> +    }
> +    return 0;
> +}
> +
>  static av_cold int codecctl_int(AVCodecContext *avctx,
>                                  enum vp8e_enc_control_id id, int val)
>  {
> @@ -383,6 +438,7 @@ static av_cold int vpx_free(AVCodecContext *avctx)
>      av_freep(&ctx->twopass_stats.buf);
>      av_freep(&avctx->stats_out);
>      free_frame_list(ctx->coded_frame_list);
> +    free_hdr10_plus_list(ctx->hdr10_plus_list);
>      return 0;
>  }
>
> @@ -828,6 +884,7 @@ static av_cold int vpx_init(AVCodecContext *avctx,
>      AVCPBProperties *cpb_props;
>      int res;
>      vpx_img_fmt_t img_fmt = VPX_IMG_FMT_I420;
> +    ctx->discard_hdr10_plus = 1;
>  #if CONFIG_LIBVPX_VP9_ENCODER
>      vpx_codec_caps_t codec_caps = vpx_codec_get_caps(iface);
>      vpx_svc_extra_cfg_t svc_params;
> @@ -850,11 +907,16 @@ static av_cold int vpx_init(AVCodecContext *avctx,
>      if (avctx->codec_id == AV_CODEC_ID_VP9) {
>          if (set_pix_fmt(avctx, codec_caps, &enccfg, &flags, &img_fmt))
>              return AVERROR(EINVAL);
> +        // Keep HDR10+ if it has bit depth higher than 8 and
> +        // it has PQ trc (SMPTE2084).
> +        if (enccfg.g_bit_depth > 8 && avctx->color_trc ==
> AVCOL_TRC_SMPTE2084) {
> +            ctx->discard_hdr10_plus = 0;
> +        }
>      }
>  #endif
>
> -    if(!avctx->bit_rate)
> -        if(avctx->rc_max_rate || avctx->rc_buffer_size ||
> avctx->rc_initial_buffer_occupancy) {
> +    if (!avctx->bit_rate)
> +        if (avctx->rc_max_rate || avctx->rc_buffer_size ||
> avctx->rc_initial_buffer_occupancy) {
>              av_log( avctx, AV_LOG_ERROR, "Rate control parameters set
> without a bitrate\n");
>              return AVERROR(EINVAL);
>          }
> @@ -1245,6 +1307,14 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              AV_WB64(side_data, 1);
>              memcpy(side_data + 8, cx_frame->buf_alpha,
> cx_frame->sz_alpha);
>          }
> +        if (cx_frame->frame_number != -1) {
> +            VPxContext *ctx = avctx->priv_data;
> +            if (!ctx->discard_hdr10_plus) {
> +                int err = copy_hdr10_plus_to_pkt(&ctx->hdr10_plus_list,
> pkt);
> +                if (err < 0)
> +                    return err;
> +            }
> +        }
>      } else {
>          return ret;
>      }
> @@ -1579,6 +1649,7 @@ static int vpx_encode(AVCodecContext *avctx,
> AVPacket *pkt,
>      const struct vpx_codec_enc_cfg *enccfg = ctx->encoder.config.enc;
>      vpx_svc_layer_id_t layer_id;
>      int layer_id_valid = 0;
> +    AVFrameSideData *hdr10_plus_metadata;
>
>      if (frame) {
>          const AVFrameSideData *sd = av_frame_get_side_data(frame,
> AV_FRAME_DATA_REGIONS_OF_INTEREST);
> @@ -1655,6 +1726,19 @@ static int vpx_encode(AVCodecContext *avctx,
> AVPacket *pkt,
>                  vp9_encode_set_roi(avctx, frame->width, frame->height,
> sd);
>              }
>          }
> +
> +        if (!ctx->discard_hdr10_plus) {
> +            // Add HDR10+ metadata to queue.
> +            hdr10_plus_metadata = av_frame_get_side_data(frame,
> AV_FRAME_DATA_DYNAMIC_HDR_PLUS);
> +            if (hdr10_plus_metadata) {
> +                struct FrameHDR10PlusList *data =
> av_malloc(sizeof(*data));
> +                if (!data)
> +                    return AVERROR(ENOMEM);
> +                data->pts = frame->pts;
> +                data->hdr10_plus =
> av_buffer_ref(hdr10_plus_metadata->buf);
> +                add_hdr10_plus(&ctx->hdr10_plus_list, data);
> +            }
> +        }
>      }
>
>      // this is for encoding with preset temporal layering patterns
> defined in
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index ca18ae631f..23a146ea7b 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_HDR10_PLUS,
> +
>      /**
>       * The number of side data types.
>       * This is not part of the public API/ABI in the sense that it may
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
>

You know that on Android gmail viewer you can delete the citation of
previsous email by long pressing on 3 dots after email like I just did?
Please do it next time. Also is Youtube going to permit HDR10+ globally?

Валерий Заподовников (12021-05-25):
> You know that on Android gmail viewer you can delete the citation of
> previsous email by long pressing on 3 dots after email like I just did?
> Please do it next time. Also is Youtube going to permit HDR10+ globally?

Please keep a little context, though.

Regards,

On 4/26/2021 10:54 PM, Mohammad Izadi wrote:
> HDR10+ metadata is stored in the bit stream for HEVC. The story is different for VP9 and cannot store the metadata in the bit stream. HDR10+ should be passed to packet side data an stored in the container (mkv) for VP9.
> 
> This CL is taking HDR10+ from AVFrame side data in libvpxenc and is passing it to the AVPacket side data.
> ---
>   libavcodec/avpacket.c  |  1 +
>   libavcodec/decode.c    |  1 +
>   libavcodec/libvpxenc.c | 88 +++++++++++++++++++++++++++++++++++++++++-
>   libavcodec/packet.h    |  8 ++++
>   4 files changed, 96 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index b5bac5c5f2..7a3b0a73e3 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -416,6 +416,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
>       case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
>       case AV_PKT_DATA_DOVI_CONF:                  return "DOVI configuration record";
>       case AV_PKT_DATA_S12M_TIMECODE:              return "SMPTE ST 12-1:2014 timecode";
> +    case AV_PKT_DATA_DYNAMIC_HDR10_PLUS:         return "HDR10+ Dynamic Metadata (SMPTE 2094-40)";
>       }
>       return NULL;
>   }
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index 0956a6ac6f..bf5fbcca97 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -1736,6 +1736,7 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame)
>           { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC },
>           { AV_PKT_DATA_ICC_PROFILE,                AV_FRAME_DATA_ICC_PROFILE },
>           { AV_PKT_DATA_S12M_TIMECODE,              AV_FRAME_DATA_S12M_TIMECODE },
> +        { AV_PKT_DATA_DYNAMIC_HDR10_PLUS,         AV_FRAME_DATA_DYNAMIC_HDR_PLUS },

This has nothing to do with encoding.

>       };
>   
>       if (IS_EMPTY(pkt) && av_fifo_size(avctx->internal->pkt_props) >= sizeof(*pkt))
> diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> index 3f36943c12..2096c08437 100644
> --- a/libavcodec/libvpxenc.c
> +++ b/libavcodec/libvpxenc.c
> @@ -63,6 +63,12 @@ struct FrameListData {
>       struct FrameListData *next;
>   };
>   
> +typedef struct FrameHDR10PlusList {
> +    int64_t pts;
> +    AVBufferRef *hdr10_plus;
> +    struct FrameHDR10PlusList *next;
> +} FrameHDR10PlusList;
> +
>   typedef struct VPxEncoderContext {
>       AVClass *class;
>       struct vpx_codec_ctx encoder;
> @@ -120,6 +126,8 @@ typedef struct VPxEncoderContext {
>       int tune_content;
>       int corpus_complexity;
>       int tpl_model;
> +    int discard_hdr10_plus;
> +    struct FrameHDR10PlusList *hdr10_plus_list;
>       /**
>        * If the driver does not support ROI then warn the first time we
>        * encounter a frame with ROI side data.
> @@ -315,6 +323,53 @@ static av_cold void free_frame_list(struct FrameListData *list)
>       }
>   }
>   
> +
> +static void add_hdr10_plus(void *list, struct FrameHDR10PlusList *data)
> +{
> +    struct FrameHDR10PlusList **p = list;
> +    while (*p)
> +        p = &(*p)->next;
> +    *p = data;
> +    data->next = NULL;
> +}
> +
> +static av_cold void free_hdr10_plus(struct FrameHDR10PlusList *p)
> +{
> +    av_buffer_unref(&p->hdr10_plus);
> +    av_free(p);
> +}
> +
> +static av_cold void free_hdr10_plus_list(struct FrameHDR10PlusList *list)
> +{
> +    struct FrameHDR10PlusList *p = list;
> +    while (p) {
> +        list = list->next;
> +        free_hdr10_plus(p);
> +        p = list;
> +    }
> +}

This looks like it should be done with AVFifoBuffer.

> +
> +static int copy_hdr10_plus_to_pkt(void *list, AVPacket *pkt)
> +{
> +    struct FrameHDR10PlusList **p = list;
> +    struct FrameHDR10PlusList *head = *p;
> +
> +    if (head && pkt && head->hdr10_plus && head->pts == pkt->pts) {
> +        uint8_t *data;
> +        *p = (*p)->next;
> +        data = av_packet_new_side_data(pkt, AV_PKT_DATA_DYNAMIC_HDR10_PLUS, head->hdr10_plus->size);
> +
> +        if (!data) {
> +            free_hdr10_plus(head);
> +            return AVERROR(ENOMEM);
> +        }
> +        memcpy(data, head->hdr10_plus->data, head->hdr10_plus->size);
> +        free_hdr10_plus(head);
> +
> +    }
> +    return 0;
> +}
> +
>   static av_cold int codecctl_int(AVCodecContext *avctx,
>                                   enum vp8e_enc_control_id id, int val)
>   {
> @@ -383,6 +438,7 @@ static av_cold int vpx_free(AVCodecContext *avctx)
>       av_freep(&ctx->twopass_stats.buf);
>       av_freep(&avctx->stats_out);
>       free_frame_list(ctx->coded_frame_list);
> +    free_hdr10_plus_list(ctx->hdr10_plus_list);
>       return 0;
>   }
>   
> @@ -828,6 +884,7 @@ static av_cold int vpx_init(AVCodecContext *avctx,
>       AVCPBProperties *cpb_props;
>       int res;
>       vpx_img_fmt_t img_fmt = VPX_IMG_FMT_I420;
> +    ctx->discard_hdr10_plus = 1;
>   #if CONFIG_LIBVPX_VP9_ENCODER
>       vpx_codec_caps_t codec_caps = vpx_codec_get_caps(iface);
>       vpx_svc_extra_cfg_t svc_params;
> @@ -850,11 +907,16 @@ static av_cold int vpx_init(AVCodecContext *avctx,
>       if (avctx->codec_id == AV_CODEC_ID_VP9) {
>           if (set_pix_fmt(avctx, codec_caps, &enccfg, &flags, &img_fmt))
>               return AVERROR(EINVAL);
> +        // Keep HDR10+ if it has bit depth higher than 8 and
> +        // it has PQ trc (SMPTE2084).
> +        if (enccfg.g_bit_depth > 8 && avctx->color_trc == AVCOL_TRC_SMPTE2084) {
> +            ctx->discard_hdr10_plus = 0;
> +        }
>       }
>   #endif
>   
> -    if(!avctx->bit_rate)
> -        if(avctx->rc_max_rate || avctx->rc_buffer_size || avctx->rc_initial_buffer_occupancy) {
> +    if (!avctx->bit_rate)
> +        if (avctx->rc_max_rate || avctx->rc_buffer_size || avctx->rc_initial_buffer_occupancy) {
>               av_log( avctx, AV_LOG_ERROR, "Rate control parameters set without a bitrate\n");
>               return AVERROR(EINVAL);
>           }
> @@ -1245,6 +1307,14 @@ FF_ENABLE_DEPRECATION_WARNINGS
>               AV_WB64(side_data, 1);
>               memcpy(side_data + 8, cx_frame->buf_alpha, cx_frame->sz_alpha);
>           }
> +        if (cx_frame->frame_number != -1) {
> +            VPxContext *ctx = avctx->priv_data;
> +            if (!ctx->discard_hdr10_plus) {
> +                int err = copy_hdr10_plus_to_pkt(&ctx->hdr10_plus_list, pkt);
> +                if (err < 0)
> +                    return err;
> +            }
> +        }
>       } else {
>           return ret;
>       }
> @@ -1579,6 +1649,7 @@ static int vpx_encode(AVCodecContext *avctx, AVPacket *pkt,
>       const struct vpx_codec_enc_cfg *enccfg = ctx->encoder.config.enc;
>       vpx_svc_layer_id_t layer_id;
>       int layer_id_valid = 0;
> +    AVFrameSideData *hdr10_plus_metadata;
>   
>       if (frame) {
>           const AVFrameSideData *sd = av_frame_get_side_data(frame, AV_FRAME_DATA_REGIONS_OF_INTEREST);
> @@ -1655,6 +1726,19 @@ static int vpx_encode(AVCodecContext *avctx, AVPacket *pkt,
>                   vp9_encode_set_roi(avctx, frame->width, frame->height, sd);
>               }
>           }
> +
> +        if (!ctx->discard_hdr10_plus) {
> +            // Add HDR10+ metadata to queue.
> +            hdr10_plus_metadata = av_frame_get_side_data(frame, AV_FRAME_DATA_DYNAMIC_HDR_PLUS);
> +            if (hdr10_plus_metadata) {
> +                struct FrameHDR10PlusList *data =  av_malloc(sizeof(*data));
> +                if (!data)
> +                    return AVERROR(ENOMEM);
> +                data->pts = frame->pts;
> +                data->hdr10_plus = av_buffer_ref(hdr10_plus_metadata->buf);

Unchecked call.

> +                add_hdr10_plus(&ctx->hdr10_plus_list, data);
> +            }
> +        }

So you're propagating frame side data into the output packet side data, 
and the encoder proper never sees or uses any of it. This seems to be 
generic enough that it should be done in encode.c for all encoders 
instead, much like it's being done for decoding scenarios in decode.c

>       }
>   
>       // this is for encoding with preset temporal layering patterns defined in
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index ca18ae631f..23a146ea7b 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_HDR10_PLUS,

Missing an APIchanges entry for this enum value.

> +
>       /**
>        * The number of side data types.
>        * This is not part of the public API/ABI in the sense that it may
>

Answered inline:


On Tue, May 25, 2021 at 8:15 AM James Almer <jamrial@gmail.com> wrote:

> On 4/26/2021 10:54 PM, Mohammad Izadi wrote:
> > HDR10+ metadata is stored in the bit stream for HEVC. The story is
> different for VP9 and cannot store the metadata in the bit stream. HDR10+
> should be passed to packet side data an stored in the container (mkv) for
> VP9.
> >
> > This CL is taking HDR10+ from AVFrame side data in libvpxenc and is
> passing it to the AVPacket side data.
> > ---
> >   libavcodec/avpacket.c  |  1 +
> >   libavcodec/decode.c    |  1 +
> >   libavcodec/libvpxenc.c | 88 +++++++++++++++++++++++++++++++++++++++++-
> >   libavcodec/packet.h    |  8 ++++
> >   4 files changed, 96 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > index b5bac5c5f2..7a3b0a73e3 100644
> > --- a/libavcodec/avpacket.c
> > +++ b/libavcodec/avpacket.c
> > @@ -416,6 +416,7 @@ const char *av_packet_side_data_name(enum
> AVPacketSideDataType type)
> >       case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
> >       case AV_PKT_DATA_DOVI_CONF:                  return "DOVI
> configuration record";
> >       case AV_PKT_DATA_S12M_TIMECODE:              return "SMPTE ST
> 12-1:2014 timecode";
> > +    case AV_PKT_DATA_DYNAMIC_HDR10_PLUS:         return "HDR10+ Dynamic
> Metadata (SMPTE 2094-40)";
> >       }
> >       return NULL;
> >   }
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index 0956a6ac6f..bf5fbcca97 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -1736,6 +1736,7 @@ int ff_decode_frame_props(AVCodecContext *avctx,
> AVFrame *frame)
> >           { AV_PKT_DATA_A53_CC,                     AV_FRAME_DATA_A53_CC
> },
> >           { AV_PKT_DATA_ICC_PROFILE,
> AV_FRAME_DATA_ICC_PROFILE },
> >           { AV_PKT_DATA_S12M_TIMECODE,
> AV_FRAME_DATA_S12M_TIMECODE },
> > +        { AV_PKT_DATA_DYNAMIC_HDR10_PLUS,
>  AV_FRAME_DATA_DYNAMIC_HDR_PLUS },
>
> This has nothing to do with encoding.
>
Currently it has nothing to do with encoding. We extract HDR10+ in hevc
decoder and pass it through into MKV. In the future, we will ingest HDR10+
to bitstream in hevc and av1 encoder.

>
> >       };
> >
> >       if (IS_EMPTY(pkt) && av_fifo_size(avctx->internal->pkt_props) >=
> sizeof(*pkt))
> > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> > index 3f36943c12..2096c08437 100644
> > --- a/libavcodec/libvpxenc.c
> > +++ b/libavcodec/libvpxenc.c
> > @@ -63,6 +63,12 @@ struct FrameListData {
> >       struct FrameListData *next;
> >   };
> >
> > +typedef struct FrameHDR10PlusList {
> > +    int64_t pts;
> > +    AVBufferRef *hdr10_plus;
> > +    struct FrameHDR10PlusList *next;
> > +} FrameHDR10PlusList;
> > +
> >   typedef struct VPxEncoderContext {
> >       AVClass *class;
> >       struct vpx_codec_ctx encoder;
> > @@ -120,6 +126,8 @@ typedef struct VPxEncoderContext {
> >       int tune_content;
> >       int corpus_complexity;
> >       int tpl_model;
> > +    int discard_hdr10_plus;
> > +    struct FrameHDR10PlusList *hdr10_plus_list;
> >       /**
> >        * If the driver does not support ROI then warn the first time we
> >        * encounter a frame with ROI side data.
> > @@ -315,6 +323,53 @@ static av_cold void free_frame_list(struct
> FrameListData *list)
> >       }
> >   }
> >
> > +
> > +static void add_hdr10_plus(void *list, struct FrameHDR10PlusList *data)
> > +{
> > +    struct FrameHDR10PlusList **p = list;
> > +    while (*p)
> > +        p = &(*p)->next;
> > +    *p = data;
> > +    data->next = NULL;
> > +}
> > +
> > +static av_cold void free_hdr10_plus(struct FrameHDR10PlusList *p)
> > +{
> > +    av_buffer_unref(&p->hdr10_plus);
> > +    av_free(p);
> > +}
> > +
> > +static av_cold void free_hdr10_plus_list(struct FrameHDR10PlusList
> *list)
> > +{
> > +    struct FrameHDR10PlusList *p = list;
> > +    while (p) {
> > +        list = list->next;
> > +        free_hdr10_plus(p);
> > +        p = list;
> > +    }
> > +}
>
> This looks like it should be done with AVFifoBuffer.
>
Fixed.

>
> > +
> > +static int copy_hdr10_plus_to_pkt(void *list, AVPacket *pkt)
> > +{
> > +    struct FrameHDR10PlusList **p = list;
> > +    struct FrameHDR10PlusList *head = *p;
> > +
> > +    if (head && pkt && head->hdr10_plus && head->pts == pkt->pts) {
> > +        uint8_t *data;
> > +        *p = (*p)->next;
> > +        data = av_packet_new_side_data(pkt,
> AV_PKT_DATA_DYNAMIC_HDR10_PLUS, head->hdr10_plus->size);
> > +
> > +        if (!data) {
> > +            free_hdr10_plus(head);
> > +            return AVERROR(ENOMEM);
> > +        }
> > +        memcpy(data, head->hdr10_plus->data, head->hdr10_plus->size);
> > +        free_hdr10_plus(head);
> > +
> > +    }
> > +    return 0;
> > +}
> > +
> >   static av_cold int codecctl_int(AVCodecContext *avctx,
> >                                   enum vp8e_enc_control_id id, int val)
> >   {
> > @@ -383,6 +438,7 @@ static av_cold int vpx_free(AVCodecContext *avctx)
> >       av_freep(&ctx->twopass_stats.buf);
> >       av_freep(&avctx->stats_out);
> >       free_frame_list(ctx->coded_frame_list);
> > +    free_hdr10_plus_list(ctx->hdr10_plus_list);
> >       return 0;
> >   }
> >
> > @@ -828,6 +884,7 @@ static av_cold int vpx_init(AVCodecContext *avctx,
> >       AVCPBProperties *cpb_props;
> >       int res;
> >       vpx_img_fmt_t img_fmt = VPX_IMG_FMT_I420;
> > +    ctx->discard_hdr10_plus = 1;
> >   #if CONFIG_LIBVPX_VP9_ENCODER
> >       vpx_codec_caps_t codec_caps = vpx_codec_get_caps(iface);
> >       vpx_svc_extra_cfg_t svc_params;
> > @@ -850,11 +907,16 @@ static av_cold int vpx_init(AVCodecContext *avctx,
> >       if (avctx->codec_id == AV_CODEC_ID_VP9) {
> >           if (set_pix_fmt(avctx, codec_caps, &enccfg, &flags, &img_fmt))
> >               return AVERROR(EINVAL);
> > +        // Keep HDR10+ if it has bit depth higher than 8 and
> > +        // it has PQ trc (SMPTE2084).
> > +        if (enccfg.g_bit_depth > 8 && avctx->color_trc ==
> AVCOL_TRC_SMPTE2084) {
> > +            ctx->discard_hdr10_plus = 0;
> > +        }
> >       }
> >   #endif
> >
> > -    if(!avctx->bit_rate)
> > -        if(avctx->rc_max_rate || avctx->rc_buffer_size ||
> avctx->rc_initial_buffer_occupancy) {
> > +    if (!avctx->bit_rate)
> > +        if (avctx->rc_max_rate || avctx->rc_buffer_size ||
> avctx->rc_initial_buffer_occupancy) {
> >               av_log( avctx, AV_LOG_ERROR, "Rate control parameters set
> without a bitrate\n");
> >               return AVERROR(EINVAL);
> >           }
> > @@ -1245,6 +1307,14 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >               AV_WB64(side_data, 1);
> >               memcpy(side_data + 8, cx_frame->buf_alpha,
> cx_frame->sz_alpha);
> >           }
> > +        if (cx_frame->frame_number != -1) {
> > +            VPxContext *ctx = avctx->priv_data;
> > +            if (!ctx->discard_hdr10_plus) {
> > +                int err = copy_hdr10_plus_to_pkt(&ctx->hdr10_plus_list,
> pkt);
> > +                if (err < 0)
> > +                    return err;
> > +            }
> > +        }
> >       } else {
> >           return ret;
> >       }
> > @@ -1579,6 +1649,7 @@ static int vpx_encode(AVCodecContext *avctx,
> AVPacket *pkt,
> >       const struct vpx_codec_enc_cfg *enccfg = ctx->encoder.config.enc;
> >       vpx_svc_layer_id_t layer_id;
> >       int layer_id_valid = 0;
> > +    AVFrameSideData *hdr10_plus_metadata;
> >
> >       if (frame) {
> >           const AVFrameSideData *sd = av_frame_get_side_data(frame,
> AV_FRAME_DATA_REGIONS_OF_INTEREST);
> > @@ -1655,6 +1726,19 @@ static int vpx_encode(AVCodecContext *avctx,
> AVPacket *pkt,
> >                   vp9_encode_set_roi(avctx, frame->width, frame->height,
> sd);
> >               }
> >           }
> > +
> > +        if (!ctx->discard_hdr10_plus) {
> > +            // Add HDR10+ metadata to queue.
> > +            hdr10_plus_metadata = av_frame_get_side_data(frame,
> AV_FRAME_DATA_DYNAMIC_HDR_PLUS);
> > +            if (hdr10_plus_metadata) {
> > +                struct FrameHDR10PlusList *data =
> av_malloc(sizeof(*data));
> > +                if (!data)
> > +                    return AVERROR(ENOMEM);
> > +                data->pts = frame->pts;
> > +                data->hdr10_plus =
> av_buffer_ref(hdr10_plus_metadata->buf);
>
> Unchecked call.
>
Fixed.

>
> > +                add_hdr10_plus(&ctx->hdr10_plus_list, data);
> > +            }
> > +        }
>
> So you're propagating frame side data into the output packet side data,
> and the encoder proper never sees or uses any of it. This seems to be
> generic enough that it should be done in encode.c for all encoders
> instead, much like it's being done for decoding scenarios in decode.c
>
Not really. Only for VP9 we need to do it. Other encoders (like H265, AV1)
do not transfer to the output packet side data and they instead write
HDR10+ in the bitstream (SEI message).

>
> >       }
> >
> >       // this is for encoding with preset temporal layering patterns
> defined in
> > diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> > index ca18ae631f..23a146ea7b 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_HDR10_PLUS,
>
> Missing an APIchanges entry for this enum value.
>
Added

>
> > +
> >       /**
> >        * The number of side data types.
> >        * This is not part of the public API/ABI in the sense that it may
> >
>
> _______________________________________________
> 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".
>