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.xi...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
