On Fri, Apr 23, 2021 at 11:53 AM James Zern <jzern-at-google....@ffmpeg.org>
wrote:
> Hi,
>
> On Fri, Apr 23, 2021 at 8:58 AM Mohammad Izadi
> <izadi-at-google....@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
