[FFmpeg-devel] [PATCH v6 4/9] avcodec: add cbs for h266/vvc

Nuo Mi nuomi2021 at gmail.com
Thu Feb 18 17:14:15 EET 2021


On Thu, Feb 18, 2021 at 8:04 AM Mark Thompson <sw at jkqxz.net> wrote:

> On 17/02/2021 01:51, Nuo Mi wrote:
> > ---
> >   configure                             |    2 +
> >   libavcodec/Makefile                   |    1 +
> >   libavcodec/cbs.c                      |    6 +
> >   libavcodec/cbs_h2645.c                |  541 ++++-
> >   libavcodec/cbs_h266.h                 |  817 +++++++
> >   libavcodec/cbs_h266_syntax_template.c | 3006 +++++++++++++++++++++++++
> >   libavcodec/cbs_internal.h             |    3 +-
> >   libavcodec/cbs_sei.c                  |   29 +
> >   8 files changed, 4397 insertions(+), 8 deletions(-)
> >   create mode 100644 libavcodec/cbs_h266.h
> >   create mode 100644 libavcodec/cbs_h266_syntax_template.c
> >
> > diff --git a/configure b/configure
> > index a76c2ec4ae..11df59a229 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2355,6 +2355,7 @@ CONFIG_EXTRA="
> >       cbs_av1
> >       cbs_h264
> >       cbs_h265
> > +    cbs_h266
> >       cbs_jpeg
> >       cbs_mpeg2
> >       cbs_vp9
> > @@ -2623,6 +2624,7 @@ threads_if_any="$THREADS_LIST"
> >   cbs_av1_select="cbs"
> >   cbs_h264_select="cbs"
> >   cbs_h265_select="cbs"
> > +cbs_h266_select="cbs"
> >   cbs_jpeg_select="cbs"
> >   cbs_mpeg2_select="cbs"
> >   cbs_vp9_select="cbs"
> > diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> > index 3341801b97..23553e68e9 100644
> > --- a/libavcodec/Makefile
> > +++ b/libavcodec/Makefile
> > @@ -73,6 +73,7 @@ OBJS-$(CONFIG_CBS)                     += cbs.o
> cbs_bsf.o
> >   OBJS-$(CONFIG_CBS_AV1)                 += cbs_av1.o
> >   OBJS-$(CONFIG_CBS_H264)                += cbs_h2645.o cbs_sei.o
> h2645_parse.o
> >   OBJS-$(CONFIG_CBS_H265)                += cbs_h2645.o cbs_sei.o
> h2645_parse.o
> > +OBJS-$(CONFIG_CBS_H266)                += cbs_h2645.o cbs_sei.o
> h2645_parse.o
> >   OBJS-$(CONFIG_CBS_JPEG)                += cbs_jpeg.o
> >   OBJS-$(CONFIG_CBS_MPEG2)               += cbs_mpeg2.o
> >   OBJS-$(CONFIG_CBS_VP9)                 += cbs_vp9.o
> > diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> > index ecf22b3fdb..6baf21d62b 100644
> > --- a/libavcodec/cbs.c
> > +++ b/libavcodec/cbs.c
> > @@ -39,6 +39,9 @@ static const CodedBitstreamType *const
> cbs_type_table[] = {
> >   #if CONFIG_CBS_H265
> >       &ff_cbs_type_h265,
> >   #endif
> > +#if CONFIG_CBS_H266
> > +    &ff_cbs_type_h266,
> > +#endif
> >   #if CONFIG_CBS_JPEG
> >       &ff_cbs_type_jpeg,
> >   #endif
> > @@ -60,6 +63,9 @@ const enum AVCodecID ff_cbs_all_codec_ids[] = {
> >   #if CONFIG_CBS_H265
> >       AV_CODEC_ID_H265,
> >   #endif
> > +#if CONFIG_CBS_H266
> > +    AV_CODEC_ID_H266,
> > +#endif
> >   #if CONFIG_CBS_JPEG
> >       AV_CODEC_ID_MJPEG,
> >   #endif
> > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> > index 57c419aa05..d39464ba49 100644
> > --- a/libavcodec/cbs_h2645.c
> > +++ b/libavcodec/cbs_h2645.c
> > @@ -24,12 +24,129 @@
> >   #include "cbs_internal.h"
> >   #include "cbs_h264.h"
> >   #include "cbs_h265.h"
> > +#include "cbs_h266.h"
> >   #include "h264.h"
> >   #include "h264_sei.h"
> >   #include "h2645_parse.h"
> >   #include "hevc.h"
> >   #include "hevc_sei.h"
> > +#include "vvc.h"
> >
> > +#define H266_IS_SLICE(nut) (nut <= VVC_RASL_NUT || (nut >=
> VVC_IDR_W_RADL && nut <= VVC_GDR_NUT))
> > +#define H266_IS_IDR(nut) (nut == VVC_IDR_W_RADL || nut == VVC_IDR_N_LP)
> > +
> > +static void h266_au_detector_init(H266AuDetector* d)
> > +{
> > +    if (!d->inited) {
> > +        d->prev_layer_id = UINT8_MAX;
> > +        d->prev_poc = INT_MAX;
> > +        d->prev_tid0_poc = INT_MAX;
> > +        d->inited = 1;
> > +    }
> > +}
> > +
> > +//8.3.1 Decoding process for picture order count.
> > +//VTM did not follow the spec, and it's much simpler than spec.
> > +//We follow the VTM.
>
> To clarify, does that mean that the VTM is wrong or that the spec defines
> it in an equivalent way which is overly complicated?
>
Yes, It equals, Spec is over complicated. Spec defined many constrain for
encoder, decoder does not need  care about it.

