[FFmpeg-devel] [PATCH 2/4] vaapi_encode_h265: Insert mastering display colour colume if needed

Xiang, Haihao haihao.xiang at intel.com
Wed May 9 07:48:13 EEST 2018


On Tue, 2018-05-08 at 22:51 +0100, Mark Thompson wrote:
> On 08/05/18 04:54, Xiang, Haihao wrote:
> > On Mon, 2018-05-07 at 22:03 +0100, Mark Thompson wrote:
> > > On 04/05/18 09:54, Xiang, Haihao wrote:
> > > > On Thu, 2018-05-03 at 22:43 +0100, Mark Thompson wrote:
> > > > > On 03/05/18 04:07, Haihao Xiang wrote:
> > > > > > '-sei xxx' is added to control SEI insertion, so far only mastering
> > > > > > display colour colume is available for testing.
> > > > > 
> > > > > Typo: "colume" (also in the commit title).
> > > > > 
> > > > 
> > > > Thanks for catching the typo, I will correct it in the new version of
> > > > patch.
> > > > 
> > > > > > v2: use the mastering display parameters from
> > > > > > AVMasteringDisplayMetadata, set SEI_MASTERING_DISPLAY to 8 to match
> > > > > > the H.264 part and take VAAPIEncodeH265Context::sei_needed as a ORed
> > > > > > value so that we needn't check the correspoding SEI message again
> > > > > > when
> > > > > > writting the header.
> > > > > > 
> > > > > > Signed-off-by: Haihao Xiang <haihao.xiang at intel.com>
> > > > > > ---
> > > > > >  libavcodec/vaapi_encode_h265.c | 128
> > > > > > ++++++++++++++++++++++++++++++++++++++++-
> > > > > >  1 file changed, 127 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/libavcodec/vaapi_encode_h265.c
> > > > > > b/libavcodec/vaapi_encode_h265.c
> > > > > > index 5203c6871d..326fe4fe66 100644
> > > > > > --- a/libavcodec/vaapi_encode_h265.c
> > > > > > +++ b/libavcodec/vaapi_encode_h265.c
> > > > > > @@ -24,15 +24,20 @@
> > > > > >  #include "libavutil/avassert.h"
> > > > > >  #include "libavutil/common.h"
> > > > > >  #include "libavutil/opt.h"
> > > > > > +#include "libavutil/mastering_display_metadata.h"
> > > > > >  
> > > > > >  #include "avcodec.h"
> > > > > >  #include "cbs.h"
> > > > > >  #include "cbs_h265.h"
> > > > > >  #include "hevc.h"
> > > > > > +#include "hevc_sei.h"
> > > > > >  #include "internal.h"
> > > > > >  #include "put_bits.h"
> > > > > >  #include "vaapi_encode.h"
> > > > > >  
> > > > > > +enum {
> > > > > > +    SEI_MASTERING_DISPLAY       = 0x08,
> > > > > > +};
> > > > > >  
> > > > > >  typedef struct VAAPIEncodeH265Context {
> > > > > >      unsigned int ctu_width;
> > > > > > @@ -47,6 +52,9 @@ typedef struct VAAPIEncodeH265Context {
> > > > > >      H265RawSPS sps;
> > > > > >      H265RawPPS pps;
> > > > > >      H265RawSlice slice;
> > > > > > +    H265RawSEI sei;
> > > > > > +
> > > > > > +    H265RawSEIMasteringDiplayColourVolume mastering_display;
> > > > > >  
> > > > > >      int64_t last_idr_frame;
> > > > > >      int pic_order_cnt;
> > > > > > @@ -58,6 +66,7 @@ typedef struct VAAPIEncodeH265Context {
> > > > > >      CodedBitstreamContext *cbc;
> > > > > >      CodedBitstreamFragment current_access_unit;
> > > > > >      int aud_needed;
> > > > > > +    int sei_needed;
> > > > > >  } VAAPIEncodeH265Context;
> > > > > >  
> > > > > >  typedef struct VAAPIEncodeH265Options {
> > > > > > @@ -65,6 +74,7 @@ typedef struct VAAPIEncodeH265Options {
> > > > > >      int aud;
> > > > > >      int profile;
> > > > > >      int level;
> > > > > > +    int sei;
> > > > > >  } VAAPIEncodeH265Options;
> > > > > >  
> > > > > >  
> > > > > > @@ -175,6 +185,64 @@ fail:
> > > > > >      return err;
> > > > > >  }
> > > > > >  
> > > > > > +static int vaapi_encode_h265_write_extra_header(AVCodecContext
> > > > > > *avctx,
> > > > > > +                                                VAAPIEncodePicture
> > > > > > *pic,
> > > > > > +                                                int index, int
> > > > > > *type,
> > > > > > +                                                char *data, size_t
> > > > > > *data_len)
> > > > > > +{
> > > > > > +    VAAPIEncodeContext      *ctx = avctx->priv_data;
> > > > > > +    VAAPIEncodeH265Context *priv = ctx->priv_data;
> > > > > > +    CodedBitstreamFragment   *au = &priv->current_access_unit;
> > > > > > +    int err, i;
> > > > > > +
> > > > > > +    if (priv->sei_needed) {
> > > > > > +        if (priv->aud_needed) {
> > > > > > +            err = vaapi_encode_h265_add_nal(avctx, au, &priv->aud);
> > > > > > +            if (err < 0)
> > > > > > +                goto fail;
> > > > > > +            priv->aud_needed = 0;
> > > > > > +        }
> > > > > > +
> > > > > > +        memset(&priv->sei, 0, sizeof(priv->sei));
> > > > > > +        priv->sei.nal_unit_header  = (H265RawNALUnitHeader) {
> > > > > > +            .nal_unit_type         = HEVC_NAL_SEI_PREFIX,
> > > > > > +            .nuh_layer_id          = 0,
> > > > > > +            .nuh_temporal_id_plus1 = 1,
> > > > > > +        };
> > > > > > +
> > > > > > +        i = 0;
> > > > > > +
> > > > > > +        if (priv->sei_needed & SEI_MASTERING_DISPLAY) {
> > > > > > +            priv->sei.payload[i].payload_type =
> > > > > > HEVC_SEI_TYPE_MASTERING_DISPLAY_INFO;
> > > > > > +            priv->sei.payload[i].payload.mastering_display = priv-
> > > > > > > mastering_display;
> > > > > > 
> > > > > > +            ++i;
> > > > > > +        }
> > > > > > +
> > > > > > +        priv->sei.payload_count = i;
> > > > > > +        av_assert0(priv->sei.payload_count > 0);
> > > > > > +
> > > > > > +        err = vaapi_encode_h265_add_nal(avctx, au, &priv->sei);
> > > > > > +        if (err < 0)
> > > > > > +            goto fail;
> > > > > > +        priv->sei_needed = 0;
> > > > > > +
> > > > > > +        err = vaapi_encode_h265_write_access_unit(avctx, data,
> > > > > > data_len,
> > > > > > au);
> > > > > > +        if (err < 0)
> > > > > > +            goto fail;
> > > > > > +
> > > > > > +        ff_cbs_fragment_uninit(priv->cbc, au);
> > > > > > +
> > > > > > +        *type = VAEncPackedHeaderRawData;
> > > > > > +        return 0;
> > > > > > +    } else {
> > > > > > +        return AVERROR_EOF;
> > > > > > +    }
> > > > > > +
> > > > > > +fail:
> > > > > > +    ff_cbs_fragment_uninit(priv->cbc, au);
> > > > > > +    return err;
> > > > > > +}
> > > > > > +
> > > > > >  static int vaapi_encode_h265_init_sequence_params(AVCodecContext
> > > > > > *avctx)
> > > > > >  {
> > > > > >      VAAPIEncodeContext                *ctx = avctx->priv_data;
> > > > > > @@ -587,6 +655,53 @@ static int
> > > > > > vaapi_encode_h265_init_picture_params(AVCodecContext *avctx,
> > > > > >          priv->aud_needed = 0;
> > > > > >      }
> > > > > >  
> > > > > > +    priv->sei_needed = 0;
> > > > > > +
> > > > > > +    if ((opt->sei & SEI_MASTERING_DISPLAY) &&
> > > > > > +        (pic->type == PICTURE_TYPE_I || pic->type ==
> > > > > > PICTURE_TYPE_IDR))
> > > > > > {
> > > > > > +        AVFrameSideData *sd =
> > > > > > +            av_frame_get_side_data(pic->input_image,
> > > > > > +                                   AV_FRAME_DATA_MASTERING_DISPLAY_
> > > > > > META
> > > > > > DATA
> > > > > > );
> > > > > 
> > > > > Do you know when this side-data might be updated - can it arrive on a
> > > > > random
> > > > > frame in the middle of the stream?  (And if so, what should we do
> > > > > about
> > > > > it?)
> > > > > 
> > > > 
> > > > According the doc below, I think it is reasonable to check this side-
> > > > data
> > > > for
> > > > I/IDR frame only. I also checked some HDR streams and this side-data is
> > > > added
> > > > for I/IDR frame. 
> > > > 
> > > > "When a mastering display colour volume SEI message is present for any
> > > > picture
> > > > of a CLVS of a particular layer, a mastering display colour volume SEI
> > > > message
> > > > shall be present for the first picture of the CLVS. The mastering
> > > > display
> > > > colour
> > > > volume SEI message persists for the current layer in decoding order from
> > > > the
> > > > current picture until the end of the CLVS. All mastering display colour
> > > > volume
> > > > SEI messages that apply to the same CLVS shall have the same content."
> > > 
> > > Right - that implies you want to write them for intra frames, but it
> > > doesn't
> > > say where they will be present on the input to the encoder.
> > > 
> > > For example, suppose we are doing a simple transcode of an H.265 input
> > > which
> > > has this metadata, and it has 60-frame GOP size.  So, there might be
> > > changes
> > > to the metadata on every 60th frame of the input to the encoder. 
> > 
> > Yes.
> > 
> > >  If we only look for the metadata on each frame which is going to be an
> > > intra
> > > frame on the output then we might miss some changes which are on frames
> > > which
> > > were intra on the input but we aren't encoding them as intra on the
> > > output.  Does that make sense?
> > 
> > Do you mean the input and output have different GOP size, so that maybe a
> > frame
> > is intra on the output but it is not intra on the input? if so, how about
> > writing the latest metadata from intra frame on the input to intra frame on
> > the
> > output?
> 
> Having thought about this a bit further, I think that would be the best answer
> here for now.
> 
> To do better we need the ability to notice the change and start a new CLVS
> with a new IDR frame - I'm looking at this anyway along with the reference
> structure stuff, since there are other settings which want the same behaviour
> (SAR and colour properties carried by the AVFrame).

I think we may do the above things in a new patch in future, e.g. check the
side-data of the AVFrame before vaapi_encode_get_next() and set a forced IDR via
ctx->force_idr. 

It seems we can't know whether an input is intra via AVFrame.pict_type, do you
have any idea?

Thanks
Haihao


> 
> > > > > > +
> > > > > > +        if (sd) {
> > > > > > +            AVMasteringDisplayMetadata *mdm =
> > > > > > +                (AVMasteringDisplayMetadata *)sd->data;
> > > > > > +
> > > > > > +            if (mdm->has_primaries && mdm->has_luminance) {
> > > > > > +                const int mapping[3] = {1, 2, 0};
> > > > > > +                const int chroma_den = 50000;
> > > > > > +                const int luma_den   = 10000;
> > > > > > +
> > > > > > +                for (i = 0; i < 3; i++) {
> > > > > > +                    const int j = mapping[i];
> > > > > > +                    priv->mastering_display.display_primaries_x[i]
> > > > > > =
> > > > > > +                        FFMIN(lrint(chroma_den *
> > > > > > +                                    av_q2d(mdm-
> > > > > > > display_primaries[j][0])),
> > > > > > 
> > > > > > +                              50000);
> > > > > > +                    priv->mastering_display.display_primaries_y[i]
> > > > > > =
> > > > > > +                        FFMIN(lrint(chroma_den *
> > > > > > +                                    av_q2d(mdm-
> > > > > > > display_primaries[j][1])),
> > > > > > 
> > > > > > +                              50000);
> > > > > > +                }
> > > > > > +
> > > > > > +                priv->mastering_display.white_point_x =
> > > > > > +                    FFMIN(lrint(chroma_den * av_q2d(mdm-
> > > > > > > white_point[0])),
> > > > > > 
> > > > > > +                          50000);
> > > > > > +                priv->mastering_display.white_point_y =
> > > > > > +                    FFMIN(lrint(chroma_den * av_q2d(mdm-
> > > > > > > white_point[1])),
> > > > > > 
> > > > > > +                          50000);
> > > > > > +
> > > > > > +                priv-
> > > > > > >mastering_display.max_display_mastering_luminance 
> > > > > > =
> > > > > > +                    lrint(luma_den * av_q2d(mdm->max_luminance));
> > > > > > +                priv-
> > > > > > >mastering_display.min_display_mastering_luminance 
> > > > > > =
> > > > > > +                    FFMIN(lrint(luma_den * av_q2d(mdm-
> > > > > > >min_luminance)),
> > > > > > +                          priv-
> > > > > > > mastering_display.max_display_mastering_luminance);
> > > > > > 
> > > > > > +
> > > > > > +                priv->sei_needed |= SEI_MASTERING_DISPLAY;
> > > > > > +            }
> > > > > > +        }
> > > > > 
> > > > > There are has_primaries and has_luminance fields in
> > > > > AVMasteringDisplayMetadata
> > > > > - do they need to be checked here?  If they don't matter then please
> > > > > add a
> > > > > comment to that effect.
> > > > 
> > > > I think user may use has_primaries and has_luminance to indicate the
> > > > side-
> > > > data
> > > > is valid or not.
> > > 
> > > Hmm.  Presumably we only get this structure if at least one of them is
> > > set.  Suppose one is valid and the other is not - what then?  Can we write
> > > some default values?  (Are what are the right default values?  Unlike with
> > > content-light-level there isn't a zero value to avoid the problem...)
> > 
> > It seems the spec doesn't define the default values, so currently the
> > metadata
> > is written when both of them are set, otherwise the metadata is ignored. 
> 
> Ok, fair enough.
> 
> > > > > > +    }
> > > > > > +
> > > > > >      vpic->decoded_curr_pic = (VAPictureHEVC) {
> > > > > >          .picture_id    = pic->recon_surface,
> > > > > >          .pic_order_cnt = priv->pic_order_cnt,
> > > > > > @@ -895,6 +1010,8 @@ static const VAAPIEncodeType
> > > > > > vaapi_encode_type_h265
> > > > > > = {
> > > > > >  
> > > > > >      .slice_header_type     = VAEncPackedHeaderHEVC_Slice,
> > > > > >      .write_slice_header    = &vaapi_encode_h265_write_slice_header,
> > > > > > +
> > > > > > +    .write_extra_header    = &vaapi_encode_h265_write_extra_header,
> > > > > >  };
> > > > > >  
> > > > > >  static av_cold int vaapi_encode_h265_init(AVCodecContext *avctx)
> > > > > > @@ -943,7 +1060,8 @@ static av_cold int
> > > > > > vaapi_encode_h265_init(AVCodecContext *avctx)
> > > > > >  
> > > > > >      ctx->va_packed_headers =
> > > > > >          VA_ENC_PACKED_HEADER_SEQUENCE | // VPS, SPS and PPS.
> > > > > > -        VA_ENC_PACKED_HEADER_SLICE;     // Slice headers.
> > > > > > +        VA_ENC_PACKED_HEADER_SLICE    | // Slice headers.
> > > > > > +        VA_ENC_PACKED_HEADER_MISC;      // SEI
> > > > > >  
> > > > > >      ctx->surface_width  = FFALIGN(avctx->width,  16);
> > > > > >      ctx->surface_height = FFALIGN(avctx->height, 16);
> > > > > > @@ -1003,6 +1121,14 @@ static const AVOption
> > > > > > vaapi_encode_h265_options[]
> > > > > > = {
> > > > > >      { LEVEL("6.2", 186) },
> > > > > >  #undef LEVEL
> > > > > >  
> > > > > > +    { "sei", "Set SEI to include",
> > > > > > +      OFFSET(sei), AV_OPT_TYPE_FLAGS,
> > > > > > +      { .i64 = SEI_MASTERING_DISPLAY },
> > > > > > +      0, INT_MAX, FLAGS, "sei" },
> > > > > > +    { "mastering_display", "Include mastering display colour
> > > > > > volume",
> > > > > > +      0, AV_OPT_TYPE_CONST, { .i64 = SEI_MASTERING_DISPLAY },
> > > > > > +      INT_MIN, INT_MAX, FLAGS, "sei" },
> > > > > > +
> > > > > >      { NULL },
> > > > > >  };
> > > > > >  
> > > > > > 
> > > > > 
> > > > > Ignoring the mastering display part, all the SEI logic looks good.
> 
> Thanks,
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list