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