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

[Nuo Mi]

>
>> > +                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.
>
Also, if we use this as start point of the native decoder, the native
decoder may need ref the picture header for multi-thread decoding.

>
>> > +    }
>> > +
>> > +    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.
>>
> Yes. It's a good solution to remove duplicate things

>
>> >       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".
>
>