On Sun, Jan 10, 2021 at 5:34 AM Mark Thompson <s...@jkqxz.net> wrote:
> On 09/01/2021 07:34, Nuo Mi wrote: > > --- > > configure | 2 + > > libavcodec/Makefile | 1 + > > libavcodec/cbs.c | 6 + > > libavcodec/cbs_h2645.c | 373 ++++ > > libavcodec/cbs_h266.h | 840 ++++++++ > > libavcodec/cbs_h266_syntax_template.c | 2761 +++++++++++++++++++++++++ > > libavcodec/cbs_internal.h | 3 +- > > 7 files changed, 3985 insertions(+), 1 deletion(-) > > create mode 100644 libavcodec/cbs_h266.h > > create mode 100644 libavcodec/cbs_h266_syntax_template.c > > > > ... > > @@ -920,6 +934,135 @@ static int > cbs_h265_read_nal_unit(CodedBitstreamContext *ctx, > > return 0; > > } > > > > +static int cbs_h266_replace_ph(CodedBitstreamContext *ctx, > > + CodedBitstreamUnit *unit) > > +{ > > + CodedBitstreamH266Context *priv = ctx->priv_data; > > + int err; > > + err = ff_cbs_make_unit_refcounted(ctx, unit); > > + if (err < 0) > > + return err; > > + av_buffer_unref(&priv->ph_ref); > > + av_assert0(unit->content_ref); > > + priv->ph_ref = av_buffer_ref(unit->content_ref); > > + if (!priv->ph_ref) > > + return AVERROR(ENOMEM); > > + priv->active_ph = priv->ph = (H266RawPH *)priv->ph_ref->data; > > Why are there too variables here? They seem to always be the same. > priv->active_ph is read-only, priv->ph is writeable pointer. I can change to priv->ph only if you prefer. > > > + return 0; > > +} > > + > > ... > > + > > static int cbs_h2645_assemble_fragment(CodedBitstreamContext *ctx, > > CodedBitstreamFragment *frag) > > { > > @@ -1248,6 +1494,11 @@ static int > cbs_h2645_assemble_fragment(CodedBitstreamContext *ctx, > > (unit->type == HEVC_NAL_VPS || > > unit->type == HEVC_NAL_SPS || > > unit->type == HEVC_NAL_PPS)) || > > + (ctx->codec->codec_id == AV_CODEC_ID_VVC && > > + (unit->type == VVC_VPS_NUT || > > + unit->type == VVC_SPS_NUT || > > + unit->type == VVC_PPS_NUT || > > + unit->type == VVC_PREFIX_APS_NUT)) || > > Also various other things, which might be here since passthrough does not > require decomposition to be implemented. > > This test is getting unwieldy - maybe it should be moved to a new function > cbs_h2645_unit_requires_zero_byte(). > done > > i == 0 /* (Assume this is the start of an access unit.) > */) { > > // zero_byte > > data[dp++] = 0; > > @@ -1362,6 +1613,41 @@ static void cbs_h265_close(CodedBitstreamContext > *ctx) > > av_buffer_unref(&h265->pps_ref[i]); > > } > > > > ... > > @@ -1506,6 +1792,77 @@ static const CodedBitstreamUnitTypeDescriptor > cbs_h265_unit_types[] = { > > CBS_UNIT_TYPE_END_OF_LIST > > }; > > > > +static void cbs_h266_free_sei(void *opaque, uint8_t *content) > > +{ > > +} > > So as implemented currently it is POD? > Yes, currently only md5 sei implemented. We can do more after your react patch merged. > > > + > > +static const CodedBitstreamUnitTypeDescriptor cbs_h266_unit_types[] = { > > ... > > + > > +typedef struct H266RawNALUnitHeader { > > + uint8_t nuh_layer_id; > > + uint8_t nal_unit_type; > > + uint8_t nuh_temporal_id_plus1; > > +} H266RawNALUnitHeader; > > + > > +typedef struct H266GeneralConstraintsInfo { > > + uint8_t gci_present_flag; > > + > > ... > > + > > + /* loop filter */ > > + uint8_t gci_no_sao_constraint_flag; > > + uint8_t gci_no_alf_constraint_flag; > > + uint8_t gci_no_ccalf_constraint_flag; > > + uint8_t gci_no_lmcs_constraint_flag; > > + uint8_t gci_no_ladf_constraint_flag; > > + uint8_t gci_no_virtual_boundaries_constraint_flag; > > + uint8_t gci_num_reserved_bits; > > Also needs gci_reserved_zero_bit[], so that we can handle streams with > future constraints rather than just rejecting them. > > "Although the value of gci_num_reserved_bits is required to be equal to 0 > in this version > of this Specification, decoders conforming to this version of this > Specification shall allow the value of > gci_num_reserved_bits greater than 0 to appear in the syntax and shall > ignore the values of all the gci_reserved_zero_bit[ i ] > syntax elements when gci_num_reserved_bits is greater than 0." > This just follows the same pattern as h265. How about we create a separate patch set for this, fix h265 as well. > > +} H266GeneralConstraintsInfo; > > + > > ... > > + > > +typedef struct H266RawVPS { > > + H266RawNALUnitHeader nal_unit_header; > > + > > + uint8_t vps_video_parameter_set_id; > > + > > + uint8_t vps_max_layers_minus1; > > + uint8_t vps_max_sublayers_minus1; > > + /*TODO add more*/ > > + H266RawExtensionData extension_data; > > +} H266RawVPS; > > You don't actually use the VPS structure at all, so given that it isn't > parsed I think it would be cleaner to just not include it here at all. > Done. The current conformance stream has no VPS. > > > +// > > ... > > + > > +typedef struct H266RawSEIBufferingPeriod { > > + uint8_t bp_seq_parameter_set_id; > > +} H266RawSEIBufferingPeriod; > > + > > +typedef struct H266RawSEIPicTiming { > > +} H266RawSEIPicTiming; > > + > > +typedef struct H266RawSEIPanScanRect { > > +} H266RawSEIPanScanRect; > > + > > +typedef struct H266RawSEIUserDataRegistered { > > + > > +} H266RawSEIUserDataRegistered; > > + > > +typedef struct H266RawSEIUserDataUnregistered { > > +} H266RawSEIUserDataUnregistered; > > + > > +typedef struct H266RawSEIRecoveryPoint { > > +} H266RawSEIRecoveryPoint; > > + > > +typedef struct H266RawSEIDisplayOrientation { > > +} H266RawSEIDisplayOrientation; > > + > > +typedef struct H266RawSEIActiveParameterSets { > > +} H266RawSEIActiveParameterSets; > > + > > +typedef struct H266RawSEIDecodedPictureHash { > > + uint8_t dph_sei_hash_type; > > + uint8_t dph_sei_single_component_flag; > > + uint8_t dph_sei_picture_md5[3][16]; > > + uint16_t dph_sei_picture_crc[3]; > > + uint32_t dph_sei_picture_checksum[3]; > > +} H266RawSEIDecodedPictureHash; > > + > > +typedef struct H266RawSEITimeCode { > > +} H266RawSEITimeCode; > > + > > +typedef struct H266RawSEIMasteringDisplayColourVolume { > > +} H266RawSEIMasteringDisplayColourVolume; > > + > > Don't include all of these empty structures. > > (Half of them are implemented in the common SEI patches on the ML now > anyway.) > Fixed > > > ... > > +#endif /* AVCODEC_CBS_H266_H */ > > diff --git a/libavcodec/cbs_h266_syntax_template.c > b/libavcodec/cbs_h266_syntax_template.c > > new file mode 100644 > > index 0000000000..6a6defc8a5 > > --- /dev/null > > +++ b/libavcodec/cbs_h266_syntax_template.c > > @@ -0,0 +1,2761 @@ > > +/* > > + * This file is part of FFmpeg. > > + * > > + * FFmpeg is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2.1 of the License, or (at your option) any later version. > > + * > > + * FFmpeg is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with FFmpeg; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > 02110-1301 USA > > + */ > > + > > +#ifndef H266_CEIL > > +#define H266_CEIL(v, a) (((v) + (a) - 1) / (a)) > > +#endif > > If it's useful to have this separately then put it in cbs_h2645.c rather > than doubled in the template. (See for example cbs_av1_tile_log2().) > It's protected by ifdef, not doubled. > > + > > +#ifndef H266_HAS_MULTI_SLICES_IN_TILE > > +#define H266_HAS_MULTI_SLICES_IN_TILE(pps, tile_y, slice) \ > > + (pps->pps_slice_width_in_tiles_minus1[slice] == 0 && \ > > + pps->pps_slice_height_in_tiles_minus1[slice] == 0 && \ > > + pps->pps_tile_row_height_minus1[tile_y] > 0) > > +#endif > > This seems to be used exactly once and match the syntax in the standard, > so I don't think it should be separate. > Done > > > + > > ... > > + > > +static int FUNC(byte_alignment)(CodedBitstreamContext *ctx, RWContext > *rw) > > +{ > > + int err; > > + > > + fixed(1, alignment_bit_equal_to_one, 1); > > + while (byte_alignment(rw) != 0) > > + fixed(1, alignment_bit_equal_to_zero, 0); > > Names aren't right. > Fixed. > > > + return 0; > > +} > > + > > +static int FUNC(general_constraints_info)(CodedBitstreamContext *ctx, > > + RWContext *rw, > > + H266GeneralConstraintsInfo > *current) > > +{ > > + int err, i; > > + > > + flag(gci_present_flag); > > + if (current->gci_present_flag) { > > + ... > > + > > + /* loop filter */ > > + flag(gci_no_sao_constraint_flag); > > + flag(gci_no_alf_constraint_flag); > > + flag(gci_no_ccalf_constraint_flag); > > + flag(gci_no_lmcs_constraint_flag); > > + flag(gci_no_ladf_constraint_flag); > > + flag(gci_no_virtual_boundaries_constraint_flag); > > + ub(8, gci_num_reserved_bits); > > + for (i = 0; i < current->gci_num_reserved_bits; i++) { > > + fixed(1, gci_reserved_zero_bit, 1); > > As noted above, needs to be passed through for future compatibility. > Yes, but it just follows h265 cbs, how about we handle it in a separate patch set. > > > + } > > + } > > + while (byte_alignment(rw) != 0) > > + fixed(1, gci_alignment_zero_bit, 0); > > + return 0; > > +} > > + > > +static int FUNC(profile_tier_level)(CodedBitstreamContext *ctx, > RWContext *rw, > > + H266RawProfileTierLevel *current, > > + int profile_tier_present_flag, > > + int max_num_sub_layers_minus1) > > +{ > > + int err, i; > > + > > + if (profile_tier_present_flag) { > > + ub(7, general_profile_idc); > > + flag(general_tier_flag); > > + } > > + ub(8, general_level_idc); > > + flag(ptl_frame_only_constraint_flag); > > + flag(ptl_multilayer_enabled_flag); > > + if (profile_tier_present_flag) { > > + CHECK(FUNC(general_constraints_info)(ctx, rw, > > + > ¤t->general_constraints_info)); > > + } > > + for (i = max_num_sub_layers_minus1 - 1; i >= 0; i--) > > + flags(ptl_sublayer_level_present_flag[i], 1, i); > > + while (byte_alignment(rw) != 0) > > + fixed(1, ptl_reserved_zero_bit, 0); > > Need not be zero, has to be passed through. > Ditto > > > + for (i = max_num_sub_layers_minus1 - 1; i >= 0; i--) > > + if (current->ptl_sublayer_level_present_flag[i]) > > + ubs(8, sublayer_level_idc[i], 1, i); > > + if (profile_tier_present_flag) { > > + ub(8, ptl_num_sub_profiles); > > + for (i = 0; i < current->ptl_num_sub_profiles; i++) > > + ubs(32, general_sub_profile_idc[i], 1, i); > > + } > > + return 0; > > +} > > + > > +static int FUNC(vui_parameters)(CodedBitstreamContext *ctx, RWContext > *rw, > > + H266RawVUI *current) > > +{ > > + int err; > > + > > + flag(vui_progressive_source_flag); > > + flag(vui_interlaced_source_flag); > > + flag(vui_non_packed_constraint_flag); > > + flag(vui_non_projected_constraint_flag); > > + flag(vui_aspect_ratio_info_present_flag); > > + if (current->vui_aspect_ratio_info_present_flag) { > > + flag(vui_aspect_ratio_constant_flag); > > + ub(8, vui_aspect_ratio_idc); > > + if (current->vui_aspect_ratio_idc == 255) { > > + ub(16, vui_sar_width); > > + ub(16, vui_sar_height); > > + } > > + } else { > > + infer(vui_aspect_ratio_constant_flag, 0); > > + infer(vui_aspect_ratio_idc, 0); > > + } > > + flag(vui_overscan_info_present_flag); > > + if (current->vui_overscan_info_present_flag) > > + flag(vui_overscan_appropriate_flag); > > + flag(vui_colour_description_present_flag); > > + if (current->vui_colour_description_present_flag) { > > + ub(8, vui_colour_primaries); > > + ub(8, vui_transfer_characteristics); > > + ub(8, vui_matrix_coeffs); > > + flag(vui_full_range_flag); > > + } else { > > + infer(vui_colour_primaries, 2); > > + infer(vui_transfer_characteristics, 2); > > + infer(vui_matrix_coeffs, 2); > > + infer(vui_full_range_flag, 0); > > + } > > + flag(vui_chroma_loc_info_present_flag); > > + if (current->vui_chroma_loc_info_present_flag) { > > + if (current->vui_progressive_source_flag && > > + !current->vui_interlaced_source_flag) { > > + ue(vui_chroma_sample_loc_type_frame, 0, 6); > > + } else { > > + ue(vui_chroma_sample_loc_type_top_field, 0, 6); > > + ue(vui_chroma_sample_loc_type_bottom_field, 0, 6); > > + } > > + } > > These are inferred as 6 when not present? > 6 only happened when ChromaFormatIdc = 1, others are not defined. and 6 is the unspecific value in the spec... Do we really need to infer it :) > > > + > > + return 0; > > +} > > + > > ... > > + > > +static int FUNC(vui_payload)(CodedBitstreamContext *ctx, RWContext *rw, > > + H266RawVUI *current, uint16_t > vui_payload_size) > > +{ > > + int err; > > + int start_position, current_position; > > +#ifdef READ > > + start_position = get_bits_count(rw); > > +#else > > + start_position = put_bits_count(rw); > > +#endif > > + CHECK(FUNC(vui_parameters)(ctx, rw, current)); > > + > > +#ifdef READ > > + current_position = get_bits_count(rw) - start_position; > > +#else > > + current_position = put_bits_count(rw) - start_position; > > +#endif > > Looks like another case for bit_position(RWContext) as in < > https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2021-January/274208.html>. > yeah, we can change it after your patch merged. > > > + if (current_position < 8 * vui_payload_size) { > > + CHECK(FUNC(payload_extension)(ctx, rw, ¤t->extension_data, > > + vui_payload_size, > current_position)); > > + fixed(1, vui_payload_bit_equal_to_one, 1); > > + while (byte_alignment(rw) != 0) > > + fixed(1, vui_payload_bit_equal_to_zero, 0); > > + } > > + return 0; > > +} > > + > > ... > > + > > +static int FUNC(ref_pic_list_struct)(CodedBitstreamContext *ctx, > RWContext *rw, > > + H266RefPicListStruct *current, > > + uint8_t list_idx, uint8_t rpls_idx, > > + const H266RawSPS *sps) > > +{ > > + > > + int err, i, j; > > + ue(num_ref_entries, 0, VVC_MAX_DPB_SIZE + 13); > > The + 13 is appearing in the sizes too. Can we give it a better name? > fixed with VVC_MAX_REF_ENTRIES > > > + if (sps->sps_long_term_ref_pics_flag && > > + rpls_idx < sps->sps_num_ref_pic_lists[list_idx] && > > + current->num_ref_entries > 0) > > + flag(ltrp_in_header_flag); > > + if (sps->sps_long_term_ref_pics_flag && > > + rpls_idx == sps->sps_num_ref_pic_lists[list_idx]) > > + infer(ltrp_in_header_flag, 1); > > + for (i = 0, j = 0; i < current->num_ref_entries; i++) { > > + if (sps->sps_inter_layer_prediction_enabled_flag) > > + flags(inter_layer_ref_pic_flag[i], 1, i); > > + else > > + infer(inter_layer_ref_pic_flag[i], 0); > > + > > + if (!current->inter_layer_ref_pic_flag[i]) { > > + if (sps->sps_long_term_ref_pics_flag) > > + flags(st_ref_pic_flag[i], 1, i); > > + else > > + infer(st_ref_pic_flag[i], 1); > > + if (current->st_ref_pic_flag[i]) { > > + int abs_delta_poc_st; > > + ues(abs_delta_poc_st[i], 0, MAX_UINT_BITS(15), 1, i); > > + if ((sps->sps_weighted_pred_flag || > > + sps->sps_weighted_bipred_flag) && i != 0) > > + abs_delta_poc_st = current->abs_delta_poc_st[i]; > > + else > > + abs_delta_poc_st = current->abs_delta_poc_st[i] + 1; > > + if (abs_delta_poc_st > 0) > > + flags(strp_entry_sign_flag[i], 1, i); > > + } else { > > + if (!current->ltrp_in_header_flag) { > > + uint8_t bits = > sps->sps_log2_max_pic_order_cnt_lsb_minus4 + 4; > > + ubs(bits, rpls_poc_lsb_lt[j], 1, j); > > + j++; > > + } > > + } > > + } else { > > + //todo: check range when vps parser is ready. > > + ues(ilrp_idx[i], 0, MAX_UINT_BITS(8), 1, i); > > + } > > + } > > + return 0; > > +} > > + > > ... > > + > > +static int FUNC(sublayer_hrd_parameters)(CodedBitstreamContext *ctx, > RWContext *rw, > > + H266RawSubLayerHRDParameters *current, int > sublayer_id, > > + const H266RawGeneralTimingHrdParameters *general) > > +{ > > + int err, i; > > + for (i = 0; i <= general->hrd_cpb_cnt_minus1; i++) { > > + ues(bit_rate_value_minus1[i], 0, UINT32_MAX - 1, 2, > sublayer_id, i); > > + ues(cpb_size_value_minus1[i], 0, UINT32_MAX - 1, 2, > sublayer_id, i); > > + if (general->general_du_hrd_params_present_flag) { > > + ues(cpb_size_du_value_minus1[i], > > + 0, UINT32_MAX - 1, 2, sublayer_id, i); > > + ues(bit_rate_du_value_minus1[i], > > + 0, UINT32_MAX - 1, 2, sublayer_id, i); > > + } > > + flags(cbr_flag[i], 2, sublayer_id, i); > > + } > > On everything here you've correctly got that there are two subscripts, but > there is actually only one in the variable - perhaps the argument shouldn't > be a substructure? > fixed > > > + return 0; > > +} > > + > > +static int FUNC(ols_timing_hrd_parameters)(CodedBitstreamContext *ctx, > > + RWContext *rw, H266RawOlsTimingHrdParameters *current, > > + uint8_t first_sublayer, uint8_t max_sublayers_minus1, > > + const H266RawGeneralTimingHrdParameters *general) > > +{ > > + int err, i; > > + for (i = first_sublayer; i <= max_sublayers_minus1; i++) { > > + flags(fixed_pic_rate_general_flag[i], 1, i); > > + if (!current->fixed_pic_rate_general_flag[i]) > > + flags(fixed_pic_rate_within_cvs_flag[i], 1, i); > > + else > > + infer(fixed_pic_rate_within_cvs_flag[i], 1); > > + if (current->fixed_pic_rate_within_cvs_flag[i]) { > > + ues(elemental_duration_in_tc_minus1[i], 0, 2047, 1, i); > > + infer(low_delay_hrd_flag[i], 0); > > + } else if ((general->general_nal_hrd_params_present_flag || > > + general->general_vcl_hrd_params_present_flag) && > > + general->hrd_cpb_cnt_minus1 == 0) { > > + flags(low_delay_hrd_flag[i], 1, i); > > + } else { > > + infer(low_delay_hrd_flag[i], 0); > > + } > > + if (general->general_nal_hrd_params_present_flag) > > + CHECK(FUNC(sublayer_hrd_parameters)(ctx, rw, > > + > ¤t->nal_sub_layer_hrd_parameters[i], > > + i, general)); > > + if (general->general_vcl_hrd_params_present_flag) > > + CHECK(FUNC(sublayer_hrd_parameters)(ctx, rw, > > + ¤t->nal_sub_layer_hrd_parameters[i], i, > general)); > > + } > > + return 0; > > +} > > + > > +static int FUNC(sps_subpic)(CodedBitstreamContext *ctx, RWContext *rw, > > + H266RawSPS *current) > > This function doesn't actually exist, and all splitting it out seems to be > doing is making the template look less like the standard? > VVC is more complex than hevc. If we do not use some sub-functions. It will be too large. not friendly for reader and indent. > > > +{ > > ... > > +} > > + > > +static int FUNC(sps)(CodedBitstreamContext *ctx, RWContext *rw, > > + H266RawSPS *current) > > +{ > > + CodedBitstreamH266Context *h266 = ctx->priv_data; > > + const H266RawVPS *vps; > > + int err, i, j; > > + unsigned int ctb_log2_size_y, min_cb_log2_size_y, > > + min_qt_log2_size_intra_y, min_qt_log2_size_inter_y, > > + ctb_size_y, max_num_merge_cand; > > + uint8_t qp_bd_offset; > > + > > + static const uint8_t h266_sub_width_c[] = { > > + 1, 2, 2, 1 > > + }; > > + static const uint8_t h266_sub_height_c[] = { > > + 1, 2, 1, 1 > > + }; > > + > > + HEADER("Sequence Parameter Set"); > > + > > + CHECK(FUNC(nal_unit_header)(ctx, rw, > > + ¤t->nal_unit_header, > VVC_SPS_NUT)); > > + > > + ub(4, sps_seq_parameter_set_id); > > + ub(4, sps_video_parameter_set_id); > > + if (!current->sps_video_parameter_set_id && !h266->vps[0]) { > > + H266RawVPS *vps0; > > + h266->vps_ref[0] = av_buffer_alloc(sizeof(H266RawVPS)); > > + if (!h266->vps_ref[0]) > > + return AVERROR(ENOMEM); > > + vps0 = h266->vps[0] = (H266RawVPS *)h266->vps_ref[0]->data; > > + vps0->vps_max_layers_minus1 = 0; > > + //todo: infer all things in 7.4.3.4 sps_video_parameter_set_id > paragraph > > + } > > + h266->active_vps = vps = > h266->vps[current->sps_video_parameter_set_id]; > > You don't actually use the vps at all, so why bother with this? > > > + > > + u(3, sps_max_sublayers_minus1, 0, VVC_MAX_SUBLAYERS - 1); > > + u(2, sps_chroma_format_idc, 0, 3); > > + u(2, sps_log2_ctu_size_minus5, 0, 3); > > + ctb_log2_size_y = current->sps_log2_ctu_size_minus5 + 5; > > + ctb_size_y = 1 << ctb_log2_size_y; > > + > > ... > > + > > + u(2, sps_num_extra_ph_bytes, 0, 2); > > + for (i = 0; i < (current->sps_num_extra_ph_bytes * 8); i++) { > > + flags(sps_extra_ph_bit_present_flag[i], 1, i); > > + } > > + > > + u(2, sps_num_extra_sh_bytes, 0, 0); > > Between zero and zero? > For the current version of spec, yes. Spec always says we need to open for some bits/values. But, from my personal experience, when we have other values in the reserved one. We usually cant correctly decode a stream. So, instead of ignoring the value, another approach is to report the error for it, so we can get the bitstream earlier and fix it. > > > + for (i = 0; i < (current->sps_num_extra_sh_bytes * 8); i++) { > > + flags(sps_extra_sh_bit_present_flag[i], 1, i); > > + } > > + > > + ... > > + > > + flag(sps_affine_enabled_flag); > > + if (current->sps_affine_enabled_flag) { > > + ue(sps_five_minus_max_num_subblock_merge_cand, > > + 0, 5 - current->sps_sbtmvp_enabled_flag); > > + flag(sps_6param_affine_enabled_flag); > > + if (current->sps_amvr_enabled_flag) > > + flag(sps_affine_amvr_enabled_flag); > > + else > > + infer(sps_affine_amvr_enabled_flag, 0); > > + flag(sps_affine_prof_enabled_flag); > > + if (current->sps_affine_prof_enabled_flag) > > + flag(sps_prof_control_present_in_ph_flag); > > + else > > + infer(sps_prof_control_present_in_ph_flag, 0); > > + } else { > > + infer(sps_five_minus_max_num_subblock_merge_cand, 0); > > This inference isn't right - there is also a temporal subblock merge > candidate. > How to get it? Spec did not define how to infer sps_five_minus_max_num_subblock_merge_cand, I just guess it from sps_6param_affine_enabled_flag > > > + infer(sps_6param_affine_enabled_flag, 0); > > + infer(sps_affine_amvr_enabled_flag, 0); > > + infer(sps_affine_prof_enabled_flag, 0); > > + infer(sps_prof_control_present_in_ph_flag, 0); > > + } > > + > > ... > > + > > + flag(sps_field_seq_flag); > > + flag(sps_vui_parameters_present_flag); > > + if (current->sps_vui_parameters_present_flag) { > > + ue(sps_vui_payload_size_minus1, 0, 1023); > > + while (byte_alignment(rw) != 0) > > + fixed(1, sps_vui_alignment_zero_bit, 0); > > + CHECK(FUNC(vui_payload)(ctx, rw, ¤t->vui, > > + current->sps_vui_payload_size_minus1 + > 1)); > > + } > > + > > + flag(sps_extension_flag); > > + if (current->sps_extension_flag) > > + CHECK(FUNC(extension_data)(ctx, rw, ¤t->extension_data)); > > + > > + CHECK(FUNC(rbsp_trailing_bits)(ctx, rw)); > > + > > + return 0; > > +} > > (I stopped here on trying to read this in detail. Even above, I haven't > checked all of the bounds.) > > > ... > > + > > +static int FUNC(aud)(CodedBitstreamContext *ctx, RWContext *rw, > > + H266RawAUD *current) > > +{ > > + int err; > > + > > + HEADER("Access Unit Delimiter"); > > + > > + CHECK(FUNC(nal_unit_header)(ctx, rw, > > + ¤t->nal_unit_header, > VVC_AUD_NUT)); > > + > > + flag(aud_irap_or_gdr_flag); > > + u(3, aud_pic_type, 0, 2); > > Another one for future-extension compatibility: > > "Decoders conforming to this version of this Specification shall ignore > reserved values of aud_pic_type." > yeah, we may need a separate patch to define a common macro, and fix h265/h266 together. > > > + > > + CHECK(FUNC(rbsp_trailing_bits)(ctx, rw)); > > + return 0; > > +} > > + > > ... > > + > > +static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw, > > + H266RawSliceHeader *current) > > +{ > > + CodedBitstreamH266Context *h266 = ctx->priv_data; > > + const H266RawSPS *sps; > > + const H266RawPPS *pps; > > + const H266RawPH *ph; > > + const H266RefPicLists *ref_pic_lists; > > + int err, i; > > + uint8_t nal_unit_type, qp_bd_offset; > > + uint16_t curr_subpic_idx; > > + uint16_t num_slices_in_subpic; > > + > > + HEADER("Slice Header"); > > + > > + CHECK(FUNC(nal_unit_header)(ctx, rw, ¤t->nal_unit_header, > -1)); > > + > > + flag(sh_picture_header_in_slice_header_flag); > > + if (current->sh_picture_header_in_slice_header_flag){ > > + CHECK(FUNC(picture_header)(ctx, rw, > ¤t->sh_picture_header)); > > + if (!h266->ph_ref) { > > + h266->ph_ref = av_buffer_allocz(sizeof(H266RawPH)); > > + if (!h266->ph_ref) > > + return AVERROR(ENOMEM); > > + h266->active_ph = h266->ph = (H266RawPH*)h266->ph_ref->data; > > + } > > + memcpy(h266->ph, ¤t->sh_picture_header, > sizeof(H266RawPH)); > > + } > > + sps = h266->active_sps; > > + pps = h266->active_pps; > > + ph = h266->active_ph; > > This is going to do something horrible if you have a frame where you lose > the NAL unit containing the picture header but it still has other slices. > > Can we detect and avoid that case? > In this case, we will have a parser to split the frames, the parser code will make sure it always has a picture header in every frame. > > + > > + if (!sps || !pps || !ph) { > > + av_log(ctx->log_ctx, AV_LOG_ERROR, "no > sps(%p)/pps(%p)/ph(%p).\n", sps, pps, ph); > > This message is definitely user-facing - it should be clearer. > > Maybe "SPS id %d not available", "PPS id %d not available", "Picture > header not available"? > actually, it's checked by picture_header(), once we got ph, we always got the sps and pps. will check ph and assert sps, pps. > > > + return AVERROR_INVALIDDATA; > > + } > > + > > ... > > + > > + return 0; > > +} > > + > > +static int FUNC(sei_decoded_picture_hash)(CodedBitstreamContext *ctx, > RWContext *rw, > > + H266RawSEIDecodedPictureHash > *current) > > +{ > > + int err, c, i; > > + > > + HEADER("Decoded Picture Hash"); > > + > > + u(8, dph_sei_hash_type, 0, 2); > > + flag(dph_sei_single_component_flag); > > + fixed(7, ph_sei_reserved_zero_7bits, 0); > > "shall ignore" > > > + > > + for (c = 0; c < (current->dph_sei_single_component_flag ? 1 : 3); > c++) { > > This variable is called cIdx, and it does appear in strings. > > > + if (current->dph_sei_hash_type == 0) { > > + for (i = 0; i < 16; i++) > > + us(8, dph_sei_picture_md5[c][i], 0x00, 0xff, 2, c, i); > > + } else if (current->dph_sei_hash_type == 1) { > > + us(16, dph_sei_picture_crc[c], 0x0000, 0xffff, 1, c); > > + } else if (current->dph_sei_hash_type == 2) { > > + us(32, dph_sei_picture_checksum[c], 0x00000000, 0xffffffff, > 1, c); > > + } > > + } > > + return 0; > > +} > > + > > ... > > Thanks, > > - Mark > _______________________________________________ > 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".