| Message ID | 20180420072747.15054-2-haihao.xiang@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On 20/04/18 08:27, Haihao Xiang wrote: > '-sei xxx' is added to control SEI insertion, so far only mastering > display colour colume is available for testing. Another patch is > required to change mastering display settings later. > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > --- > libavcodec/vaapi_encode_h265.c | 94 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 93 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c > index 5203c6871d..a8cb6a6d8b 100644 > --- a/libavcodec/vaapi_encode_h265.c > +++ b/libavcodec/vaapi_encode_h265.c > @@ -29,10 +29,14 @@ > #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 = 0x01, Since the options are essentially the same I think it would be nice if these values matched the H.264 ones? (That is, make this value 8.) Should this be mastering display specifically, or would it be better for this option to be called something like "HDR metadata" (just "hdr" as the option name?) which also includes content light level? (Compare the -sei timing option on H.264, which gives you both buffering period and picture timing SEI.) > +}; > > typedef struct VAAPIEncodeH265Context { > unsigned int ctu_width; > @@ -47,6 +51,9 @@ typedef struct VAAPIEncodeH265Context { > H265RawSPS sps; > H265RawPPS pps; > H265RawSlice slice; > + H265RawSEI sei; > + > + H265RawSEIMasteringDiplayColorVolume mastering_display; > > int64_t last_idr_frame; > int pic_order_cnt; > @@ -58,6 +65,7 @@ typedef struct VAAPIEncodeH265Context { > CodedBitstreamContext *cbc; > CodedBitstreamFragment current_access_unit; > int aud_needed; > + int sei_needed; > } VAAPIEncodeH265Context; > > typedef struct VAAPIEncodeH265Options { > @@ -65,6 +73,7 @@ typedef struct VAAPIEncodeH265Options { > int aud; > int profile; > int level; > + int sei; > } VAAPIEncodeH265Options; > > > @@ -175,6 +184,61 @@ 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; > + VAAPIEncodeH265Options *opt = ctx->codec_options; > + 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.nal_unit_type = HEVC_NAL_SEI_PREFIX; > + priv->sei.nal_unit_header.nuh_temporal_id_plus1 = 1; Might look nicer as a compound literal? > + i = 0; > + > + if (opt->sei & SEI_MASTERING_DISPLAY && pic->type == PICTURE_TYPE_IDR) { > + 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 +651,23 @@ static int vaapi_encode_h265_init_picture_params(AVCodecContext *avctx, > priv->aud_needed = 0; > } > > + if ((opt->sei & SEI_MASTERING_DISPLAY) && > + (pic->type == PICTURE_TYPE_I || pic->type == PICTURE_TYPE_IDR)) { > + // hard-coded value for testing, change it later > + for (i = 0; i < 3; i++) { > + priv->mastering_display.display_primaries[i].x = 50000; > + priv->mastering_display.display_primaries[i].y = 50000; > + } > + > + priv->mastering_display.white_point_x = 50000; > + priv->mastering_display.white_point_y = 50000; > + > + priv->mastering_display.max_display_mastering_luminance = 5000; > + priv->mastering_display.min_display_mastering_luminance = 1000; > + > + priv->sei_needed = 1; > + } Obviously this needs to contain real data before it can be applied. Is it actually going to work as part of the sequence parameters? The mastering display metadata side-data which presumably will be the input for this is per-frame. > + > vpic->decoded_curr_pic = (VAPictureHEVC) { > .picture_id = pic->recon_surface, > .pic_order_cnt = priv->pic_order_cnt, > @@ -895,6 +976,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 +1026,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 +1087,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 = 0 }, > + 0, INT_MAX, FLAGS, "sei" }, If actual inclusion is conditional on the side-data being present on the input then it probably makes sense for it to be on by default. > + { "mastering_display", "Include mastering display colour volumne", > + 0, AV_OPT_TYPE_CONST, { .i64 = SEI_MASTERING_DISPLAY }, > + INT_MIN, INT_MAX, FLAGS, "sei" }, > + > { NULL }, > }; > > Thanks, - Mark
> On 20/04/18 08:27, Haihao Xiang wrote: > > '-sei xxx' is added to control SEI insertion, so far only mastering > > display colour colume is available for testing. Another patch is > > required to change mastering display settings later. > > > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > > --- > > libavcodec/vaapi_encode_h265.c | 94 > > +++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 93 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c > > index 5203c6871d..a8cb6a6d8b 100644 > > --- a/libavcodec/vaapi_encode_h265.c > > +++ b/libavcodec/vaapi_encode_h265.c > > @@ -29,10 +29,14 @@ > > #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 = 0x01, > > Since the options are essentially the same I think it would be nice if these > values matched the H.264 ones? (That is, make this value 8.) > My original thought was to make this value 8, but I am not sure whether another SEI_XXX will be added for H.264 in the future. Will we use 8 for a new SEI_XXX for H.264? > Should this be mastering display specifically, or would it be better for this > option to be called something like "HDR metadata" (just "hdr" as the option > name?) which also includes content light level? (Compare the -sei timing > option on H.264, which gives you both buffering period and picture timing > SEI.) > Good idea, I prefer using hdr for mastering display and content light level. Actually adding support for content light level is in my todo list. > > +}; > > > > typedef struct VAAPIEncodeH265Context { > > unsigned int ctu_width; > > @@ -47,6 +51,9 @@ typedef struct VAAPIEncodeH265Context { > > H265RawSPS sps; > > H265RawPPS pps; > > H265RawSlice slice; > > + H265RawSEI sei; > > + > > + H265RawSEIMasteringDiplayColorVolume mastering_display; > > > > int64_t last_idr_frame; > > int pic_order_cnt; > > @@ -58,6 +65,7 @@ typedef struct VAAPIEncodeH265Context { > > CodedBitstreamContext *cbc; > > CodedBitstreamFragment current_access_unit; > > int aud_needed; > > + int sei_needed; > > } VAAPIEncodeH265Context; > > > > typedef struct VAAPIEncodeH265Options { > > @@ -65,6 +73,7 @@ typedef struct VAAPIEncodeH265Options { > > int aud; > > int profile; > > int level; > > + int sei; > > } VAAPIEncodeH265Options; > > > > > > @@ -175,6 +184,61 @@ 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; > > + VAAPIEncodeH265Options *opt = ctx->codec_options; > > + 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.nal_unit_type = HEVC_NAL_SEI_PREFIX; > > + priv->sei.nal_unit_header.nuh_temporal_id_plus1 = 1; > > Might look nicer as a compound literal? Agree, I will update the patch. > > > + i = 0; > > + > > + if (opt->sei & SEI_MASTERING_DISPLAY && pic->type == > > PICTURE_TYPE_IDR) { > > + 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 +651,23 @@ static int > > vaapi_encode_h265_init_picture_params(AVCodecContext *avctx, > > priv->aud_needed = 0; > > } > > > > + if ((opt->sei & SEI_MASTERING_DISPLAY) && > > + (pic->type == PICTURE_TYPE_I || pic->type == PICTURE_TYPE_IDR)) { > > + // hard-coded value for testing, change it later > > + for (i = 0; i < 3; i++) { > > + priv->mastering_display.display_primaries[i].x = 50000; > > + priv->mastering_display.display_primaries[i].y = 50000; > > + } > > + > > + priv->mastering_display.white_point_x = 50000; > > + priv->mastering_display.white_point_y = 50000; > > + > > + priv->mastering_display.max_display_mastering_luminance = 5000; > > + priv->mastering_display.min_display_mastering_luminance = 1000; > > + > > + priv->sei_needed = 1; > > + } > > Obviously this needs to contain real data before it can be applied. > Yes. Currently I have no idea how does a user pass these data to hevc_vaapi encoder in FFmpeg. Is adding new options for these data acceptable? > Is it actually going to work as part of the sequence parameters? The > mastering display metadata side-data which presumably will be the input for > this is per-frame. > Do you mean a part of the sequence parameters for VAAPI? The vaapi driver doesn't require these data. > > + > > vpic->decoded_curr_pic = (VAPictureHEVC) { > > .picture_id = pic->recon_surface, > > .pic_order_cnt = priv->pic_order_cnt, > > @@ -895,6 +976,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 +1026,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 +1087,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 = 0 }, > > + 0, INT_MAX, FLAGS, "sei" }, > > If actual inclusion is conditional on the side-data being present on the input > then it probably makes sense for it to be on by default. > If setting the default value to 1, mastering display will be added even if '-sei xxx' is not set in the command line. Does it make sense? > > + { "mastering_display", "Include mastering display colour volumne", > > + 0, AV_OPT_TYPE_CONST, { .i64 = SEI_MASTERING_DISPLAY }, > > + INT_MIN, INT_MAX, FLAGS, "sei" }, > > + > > { NULL }, > > }; > > > > > > Thanks, > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 23/04/18 08:08, Xiang, Haihao wrote:>> On 20/04/18 08:27, Haihao Xiang wrote: >>> '-sei xxx' is added to control SEI insertion, so far only mastering >>> display colour colume is available for testing. Another patch is >>> required to change mastering display settings later. >>> >>> Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> >>> --- >>> libavcodec/vaapi_encode_h265.c | 94 >>> +++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 93 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c >>> index 5203c6871d..a8cb6a6d8b 100644 >>> --- a/libavcodec/vaapi_encode_h265.c >>> +++ b/libavcodec/vaapi_encode_h265.c >>> @@ -29,10 +29,14 @@ >>> #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 = 0x01, >> >> Since the options are essentially the same I think it would be nice if these >> values matched the H.264 ones? (That is, make this value 8.) >> > > My original thought was to make this value 8, but I am not sure whether another > SEI_XXX will be added for H.264 in the future. Will we use 8 for a new SEI_XXX > for H.264? I think that's fine - if we do add anything new for H.264 then it can have another value. It would also be possible to add support for the HDR metadata to the H.264 code, though it's probably not of much use here when hardware isn't going to support >8-bit video. >> Should this be mastering display specifically, or would it be better for this >> option to be called something like "HDR metadata" (just "hdr" as the option >> name?) which also includes content light level? (Compare the -sei timing >> option on H.264, which gives you both buffering period and picture timing >> SEI.) >> > > Good idea, I prefer using hdr for mastering display and content light level. > Actually adding support for content light level is in my todo list. Ok, nice :) >>> +}; >>> >>> typedef struct VAAPIEncodeH265Context { >>> unsigned int ctu_width; >>> @@ -47,6 +51,9 @@ typedef struct VAAPIEncodeH265Context { >>> H265RawSPS sps; >>> H265RawPPS pps; >>> H265RawSlice slice; >>> + H265RawSEI sei; >>> + >>> + H265RawSEIMasteringDiplayColorVolume mastering_display; >>> >>> int64_t last_idr_frame; >>> int pic_order_cnt; >>> @@ -58,6 +65,7 @@ typedef struct VAAPIEncodeH265Context { >>> CodedBitstreamContext *cbc; >>> CodedBitstreamFragment current_access_unit; >>> int aud_needed; >>> + int sei_needed; >>> } VAAPIEncodeH265Context; >>> >>> typedef struct VAAPIEncodeH265Options { >>> @@ -65,6 +73,7 @@ typedef struct VAAPIEncodeH265Options { >>> int aud; >>> int profile; >>> int level; >>> + int sei; >>> } VAAPIEncodeH265Options; >>> >>> >>> @@ -175,6 +184,61 @@ 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; >>> + VAAPIEncodeH265Options *opt = ctx->codec_options; >>> + 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.nal_unit_type = HEVC_NAL_SEI_PREFIX; >>> + priv->sei.nal_unit_header.nuh_temporal_id_plus1 = 1; >> >> Might look nicer as a compound literal? > > Agree, I will update the patch. > >> >>> + i = 0; >>> + >>> + if (opt->sei & SEI_MASTERING_DISPLAY && pic->type == >>> PICTURE_TYPE_IDR) { >>> + 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 +651,23 @@ static int >>> vaapi_encode_h265_init_picture_params(AVCodecContext *avctx, >>> priv->aud_needed = 0; >>> } >>> >>> + if ((opt->sei & SEI_MASTERING_DISPLAY) && >>> + (pic->type == PICTURE_TYPE_I || pic->type == PICTURE_TYPE_IDR)) { >>> + // hard-coded value for testing, change it later >>> + for (i = 0; i < 3; i++) { >>> + priv->mastering_display.display_primaries[i].x = 50000; >>> + priv->mastering_display.display_primaries[i].y = 50000; >>> + } >>> + >>> + priv->mastering_display.white_point_x = 50000; >>> + priv->mastering_display.white_point_y = 50000; >>> + >>> + priv->mastering_display.max_display_mastering_luminance = 5000; >>> + priv->mastering_display.min_display_mastering_luminance = 1000; >>> + >>> + priv->sei_needed = 1; >>> + } >> >> Obviously this needs to contain real data before it can be applied. >> > > Yes. Currently I have no idea how does a user pass these data to hevc_vaapi > encoder in FFmpeg. Is adding new options for these data acceptable? Look at AVMasteringDisplayMetadata (and AVContentLightMetadata), which are carried as AVFrameSideData. That can be set either by the source metadata (some container and codec), or it can be added by library users - there isn't currently a way to inject it from the ffmpeg command-line, but if you need one then I imagine the place to do it would be in libavfilter. >> Is it actually going to work as part of the sequence parameters? The >> mastering display metadata side-data which presumably will be the input for >> this is per-frame. >> > > Do you mean a part of the sequence parameters for VAAPI? The vaapi driver > doesn't require these data. I mean that you probably don't know this information when the init_sequence_params function is called, because it exists as frame side-data. I think that means is has to go in init_picture_params instead. >>> + >>> vpic->decoded_curr_pic = (VAPictureHEVC) { >>> .picture_id = pic->recon_surface, >>> .pic_order_cnt = priv->pic_order_cnt, >>> @@ -895,6 +976,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 +1026,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 +1087,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 = 0 }, >>> + 0, INT_MAX, FLAGS, "sei" }, >> >> If actual inclusion is conditional on the side-data being present on the input >> then it probably makes sense for it to be on by default. >> > > If setting the default value to 1, mastering display will be added even if '-sei > xxx' is not set in the command line. Does it make sense? And following on from the above, it would only be included with a given frame if the side-data is actually present. So, setting the option by default would do nothing for most streams (again compare -sei timing in H.264, which does nothing in CQ mode). >>> + { "mastering_display", "Include mastering display colour volumne", >>> + 0, AV_OPT_TYPE_CONST, { .i64 = SEI_MASTERING_DISPLAY }, >>> + INT_MIN, INT_MAX, FLAGS, "sei" }, >>> + >>> { NULL }, >>> }; >>> - Mark
On Mon, 2018-04-23 at 23:51 +0100, Mark Thompson wrote: > On 23/04/18 08:08, Xiang, Haihao wrote:>> On 20/04/18 08:27, Haihao Xiang > wrote: > > > > '-sei xxx' is added to control SEI insertion, so far only mastering > > > > display colour colume is available for testing. Another patch is > > > > required to change mastering display settings later. > > > > > > > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > > > > --- > > > > libavcodec/vaapi_encode_h265.c | 94 > > > > +++++++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 93 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/libavcodec/vaapi_encode_h265.c > > > > b/libavcodec/vaapi_encode_h265.c > > > > index 5203c6871d..a8cb6a6d8b 100644 > > > > --- a/libavcodec/vaapi_encode_h265.c > > > > +++ b/libavcodec/vaapi_encode_h265.c > > > > @@ -29,10 +29,14 @@ > > > > #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 = 0x01, > > > > > > Since the options are essentially the same I think it would be nice if > > > these > > > values matched the H.264 ones? (That is, make this value 8.) > > > > > > > My original thought was to make this value 8, but I am not sure whether > > another > > SEI_XXX will be added for H.264 in the future. Will we use 8 for a new > > SEI_XXX > > for H.264? > > I think that's fine - if we do add anything new for H.264 then it can have > another value. > > It would also be possible to add support for the HDR metadata to the H.264 > code, though it's probably not of much use here when hardware isn't going to > support >8-bit video. > > > > Should this be mastering display specifically, or would it be better for > > > this > > > option to be called something like "HDR metadata" (just "hdr" as the > > > option > > > name?) which also includes content light level? (Compare the -sei timing > > > option on H.264, which gives you both buffering period and picture timing > > > SEI.) > > > > > > > Good idea, I prefer using hdr for mastering display and content light level. > > Actually adding support for content light level is in my todo list. > > Ok, nice :) I use two different SEI values for mastering display and content light level in the new version of patch because VAAPIEncodeH265Context::sei_needed is taken as a ORed value and we needn't check the corresponding SEI message as before when writing the header. > > > > > +}; > > > > > > > > typedef struct VAAPIEncodeH265Context { > > > > unsigned int ctu_width; > > > > @@ -47,6 +51,9 @@ typedef struct VAAPIEncodeH265Context { > > > > H265RawSPS sps; > > > > H265RawPPS pps; > > > > H265RawSlice slice; > > > > + H265RawSEI sei; > > > > + > > > > + H265RawSEIMasteringDiplayColorVolume mastering_display; > > > > > > > > int64_t last_idr_frame; > > > > int pic_order_cnt; > > > > @@ -58,6 +65,7 @@ typedef struct VAAPIEncodeH265Context { > > > > CodedBitstreamContext *cbc; > > > > CodedBitstreamFragment current_access_unit; > > > > int aud_needed; > > > > + int sei_needed; > > > > } VAAPIEncodeH265Context; > > > > > > > > typedef struct VAAPIEncodeH265Options { > > > > @@ -65,6 +73,7 @@ typedef struct VAAPIEncodeH265Options { > > > > int aud; > > > > int profile; > > > > int level; > > > > + int sei; > > > > } VAAPIEncodeH265Options; > > > > > > > > > > > > @@ -175,6 +184,61 @@ 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; > > > > + VAAPIEncodeH265Options *opt = ctx->codec_options; > > > > + 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.nal_unit_type = HEVC_NAL_SEI_PREFIX; > > > > + priv->sei.nal_unit_header.nuh_temporal_id_plus1 = 1; > > > > > > Might look nicer as a compound literal? > > > > Agree, I will update the patch. > > > > > > > > > + i = 0; > > > > + > > > > + if (opt->sei & SEI_MASTERING_DISPLAY && pic->type == > > > > PICTURE_TYPE_IDR) { > > > > + 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 +651,23 @@ static int > > > > vaapi_encode_h265_init_picture_params(AVCodecContext *avctx, > > > > priv->aud_needed = 0; > > > > } > > > > > > > > + if ((opt->sei & SEI_MASTERING_DISPLAY) && > > > > + (pic->type == PICTURE_TYPE_I || pic->type == PICTURE_TYPE_IDR)) > > > > { > > > > + // hard-coded value for testing, change it later > > > > + for (i = 0; i < 3; i++) { > > > > + priv->mastering_display.display_primaries[i].x = 50000; > > > > + priv->mastering_display.display_primaries[i].y = 50000; > > > > + } > > > > + > > > > + priv->mastering_display.white_point_x = 50000; > > > > + priv->mastering_display.white_point_y = 50000; > > > > + > > > > + priv->mastering_display.max_display_mastering_luminance = 5000; > > > > + priv->mastering_display.min_display_mastering_luminance = 1000; > > > > + > > > > + priv->sei_needed = 1; > > > > + } > > > > > > Obviously this needs to contain real data before it can be applied. > > > > > > > Yes. Currently I have no idea how does a user pass these data to hevc_vaapi > > encoder in FFmpeg. Is adding new options for these data acceptable? > > Look at AVMasteringDisplayMetadata (and AVContentLightMetadata), which are > carried as AVFrameSideData. That can be set either by the source metadata > (some container and codec), or it can be added by library users Thanks, I use AVMasteringDisplayMetadata and AVMasteringDisplayMetadata in the new patches. > - there isn't currently a way to inject it from the ffmpeg command-line, but > if you need one then I imagine the place to do it would be in libavfilter. > > > > Is it actually going to work as part of the sequence parameters? The > > > mastering display metadata side-data which presumably will be the input > > > for > > > this is per-frame. > > > > > > > Do you mean a part of the sequence parameters for VAAPI? The vaapi driver > > doesn't require these data. > > I mean that you probably don't know this information when the > init_sequence_params function is called, because it exists as frame side- > data. I think that means is has to go in init_picture_params instead. Yes, it is done in init_picture_params. > > > > > + > > > > vpic->decoded_curr_pic = (VAPictureHEVC) { > > > > .picture_id = pic->recon_surface, > > > > .pic_order_cnt = priv->pic_order_cnt, > > > > @@ -895,6 +976,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 +1026,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 +1087,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 = 0 }, > > > > + 0, INT_MAX, FLAGS, "sei" }, > > > > > > If actual inclusion is conditional on the side-data being present on the > > > input > > > then it probably makes sense for it to be on by default. > > > > > > > If setting the default value to 1, mastering display will be added even if > > '-sei > > xxx' is not set in the command line. Does it make sense? > > And following on from the above, it would only be included with a given frame > if the side-data is actually present. So, setting the option by default would > do nothing for most streams (again compare -sei timing in H.264, which does > nothing in CQ mode). > > > > > + { "mastering_display", "Include mastering display colour volumne", > > > > + 0, AV_OPT_TYPE_CONST, { .i64 = SEI_MASTERING_DISPLAY }, > > > > + INT_MIN, INT_MAX, FLAGS, "sei" }, > > > > + > > > > { NULL }, > > > > }; > > > > > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c index 5203c6871d..a8cb6a6d8b 100644 --- a/libavcodec/vaapi_encode_h265.c +++ b/libavcodec/vaapi_encode_h265.c @@ -29,10 +29,14 @@ #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 = 0x01, +}; typedef struct VAAPIEncodeH265Context { unsigned int ctu_width; @@ -47,6 +51,9 @@ typedef struct VAAPIEncodeH265Context { H265RawSPS sps; H265RawPPS pps; H265RawSlice slice; + H265RawSEI sei; + + H265RawSEIMasteringDiplayColorVolume mastering_display; int64_t last_idr_frame; int pic_order_cnt; @@ -58,6 +65,7 @@ typedef struct VAAPIEncodeH265Context { CodedBitstreamContext *cbc; CodedBitstreamFragment current_access_unit; int aud_needed; + int sei_needed; } VAAPIEncodeH265Context; typedef struct VAAPIEncodeH265Options { @@ -65,6 +73,7 @@ typedef struct VAAPIEncodeH265Options { int aud; int profile; int level; + int sei; } VAAPIEncodeH265Options; @@ -175,6 +184,61 @@ 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; + VAAPIEncodeH265Options *opt = ctx->codec_options; + 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.nal_unit_type = HEVC_NAL_SEI_PREFIX; + priv->sei.nal_unit_header.nuh_temporal_id_plus1 = 1; + i = 0; + + if (opt->sei & SEI_MASTERING_DISPLAY && pic->type == PICTURE_TYPE_IDR) { + 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 +651,23 @@ static int vaapi_encode_h265_init_picture_params(AVCodecContext *avctx, priv->aud_needed = 0; } + if ((opt->sei & SEI_MASTERING_DISPLAY) && + (pic->type == PICTURE_TYPE_I || pic->type == PICTURE_TYPE_IDR)) { + // hard-coded value for testing, change it later + for (i = 0; i < 3; i++) { + priv->mastering_display.display_primaries[i].x = 50000; + priv->mastering_display.display_primaries[i].y = 50000; + } + + priv->mastering_display.white_point_x = 50000; + priv->mastering_display.white_point_y = 50000; + + priv->mastering_display.max_display_mastering_luminance = 5000; + priv->mastering_display.min_display_mastering_luminance = 1000; + + priv->sei_needed = 1; + } + vpic->decoded_curr_pic = (VAPictureHEVC) { .picture_id = pic->recon_surface, .pic_order_cnt = priv->pic_order_cnt, @@ -895,6 +976,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 +1026,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 +1087,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 = 0 }, + 0, INT_MAX, FLAGS, "sei" }, + { "mastering_display", "Include mastering display colour volumne", + 0, AV_OPT_TYPE_CONST, { .i64 = SEI_MASTERING_DISPLAY }, + INT_MIN, INT_MAX, FLAGS, "sei" }, + { NULL }, };
'-sei xxx' is added to control SEI insertion, so far only mastering display colour colume is available for testing. Another patch is required to change mastering display settings later. Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> --- libavcodec/vaapi_encode_h265.c | 94 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 93 insertions(+), 1 deletion(-)