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