| Message ID | 20190621012126.9138-1-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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. (Your comment on IRC was wrong: The Apple samples contain two SPS and PPS each.) James Almer: > As defined in section F.14.2.8 and F.14.3.8 > > Signed-off-by: James Almer <jamrial@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. (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. > + 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
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 <jamrial@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-request@ffmpeg.org with subject "unsubscribe". >
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) { + ub(3, alpha_channel_use_idc); + u(3, alpha_channel_bit_depth_minus8, 0, sps->bit_depth_luma_minus8); + 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); + 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 {
As defined in section F.14.2.8 and F.14.3.8 Signed-off-by: James Almer <jamrial@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(+)