>
> > +static int h266_get_slice_poc(CodedBitstreamH266Context *h266, void*
> log_ctx,
> > +                              const H266RawPH *ph, const
> H266RawSliceHeader *slice,
> > +                              int *poc)
> > +{
> > +    int poc_msb, max_poc_lsb, poc_lsb;
> > +    H266AuDetector* d = &h266->priv.au_detector;
> > +    const H266RawSPS* sps;
> > +    const H266RawPPS* pps;
>
> Please keep the '*'s as part of the declarator, not the
> declaration-specifiers (in lots of places in these functions).
>
Used a regex to search and replace. should be done now

>
> > +
> > +    pps = h266->pps[ph->ph_pic_parameter_set_id];
> > +    if (!pps) {
> > +        av_log(log_ctx, AV_LOG_ERROR, "PPS id %d is not avaliable.\n",
> > +               ph->ph_pic_parameter_set_id);
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    sps = h266->sps[pps->pps_seq_parameter_set_id];
> > +    if (!sps) {
> > +        av_log(log_ctx, AV_LOG_ERROR, "SPS id %d is not avaliable.\n",
> > +               pps->pps_seq_parameter_set_id);
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    max_poc_lsb = 1 << (sps->sps_log2_max_pic_order_cnt_lsb_minus4 + 4);
> > +    poc_lsb = ph->ph_pic_order_cnt_lsb;
> > +    if (H266_IS_IDR(slice->nal_unit_header.nal_unit_type)) {
> > +        if (ph->ph_poc_msb_cycle_present_flag)
> > +            poc_msb = ph->ph_poc_msb_cycle_val * max_poc_lsb;
> > +        else
> > +            poc_msb = 0;
> > +    } else {
> > +        int prev_poc = d->prev_tid0_poc;
> > +        int prev_poc_lsb = prev_poc & (max_poc_lsb - 1);
> > +        int prev_poc_msb = prev_poc - prev_poc_lsb;
> > +        if (ph->ph_poc_msb_cycle_present_flag) {
> > +             poc_msb = ph->ph_poc_msb_cycle_val * max_poc_lsb;
> > +        } else {
> > +            if ((poc_lsb < prev_poc_lsb) && ((prev_poc_lsb - poc_lsb)
> >= (max_poc_lsb / 2)))
> > +                poc_msb = prev_poc_msb + max_poc_lsb;
> > +            else if ((poc_lsb > prev_poc_lsb) && ((poc_lsb -
> prev_poc_lsb) > (max_poc_lsb / 2)))
> > +                poc_msb = prev_poc_msb - max_poc_lsb;
> > +            else
> > +                poc_msb = prev_poc_msb;
> > +        }
> > +    }
> > +
> > +    *poc = poc_msb + poc_lsb;
> > +    return 0;
> > +}
> > +
> > +int h266_is_au_start(CodedBitstreamH266Context *h266,
> CodedBitstreamFragment *pu, void* log_ctx)
>
> As an unexported global, this needs an "ff_" prefix.
>
fixed

>
> > +{
> > +    //7.4.2.4.3
> > +    H266AuDetector *d = &h266->priv.au_detector;
> > +    int i, ret;
> > +    const H266RawNALUnitHeader *nal;
> > +    const H266RawPH *ph = h266->ph;
> > +    const H266RawSlice *slice = NULL;
> > +    int poc, nut;
> > +
> > +    if (!ph) {
> > +        av_log(log_ctx, AV_LOG_ERROR, "can't find picture header in
> picutre unit.\n");
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +    for (i = 0; i < pu->nb_units; i++) {
> > +        nal = pu->units[i].content;
> > +        if (!nal)
> > +            continue;
> > +        if (H266_IS_SLICE(nal->nal_unit_type)) {
> > +            slice = pu->units[i].content;
> > +            break;
> > +        }
> > +    }
> > +    if (!slice) {
> > +        av_log(log_ctx, AV_LOG_ERROR, "can't find first slice.\n");
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    h266_au_detector_init(&h266->priv.au_detector);
> > +
> > +    if ((ret = h266_get_slice_poc(h266, log_ctx, ph, &slice->header,
> &poc)) < 0)
> > +        return ret;
> > +
> > +    ret = (nal->nuh_layer_id <= d->prev_layer_id) || (poc !=
> d->prev_poc);
> > +
> > +    nut = nal->nal_unit_type;
> > +    d->prev_layer_id = nal->nuh_layer_id;
> > +    d->prev_poc = poc;
> > +    if (nal->nuh_temporal_id_plus1 == 1 &&
> > +        !ph->ph_non_ref_pic_flag && nut != VVC_RADL_NUT && nut !=
> VVC_RASL_NUT) {
> > +        d->prev_tid0_poc = poc;
> > +    }
> > +    return ret;
> > +}
>
> None of this logic is used by the syntax tables, so I think move it down
> below them.
>
Done

>
> > +
> > +static av_always_inline unsigned int h266_ceil(unsigned int v, unsigned
> int align)
> > +{
> > +    return (((v) + (align) - 1) / (align));
> > +}
> >
> >   static int cbs_read_ue_golomb(CodedBitstreamContext *ctx,
> GetBitContext *gbc,
> >                                 const char *name, const int *subscripts,
> > ...
> > @@ -515,6 +641,12 @@ static int
> cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
> >       if (frag->data_size == 0)
> >           return 0;
> >
> > +    if (codec_id == AV_CODEC_ID_VVC) {
> > +        //we deactive picture header here to avoid reuse previous
> frame's ph.
> > +        CodedBitstreamH266Context* h266 = ctx->priv_data;
> > +        h266->ph = NULL;
> > +    }
> > +
> >       if (header && frag->data[0] && codec_id == AV_CODEC_ID_H264) {
> >           // AVCC header.
> >           size_t size, start, end;
> > @@ -716,6 +848,12 @@ static int
> cbs_h2645_replace_ps(CodedBitstreamContext *ctx,
> >       H2645_PS_TYPE(H265, HEVC_NAL_ ## cname, cname, uname, \
> >                     HEVC_MAX_ ## cname ## _COUNT, \
> >                     uname ## _ ## id_name, active_ ## uname)
> > +#define H266_PS_TYPE(cname, uname, id_name) \
> > +        H26456_PS_TYPE(H266, VVC_ ## cname ## _NUT, cname, uname, \
> > +            VVC_MAX_ ## cname ## _COUNT, \
> > +            offsetof(H266 ## Raw ## cname, uname ## _ ## id_name ##
> _parameter_set_id), \
> > +            0)
>
> As I said in the comment on the previous patch, offsetof(common) is zero
> so there is no need to mess around with adding more offsetof()s.
>
> > +#define H266_PH_TYPE  H26456_PS_TYPE(H266, VVC_PH_NUT, PH, ph, 1, 0, 0)
>
> I'm not convinced that reusing the replace-PS function for the PH is
> making this nicer (including the name).  Expanding the unref/ref inline
> looks simpler than adding extra special cases here.
>
I will change it

>
> >
> >       static const PSType ps_types[] = {
> >           H264_PS_TYPE(SPS, sps, seq),
> > @@ -723,6 +861,10 @@ static int
> cbs_h2645_replace_ps(CodedBitstreamContext *ctx,
> >           H265_PS_TYPE(VPS, vps, video),
> >           H265_PS_TYPE(SPS, sps, seq),
> >           H265_PS_TYPE(PPS, pps, pic),
> > +        H266_PS_TYPE(VPS, vps, video),
> > +        H266_PS_TYPE(SPS, sps, seq),
> > +        H266_PS_TYPE(PPS, pps, pic),
> > +        H266_PH_TYPE,
> >       };
> >
> >       const PSType *ps_type;
> > @@ -1019,6 +1161,131 @@ static int
> cbs_h265_read_nal_unit(CodedBitstreamContext *ctx,
> >       return 0;
> >   }
> >
> > ...
> >   static int cbs_h2645_unit_requires_zero_byte(enum AVCodecID codec_id,
> >                                                CodedBitstreamUnitType
> type,
> > -                                             int nal_unit_index)
> > +                                             int nal_unit_index, int
> h266_au_start)
> >   {
> > -    // Section B.1.2 in H.264, section B.2.2 in H.265.
> > +    // Section B.1.2 in H.264, section B.2.2 in H.265, H.266.
> >       if (nal_unit_index == 0) {
> > -        // Assume that this is the first NAL unit in an access unit.
> > -        return 1;
> > +        if (codec_id == AV_CODEC_ID_VVC) {
> > +            if (h266_au_start)
> > +                return 1;
> > +        } else {
> > +            // Assume that this is the first NAL unit in an access unit.
> > +            return 1;
> > +        }
> >       }
> >       if (codec_id == AV_CODEC_ID_H264)
> >           return type == H264_NAL_SPS || type == H264_NAL_PPS;
> >       if (codec_id == AV_CODEC_ID_HEVC)
> >           return type == HEVC_NAL_VPS || type == HEVC_NAL_SPS || type ==
> HEVC_NAL_PPS;
> > +    if (codec_id == AV_CODEC_ID_VVC)
> > +        return type >= VVC_OPI_NUT && type <= VVC_SUFFIX_APS_NUT; >
>    return 0;
> >   }
> >
> > @@ -1327,7 +1717,7 @@ static int
> cbs_h2645_assemble_fragment(CodedBitstreamContext *ctx,
> >   {
> >       uint8_t *data;
> >       size_t max_size, dp, sp;
> > -    int err, i, zero_run;
> > +    int err, i, zero_run, h266_au_start = 0;
> >
> >       for (i = 0; i < frag->nb_units; i++) {
> >           // Data should already all have been written when we get here.
> > @@ -1343,7 +1733,10 @@ static int
> cbs_h2645_assemble_fragment(CodedBitstreamContext *ctx,
> >       data = av_realloc(NULL, max_size + AV_INPUT_BUFFER_PADDING_SIZE);
> >       if (!data)
> >           return AVERROR(ENOMEM);
> > -
> > +    if (ctx->codec->codec_id == AV_CODEC_ID_VVC) {
> > +        CodedBitstreamH266Context *h266 = ctx->priv_data;
> > +        h266_au_start = h266_is_au_start(h266, frag, ctx->log_ctx) > 0;
> > +    }
>
> It is unclear to me why this wants a special case.
>
> The current logic is that we are writing an AU, so the first NAL unit in
> it is necessarily an AU start and subsequent NAL units are not?
>
H.266 AU contains one or more PU(3.105). One PU contains one coded picture.
Only the first NAL unit of an AU needs the zero_byte(B.2.2)
H.265 has no PU, each AU has one coded picture(3.1)
H.266's PU is the H.265's AU. In vvc_parser, we split bitstream to PU.

>
> >       dp = 0;
> >       for (i = 0; i < frag->nb_units; i++) {
> >           CodedBitstreamUnit *unit = &frag->units[i];
> > @@ -1356,7 +1749,7 @@ static int
> cbs_h2645_assemble_fragment(CodedBitstreamContext *ctx,
> >                   frag->data_bit_padding = unit->data_bit_padding;
> >           }
> >
> > -        if (cbs_h2645_unit_requires_zero_byte(ctx->codec->codec_id,
> unit->type, i)) {
> > +        if (cbs_h2645_unit_requires_zero_byte(ctx->codec->codec_id,
> unit->type, i, h266_au_start)) {
> >               // zero_byte
> >               data[dp++] = 0;
> >           }
> > @@ -1470,6 +1863,37 @@ static void cbs_h265_close(CodedBitstreamContext
> *ctx)
> >           av_buffer_unref(&h265->pps_ref[i]);
> >   }
> >
> > ...
> > diff --git a/libavcodec/cbs_h266.h b/libavcodec/cbs_h266.h
> > new file mode 100644
> > index 0000000000..20d547a26d
> > --- /dev/null
> > +++ b/libavcodec/cbs_h266.h
> > @@ -0,0 +1,817 @@
> > +/*
> > + * 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 AVCODEC_CBS_H266_H
> > +#define AVCODEC_CBS_H266_H
> > +
> > +#include <stddef.h>
> > +#include <stdint.h>
> > +
> > +#include "cbs_h2645.h"
> > +#include "cbs_sei.h"
> > +#include "vvc.h"
> > +
> > +enum {
> > +    // This limit is arbitrary - it is sufficient for one message of
> each
> > +    // type plus some repeats, and will therefore easily cover all sane
> > +    // streams.  However, it is possible to make technically-valid
> streams
> > +    // for which it will fail (for example, by including a large number
> of
> > +    // user-data-unregistered messages).
> > +    VVC_MAX_SEI_PAYLOADS = 64,
>
> Not used anywhere.
>
removed

>
> > +};
> > +
> > +typedef struct H266RawNALUnitHeader {
> > +    uint8_t nuh_layer_id;
> > +    uint8_t nal_unit_type;
> > +    uint8_t nuh_temporal_id_plus1;
> > +    uint8_t nuh_reserved_zero_bit;
> > +} H266RawNALUnitHeader;
> > +
> > ...
> > +
> > +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];
> > +
> > +    uint8_t  ph_sei_reserved_zero_7bits;
>
> "dph"
>
fixed

>
> > +} H266RawSEIDecodedPictureHash;
> > +
> > +typedef struct H266RawSEIPayload {
> > +    uint32_t payload_type;
> > +    uint32_t payload_size;
> > +    union {
> > +        H266RawSEIDecodedPictureHash decoded_picture_hash;
> > +        struct {
> > +            uint8_t     *data;
> > +            AVBufferRef *data_ref;
> > +            size_t       data_length;
> > +        } other;
> > +    } payload;
> > +    H266RawExtensionData extension_data;
> > +} H266RawSEIPayload;
>
> Not used.
>
removed

>
> > +
> > +typedef struct H266RawSEI {
> > +    H266RawNALUnitHeader nal_unit_header;
> > +    SEIRawMessageList    message_list;
> > +} H266RawSEI;
> > +
> > +typedef struct H266AuDetector {
> > +    uint8_t inited;
> > +    uint8_t prev_layer_id;
> > +    int prev_tid0_poc;
> > +    int prev_poc;
> > +} H266AuDetector;
> > +
> > +typedef struct CodedBitstreamH266Context {
> > +    // Reader/writer context in common with the H.264 implementation.
> > +    CodedBitstreamH2645Context common;
> > +
> > +    // All currently available parameter sets.  These are updated when
> > +    // any parameter set NAL unit is read/written with this context.
> > +    AVBufferRef *vps_ref[VVC_MAX_VPS_COUNT];
> > +    AVBufferRef *sps_ref[VVC_MAX_SPS_COUNT];
> > +    AVBufferRef *pps_ref[VVC_MAX_PPS_COUNT];
> > +    AVBufferRef *ph_ref;
> > +    H266RawVPS  *vps[VVC_MAX_SPS_COUNT];
> > +    H266RawSPS  *sps[VVC_MAX_SPS_COUNT];
> > +    H266RawPPS  *pps[VVC_MAX_PPS_COUNT];
> > +    H266RawPH   *ph;
> > +
> > +    struct {
> > +        H266AuDetector au_detector;
> > +    } priv;
>
> What is the extra struct layer doing?  This is already the private context
> for an H.266 CBS instance.
>
The caller can touch the sps and pps. But, for au_detector, it's should not
use by the caller, it can only be used by the ff_h266_is_au_start function

>
> > +
> > +} CodedBitstreamH266Context;
> > +
> > ... > +
> > +static int FUNC(ph)(CodedBitstreamContext *ctx, RWContext *rw,
> H266RawPH *current)
> > +{
> > +    int err;
> > +
> > +    HEADER("Picture Header");
> > +
> > +    CHECK(FUNC(nal_unit_header)(ctx, rw, &current->nal_unit_header,
> VVC_PH_NUT));
> > +    CHECK(FUNC(picture_header)(ctx, rw, current));
> > +    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, &current->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,
> &current->sh_picture_header));
> > +        if (!h266->ph_ref) {
> > +            h266->ph_ref = av_buffer_allocz(sizeof(H266RawPH));
> > +            if (!h266->ph_ref)
> > +                return AVERROR(ENOMEM);
> > +        }
> > +        h266->ph = (H266RawPH*)h266->ph_ref->data;
> > +        memcpy(h266->ph, &current->sh_picture_header,
> sizeof(H266RawPH));
>
> This memcpy() is naughty - if that was a ref to a previous picture header
> then you've just overwritten the reference while the other unit still holds
> it.
>
> If the slice is unit->content then unit->content_ref is a reference to a
> superstructure of the picture header, avoiding the copy, but I'm not sure
> it's the right way to do it.
>
> Is it right that the two possible options here are:
> * Store a ref, which might be to a newly-allocated buffer or might be to a
> previous unit.
>
* Store the whole structure in the private context.
>
?
>
> If the structure isn't too large, then the second option feels much
> simpler.
>
> If it is large such that holding references and avoiding copies is
> beneficial (which is quite large, every ref entails a malloc/free
> call-pair), then we probably want that ref to always be to the unit?
>
If we store the whole structure, it's hard to detect the picture header
missing case.

>
> > +    }
> > +
> > +    ph = h266->ph;
> > +    if (!ph) {
> > +        av_log(ctx->log_ctx, AV_LOG_ERROR, "Picture header not
> available.\n");
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > ...
> > diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h
> > index a392880036..118b1052d4 100644
> > --- a/libavcodec/cbs_internal.h
> > +++ b/libavcodec/cbs_internal.h
> > @@ -40,7 +40,7 @@ enum CBSContentType {
> >   enum {
> >         // Maximum number of unit types described by the same unit type
> >         // descriptor.
> > -      CBS_MAX_UNIT_TYPES  = 3,
> > +      CBS_MAX_UNIT_TYPES  = 4,
>
> Heh, fair :)
>
> >         // Maximum number of reference buffer offsets in any one unit.
> >         CBS_MAX_REF_OFFSETS = 2,
> >         // Special value used in a unit type descriptor to indicate that
> it
> > @@ -204,6 +204,7 @@ int ff_cbs_write_signed(CodedBitstreamContext *ctx,
> PutBitContext *pbc,
> >   extern const CodedBitstreamType ff_cbs_type_av1;
> >   extern const CodedBitstreamType ff_cbs_type_h264;
> >   extern const CodedBitstreamType ff_cbs_type_h265;
> > +extern const CodedBitstreamType ff_cbs_type_h266;
> >   extern const CodedBitstreamType ff_cbs_type_jpeg;
> >   extern const CodedBitstreamType ff_cbs_type_mpeg2;
> >   extern const CodedBitstreamType ff_cbs_type_vp9;
> > diff --git a/libavcodec/cbs_sei.c b/libavcodec/cbs_sei.c
> > index c49830ad77..96c60259ce 100644
> > --- a/libavcodec/cbs_sei.c
> > +++ b/libavcodec/cbs_sei.c
> > @@ -20,6 +20,7 @@
> >   #include "cbs_internal.h"
> >   #include "cbs_h264.h"
> >   #include "cbs_h265.h"
> > +#include "cbs_h266.h"
> >   #include "cbs_sei.h"
> >
> >   static void cbs_free_user_data_registered(void *opaque, uint8_t *data)
> > @@ -132,6 +133,13 @@ static int cbs_sei_get_unit(CodedBitstreamContext
> *ctx,
> >           else
> >               sei_type = HEVC_NAL_SEI_SUFFIX;
> >           break;
> > +    case AV_CODEC_ID_H266:
> > +        highest_vcl_type = VVC_RSV_IRAP_11;
> > +        if (prefix)
> > +            sei_type = VVC_PREFIX_SEI_NUT;
> > +        else
> > +            sei_type = VVC_SUFFIX_SEI_NUT;
> > +        break;
> >       default:
> >           return AVERROR(EINVAL);
> >       }
> > @@ -207,6 +215,18 @@ static int cbs_sei_get_unit(CodedBitstreamContext
> *ctx,
> >               memcpy(unit->content, &sei, sizeof(sei));
> >           }
> >           break;
> > +    case AV_CODEC_ID_H266:
> > +        {
> > +            H266RawSEI sei = {
> > +                .nal_unit_header = {
> > +                    .nal_unit_type         = sei_type,
> > +                    .nuh_layer_id          = 0,
> > +                    .nuh_temporal_id_plus1 = 1,
> > +                },
> > +            };
> > +            memcpy(unit->content, &sei, sizeof(sei));
> > +        }
> > +        break;
> >       default:
> >           av_assert0(0);
> >       }
> > @@ -237,6 +257,15 @@ static int
> cbs_sei_get_message_list(CodedBitstreamContext *ctx,
> >               *list = &sei->message_list;
> >           }
> >           break;
> > +    case AV_CODEC_ID_H266:
> > +        {
> > +            H266RawSEI *sei = unit->content;
> > +            if (unit->type != VVC_PREFIX_SEI_NUT &&
> > +                unit->type != VVC_SUFFIX_SEI_NUT)
> > +                return AVERROR(EINVAL);
> > +            *list = &sei->message_list;
> > +        }
> > +        break;
>
> Yay, I'm glad the SEI stuff fitted in nicely with no extra weird cases.
>
> >       default:
> >           return AVERROR(EINVAL);
> >       }
> >
> Thanks,
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".


More information about the ffmpeg-devel mailing list