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

'-sei xxx' is added to control SEI insertion, so far only mastering
display colour colume is available for testing.

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@intel.com>
---
 libavcodec/vaapi_encode_h265.c | 128 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 127 insertions(+), 1 deletion(-)

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).

> 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@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_METADATA);

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?)

> +
> +        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.

> +    }
> +
>      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

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@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_METADATA

> > );

> 

> 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."

> > +

> > +        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.


> 

> > +    }

> > +

> >      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@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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@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_METADATA
>>> );
>>
>> 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.  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?

>>> +
>>> +        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...)

>>> +    }
>>> +
>>>      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

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@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?


> 

> > > > +

> > > > +        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. 

> 

> > > > +    }

> > > > +

> > > >      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@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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@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).

>>>>> +
>>>>> +        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

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@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@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

On 09/05/18 05:48, Xiang, Haihao wrote:
> 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@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. 

Yes, that is what I was thinking.  The side data can get checked at the same time as other possibly-changing parameters (like aspect ratio), and a new IDR frame / CLVS forced if it changes from what we are currently using.

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

Correct, this isn't visible here.  AVFrame.pict_type can be set by a libavcodec user to force key frames in particular places if it wants, but otherwise is unused on input to the encoder.

- Mark

On Wed, 2018-05-09 at 14:53 +0100, Mark Thompson wrote:
> On 09/05/18 05:48, Xiang, Haihao wrote:

> > 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@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,

> > > > > > > > +                                                VAAPIEncodePict

> > > > > > > > ure

> > > > > > > > *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_DISP

> > > > > > > > LAY_

> > > > > > > > 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. 

> 

> Yes, that is what I was thinking.  The side data can get checked at the same

> time as other possibly-changing parameters (like aspect ratio), and a new IDR

> frame / CLVS forced if it changes from what we are currently using.

> 

> > It seems we can't know whether an input is intra via AVFrame.pict_type, do

> > you

> > have any idea?

> 

> Correct, this isn't visible here.  AVFrame.pict_type can be set by a

> libavcodec user to force key frames in particular places if it wants, but

> otherwise is unused on input to the encoder.

> 


Ok, I will still use the original logic which should work when we force an IDR
frame for the matadata changes, I will add some comments as well in the new
version of patch.

Thanks
Haihao


> - Mark

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel