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

[Mohammad Izadi]

On Fri, Apr 23, 2021 at 11:53 AM James Zern <jzern-at-google.com at ffmpeg.org>
wrote:

> Hi,
>
> On Fri, Apr 23, 2021 at 8:58 AM Mohammad Izadi
> <izadi-at-google.com at ffmpeg.org> 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 | 71 ++++++++++++++++++++++++++++++++++++++++++
> >  libavcodec/packet.h    | 10 +++++-
> >  4 files changed, 82 insertions(+), 1 deletion(-)
> >
>
> Just some quick cosmetics, I didn't take a close look at the
> implementation.
>
> > [...]
> > +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) {
>
> Add whitespace after 'if' and elsewhere before the '{'. You can use
> tools/patcheck as a guide to the formatting [1].
>
*Fixed.*

>
> > [...]
> >
> > @@ -1245,6 +1300,12 @@ 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) { // Not invisible frame
>
> VP9 won't emit frames like this, but this is a shared path with VP8. Can
> we get
> this metadata (unnecessarily) when doing a 8-bit encode?
>
*Great point! planned to discard it in matroska, but we can discard HDR10+
if it is PQ HDR (high bit depth and SPMTE2084). We shouldn't get it. Fixed
in the code.*

> > +            VPxContext *ctx = avctx->priv_data;
> > +            int err = copy_hdr10_plus_to_pkt(&ctx->hdr10_plus_list,
> pkt);
> > +            if (err < 0)
> > +                return err;
> > +        }
> >      } else {
> >          return ret;
> >      }
> > @@ -1579,6 +1640,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 +1717,15 @@ static int vpx_encode(AVCodecContext *avctx,
> AVPacket *pkt,
> >                  vp9_encode_set_roi(avctx, frame->width, frame->height,
> sd);
> >              }
> >          }
> > +
> > +        // 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(struct
> FrameHDR10PlusList));
>
> This allocation should be checked and prefer sizeof(*data).
>
*Fixed.*

>
> > +            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..89e683b357 100644
> > --- a/libavcodec/packet.h
> > +++ b/libavcodec/packet.h
> > @@ -298,7 +298,15 @@ enum AVPacketSideDataType {
> >       * If its value becomes huge, some code using it
> >       * needs to be updated as it assumes it to be smaller than other
> limits.
> >       */
> > -    AV_PKT_DATA_NB
> > +    AV_PKT_DATA_NB,
>
> This should be the final entry in the enum, see the comment above.
>
*Fixed.*

>

>
> > +
> > +    /**
> > +     * 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
> >  };
> >
> >  #define AV_PKT_DATA_QUALITY_FACTOR AV_PKT_DATA_QUALITY_STATS
> //DEPRECATED
>
> [1] https://ffmpeg.org/developer.html#Coding-Rules-1
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>