On 6/22/2019 3:32 AM, Andreas Rheinhardt wrote: > Hello, > > first a general note: hevc_parse_nal_header in h2645_parse.c > explicitly discards any NAL units with nuh_layer_id != 0. As long as > this is so adding support for this SEI is pointless.
If you try this patch with the sample i linked, you'll see the SEI is parsed and shown. > (Your comment on IRC was wrong: The Apple samples contain two SPS and > PPS each.) Ah, that would explain why trace_headers didn't show them. CBS should probably stop using h2645_parse functions then, and duplicate its functionality. I'd rather not change h2645_parse for this and risk unpredictable behavior from the decoder. > > James Almer: >> As defined in section F.14.2.8 and F.14.3.8 >> >> Signed-off-by: James Almer <jamr...@gmail.com> >> --- >> https://trac.ffmpeg.org/attachment/ticket/7965/puppets_with_alpha_hevc.mov >> >> libavcodec/cbs_h2645.c | 1 + >> libavcodec/cbs_h265.h | 12 +++++++++ >> libavcodec/cbs_h265_syntax_template.c | 37 +++++++++++++++++++++++++++ >> libavcodec/hevc_sei.h | 1 + >> 4 files changed, 51 insertions(+) >> >> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c >> index 0456937710..f35a2c01f7 100644 >> --- a/libavcodec/cbs_h2645.c >> +++ b/libavcodec/cbs_h2645.c >> @@ -530,6 +530,7 @@ static void cbs_h265_free_sei_payload(H265RawSEIPayload >> *payload) >> case HEVC_SEI_TYPE_MASTERING_DISPLAY_INFO: >> case HEVC_SEI_TYPE_CONTENT_LIGHT_LEVEL_INFO: >> case HEVC_SEI_TYPE_ALTERNATIVE_TRANSFER_CHARACTERISTICS: >> + case HEVC_SEI_TYPE_ALPHA_CHANNEL_INFO: >> break; >> case HEVC_SEI_TYPE_USER_DATA_REGISTERED_ITU_T_T35: >> av_buffer_unref(&payload->payload.user_data_registered.data_ref); >> diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h >> index c9bc90187b..ad746bf35f 100644 >> --- a/libavcodec/cbs_h265.h >> +++ b/libavcodec/cbs_h265.h >> @@ -679,6 +679,17 @@ typedef struct >> H265RawSEIAlternativeTransferCharacteristics { >> uint8_t preferred_transfer_characteristics; >> } H265RawSEIAlternativeTransferCharacteristics; >> >> +typedef struct H265RawSEIAlphaChannelInfo { >> + uint8_t alpha_channel_cancel_flag; >> + uint8_t alpha_channel_use_idc; >> + uint8_t alpha_channel_bit_depth_minus8; >> + uint16_t alpha_transparent_value; >> + uint16_t alpha_opaque_value; >> + uint8_t alpha_channel_incr_flag; >> + uint8_t alpha_channel_clip_flag; >> + uint8_t alpha_channel_clip_type_flag; >> +} H265RawSEIAlphaChannelInfo; >> + >> typedef struct H265RawSEIPayload { >> uint32_t payload_type; >> uint32_t payload_size; >> @@ -697,6 +708,7 @@ typedef struct H265RawSEIPayload { >> H265RawSEIContentLightLevelInfo content_light_level; >> H265RawSEIAlternativeTransferCharacteristics >> alternative_transfer_characteristics; >> + H265RawSEIAlphaChannelInfo alpha_channel_info; >> struct { >> uint8_t *data; >> size_t data_length; >> diff --git a/libavcodec/cbs_h265_syntax_template.c >> b/libavcodec/cbs_h265_syntax_template.c >> index f279d283d9..bcddd6d3b2 100644 >> --- a/libavcodec/cbs_h265_syntax_template.c >> +++ b/libavcodec/cbs_h265_syntax_template.c >> @@ -2028,6 +2028,42 @@ static int >> FUNC(sei_alternative_transfer_characteristics)(CodedBitstreamContext >> return 0; >> } >> >> +static int FUNC(sei_alpha_channel_info)(CodedBitstreamContext *ctx, >> + RWContext *rw, >> + H265RawSEIAlphaChannelInfo *current) >> +{ >> + CodedBitstreamH265Context *h265 = ctx->priv_data; >> + const H265RawSPS *sps = h265->active_sps; >> + int err, length; >> + >> + HEADER("Alpha Channel Information"); >> + >> + if (!sps) { >> + av_log(ctx->log_ctx, AV_LOG_ERROR, >> + "No active SPS for alpha_channel_info.\n"); >> + return AVERROR_INVALIDDATA; >> + } >> + >> + flag(alpha_channel_cancel_flag); >> + if(!current->alpha_channel_cancel_flag) { > Nit: Missing space after if. >> + ub(3, alpha_channel_use_idc); >> + u(3, alpha_channel_bit_depth_minus8, 0, sps->bit_depth_luma_minus8); > This check is possibly problematic: Think of a file containing more > than one set of parameter sets in its extradata. Given that no coded > picture has been encountered yet, no parameter set is active according > to the standard. But that is not what cbs currently does: Every time a > new PPS is encountered, the SPS referenced therein is considered the > new active SPS. And this can be wrong and in this case it might lead > to valid files being rejected. So I don't think that it is wise to > perform any check on the value of alpha_channel_bit_depth_minus8. Ok, will remove the check. > > (Btw: Actually, alpha_channel_bit_depth_minus8 has to be equal to > bit_depth_luma_minus8 of the primary coded picture, so if you want to > check, you can also use the lower bound. (I also wonder whether it is > really intended that alpha_channel_bit_depth_minus8 shall be equal to > bit_depth_luma_minus8 of the primary coded picture and not the > auxiliary coded picture that actually contains the alpha channel. Is > it actually legal for bit_depth_luma_minus8 of the auxiliary and the > primary coded picture to differ in this case? I have found nothing > that says that it is illegal.)) > >> + length = current->alpha_channel_bit_depth_minus8 + 9; >> + ub(length, alpha_transparent_value); >> + ub(length, alpha_opaque_value); >> + flag(alpha_channel_incr_flag); >> + flag(alpha_channel_clip_flag); >> + if(current->alpha_channel_clip_flag) >> + flag(alpha_channel_clip_type_flag); >> + } else { >> + infer(alpha_channel_use_idc, 2); > Nit: The 2 can be aligned with the 0. Will change. >> + infer(alpha_channel_incr_flag, 0); >> + infer(alpha_channel_clip_flag, 0); >> + } >> + >> + return 0; >> +} >> + >> static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw, >> H265RawSEIPayload *current, int prefix) >> { >> @@ -2080,6 +2116,7 @@ static int FUNC(sei_payload)(CodedBitstreamContext >> *ctx, RWContext *rw, >> SEI_TYPE_N(CONTENT_LIGHT_LEVEL_INFO, 1, 0, content_light_level); >> SEI_TYPE_N(ALTERNATIVE_TRANSFER_CHARACTERISTICS, >> 1, 0, >> alternative_transfer_characteristics); >> + SEI_TYPE_N(ALPHA_CHANNEL_INFO, 1, 0, alpha_channel_info); >> >> #undef SEI_TYPE >> default: >> diff --git a/libavcodec/hevc_sei.h b/libavcodec/hevc_sei.h >> index 2fec00ace0..f6516ae982 100644 >> --- a/libavcodec/hevc_sei.h >> +++ b/libavcodec/hevc_sei.h >> @@ -56,6 +56,7 @@ typedef enum { >> HEVC_SEI_TYPE_MASTERING_DISPLAY_INFO = 137, >> HEVC_SEI_TYPE_CONTENT_LIGHT_LEVEL_INFO = 144, >> HEVC_SEI_TYPE_ALTERNATIVE_TRANSFER_CHARACTERISTICS = 147, >> + HEVC_SEI_TYPE_ALPHA_CHANNEL_INFO = 165, >> } HEVC_SEI_Type; >> >> typedef struct HEVCSEIPictureHash { >> > > - Andreas > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
