Hi Andreas,
thank you for the review.
On Fri, Dec 8, 2023 at 8:17 PM Andreas Rheinhardt <
andreas.rheinha...@outlook.com> wrote:
>
> > +
> > +static int min_pu_arrays_init(VVCFrameContext *fc, const int
> pic_size_in_min_pu)
> > +{
> > + if (fc->tab.pic_size_in_min_pu != pic_size_in_min_pu) {
> > + min_pu_arrays_free(fc);
> > + fc->tab.msf = av_mallocz(pic_size_in_min_pu);
> > + fc->tab.iaf = av_mallocz(pic_size_in_min_pu);
> > + fc->tab.mmi = av_mallocz(pic_size_in_min_pu);
> > + fc->tab.mvf = av_mallocz(pic_size_in_min_pu *
> sizeof(*fc->tab.mvf));
>
> Do these have to be separate allocations? If there were allocated
> jointly, one memset below would suffice.
>
They are separate flags, if we combine them. We can't use memset to set
flags for a block.
>
> > +
> > + if (!fc->cu_pool) {
> > + fc->cu_pool = ff_refstruct_pool_alloc(sizeof(CodingUnit), 0);
> > + if (!fc->cu_pool)
> > + goto fail;
>
> The size of the objects contained in this pool don't depend on any
> bitstream parameters. You can therefore simply use a single pool (in
> VVCContext) that is allocated in vvc_decode_init() and freed in
> vvc_decode_free().
> The same goes for tu_pool below.
>
A global pool may have a performance issue for huge thread number.
Move it to frame_context_init
>
>
>
> > +static int slices_realloc(VVCFrameContext *fc)
> > +{
> > + void *p;
> > + const int size = (fc->nb_slices_allocated + 1) * 3 / 2;
> > +
> > + if (fc->nb_slices < fc->nb_slices_allocated)
> > + return 0;
> > +
> > + p = av_realloc(fc->slices, size * sizeof(*fc->slices));
>
> av_realloc_array()
>
done
>
> > + if (!p)
> > + return AVERROR(ENOMEM);
> > +
> > + fc->slices = p;
> > + for (int i = fc->nb_slices_allocated; i < size; i++) {
> > + fc->slices[i] = av_calloc(1, sizeof(*fc->slices[0]));
>
> av_mallocz().
>
done
>
> > + if (!fc->slices[i]) {
> > + for (int j = fc->nb_slices_allocated; j < i; j++)
> > + av_freep(&fc->slices[j]);
> > + return AVERROR(ENOMEM);
>
> Can't you simply set fc->nb_slices_allocated to i in order to avoid this
> loop?
>
done
> > +
> > +static int init_slice_context(SliceContext *sc, VVCFrameContext *fc,
> const H2645NAL *nal, const CodedBitstreamUnit *unit)
> > +{
> > + const VVCSH *sh = &sc->sh;
> > + const H266RawSlice *slice = (const H266RawSlice *)unit->content;
>
> Please no pointless casts. Also, why is there unnecessary whitespace in
> front of '='?
>
Fix here and serval other places
The whitespace will make all = in a col.
> > + int nb_eps = sh->r->num_entry_points + 1;
> > + int ctu_addr = 0;
> > + GetBitContext gb;
> > +
> > + if (sc->nb_eps != nb_eps) {
> > + eps_free(sc);
> > + sc->eps = av_calloc(nb_eps, sizeof(*sc->eps));
> > + if (!sc->eps)
> > + return AVERROR(ENOMEM);
>
> In case of error, sc->eps is NULL, yet sc->nb_eps may be != 0. Stuff
> like this can (and does) lead to crashes.
>
added "slice->nb_eps = 0;" to eps_free
>
> > +static int vvc_ref_frame(VVCFrameContext *fc, VVCFrame *dst, VVCFrame
> *src)
>
> src should be const.
>
done
>
> > +
> > +static av_cold int frame_context_init(VVCFrameContext *fc,
> AVCodecContext *avctx)
> > +{
> > +
> > + fc->avctx = av_memdup(avctx, sizeof(*avctx));
>
> When I read this, I presumed you are using multiple AVCodecContexts to
> store the ever changing state of the AVCodecContext fields similarly to
> update_context_from_thread() in pthread_frame.c. But it seems you don't.
> These contexts are only used as a) logcontexts (where the actual
> user-facing AVCodecContext should be used, so that the user can make
> sense of the logmessages!), b) in ff_thread_get_buffer() and c) in
> export_frame_params() where only some basic fields
> (dimension-related+pix_fmt) is set. Presumably c) is done for b).
>
I remember if i did not use a local AVCodecContext it would trigger some
assert when resolution changed.
>
> But the user is allowed to change the provided callbacks in the master
> context at any time. E.g. the call to ff_thread_get_buffer() in
> vvc_refs.c currently uses the VVCFrameContext and therefore uses the
> get_buffer2 callback in place now (during av_memdup()). This is wrong.
>
This will not happen. av_memdup only happens in vvc_decode_init.
Nobody will call ff_thread_get_buffer at this time
>
> I think you can just remove VVCFrameContext.avctx and use the
> user-facing AVCodecContext if you set the AVFrame properties that are
> normally derived from the AVCodecContext directly on the AVFrame before
> ff_thread_get_buffer().
Could you explain more about how to create a user-facing AVCodecContext?
>
> > +
> > +static int decode_nal_unit(VVCContext *s, VVCFrameContext *fc, const
> H2645NAL *nal, const CodedBitstreamUnit *unit)
> > +{
> > + int ret;
> > +
> > + s->temporal_id = nal->temporal_id;
> > +
> > + switch (unit->type) {
> > + case VVC_VPS_NUT:
> > + case VVC_SPS_NUT:
> > + case VVC_PPS_NUT:
> > + /* vps, sps, sps cached by s->cbc */
> > + break;
> > + case VVC_TRAIL_NUT:
> > + case VVC_STSA_NUT:
> > + case VVC_RADL_NUT:
> > + case VVC_RASL_NUT:
> > + case VVC_IDR_W_RADL:
> > + case VVC_IDR_N_LP:
> > + case VVC_CRA_NUT:
> > + case VVC_GDR_NUT:
> > + ret = decode_slice(s, fc, nal, unit);
> > + if (ret < 0)
> > + goto fail;
> > + break;
> > + case VVC_PREFIX_APS_NUT:
> > + case VVC_SUFFIX_APS_NUT:
> > + ret = ff_vvc_decode_aps(&s->ps, unit);
> > + if (ret < 0)
> > + goto fail;
> > + break;
> > + default:
> > + av_log(s->avctx, AV_LOG_INFO,
> > + "Skipping NAL unit %d\n", unit->type);
>
> This will probably be very noisy (and warn for every SEI). I don't think
> it is even needed, as h2645_parse.c already contains debug log messages
> to display the unit type.
>
It's copied from hevcdec. It means we did not handle the nal diffrent than
h2645_parser.c messages
A fail that is only "return ret" is pointless (not only here).
>
At someday if we need to add some cleanup code. we do not need to change
all returns to goto.
>
> > static void vvc_decode_flush(AVCodecContext *avctx)
>
> Should also be av_cold
>
done
>
> > {
> > + VVCContext *s = avctx->priv_data;
> > + int got_output;
> > + AVFrame *output = av_frame_alloc();
>
> Allocating a frame for flushing is bad enough, but you are only flushing
> if said allocating succeeds. If it does not, then we never wait for
> frames which are currently decoded by other threads, don't we? So there
> can be races and even crashes when this function is called from
> vvc_decode_free() and allocation.
> Instead you could pass NULL to wait_delayed_frame() and make it unref
> the frames (instead of moving them) in case the output frame is NULL.
>
done
>
> > +
> > + if (output) {
> > + while (s->nb_delayed) {
> > + wait_delayed_frame(s, output, &got_output);
> > + if (got_output) {
> > + av_frame_unref(output);
> > + }
> > + }
> > + av_frame_free(&output);
> > + }
> > }
> >
> > static av_cold int vvc_decode_free(AVCodecContext *avctx)
> > {
> > + VVCContext *s = avctx->priv_data;
> > + int i;
> > +
> > + ff_cbs_fragment_free(&s->current_frame);
>
> Is it sure that the fragment is not in use (given that other threads may
> be running now before vvc_decode_flush())?
>
Do you mean the executor threads? If they want to use some data, they will
take their own hip.
see ff_refstruct_replace(&sps->r, rsps);
>
> > + vvc_decode_flush(avctx);
> > + ff_vvc_executor_free(&s->executor);
> > + if (s->fcs) {
> > + for (i = 0; i < s->nb_fcs; i++)
>
> for (int i = 0; is better as it has smaller scope; in this case, it also
> allows to save a line of code. Something similar is possible in
> decode_nal_units(), please check the other patches, too.
Yeah, Most of the code uses the smallest scope. But some codes are copied
from hevc or just because I missed them. :)
double double-checked and fixed all(hope so)
>
> > + frame_context_free(s->fcs + i);
> > + av_free(s->fcs);
> > + }
> > + ff_vvc_ps_uninit(&s->ps);
> > + ff_cbs_close(&s->cbc);
> > +
> > return 0;
> > }
> >
> > +#define VVC_MAX_FRMAE_DELAY 16
>
> typo
>
fixed
>
> > static av_cold int vvc_decode_init(AVCodecContext *avctx)
> > {
> > + VVCContext *s = avctx->priv_data;
> > + int ret;
> > +
> > + s->avctx = avctx;
> > +
> > + if (ff_cbs_init(&s->cbc, AV_CODEC_ID_VVC, avctx))
> > + goto fail;
>
> Forward the error code.
>
done
>
> > +
> > + s->nb_fcs = (avctx->flags & AV_CODEC_FLAG_LOW_DELAY) ? 1 :
> FFMIN(av_cpu_count(), VVC_MAX_FRMAE_DELAY);
>
> This may evaluate av_cpu_count() multiple times. Furthermore I don't
> know why this define is used here at all: With frame threading, the
> number of frame threads is not limited by the delay/number of reordering
> frames at all (we even have frame-threading for decoders without
> frame-reordering at all).
>
vvc_decode_frame only allows 1 frame in 1 frame out. We can remove the
delay if we switch to FFCodec->receive_frame,
>
> But worst of this is that you do not check avctx->thread_count at all.
>
we do not use avctx->thread_count. we use the executor to manage threads.
>
> > + s->fcs = av_calloc(s->nb_fcs, sizeof(*s->fcs));
> > + if (!s->fcs)
> > + goto fail;
> > +
> > + for (int i = 0; i < s->nb_fcs; i++) {
> > + VVCFrameContext *fc = s->fcs + i;
> > + ret = frame_context_init(fc, avctx);
> > + if (ret < 0)
> > + goto fail;
> > + }
> > +
> > + s->executor = ff_vvc_executor_alloc(s, s->nb_fcs);
> > + if (!s->executor)
> > + goto fail;
> > +
> > + s->eos = 1;
> > + GDR_SET_RECOVERED(s);
> > + memset(&ff_vvc_default_scale_m, 16, sizeof(ff_vvc_default_scale_m));
>
> This needs to be done once (i.e. protected by an AVOnce) and not every
> time a decoder is set up. Otherwise there might be data races.
>
It's not read and set, it will no data races:), I can change it to AVOnce
.
>
> > +
> > return 0;
> > +
> > +fail:
> > + vvc_decode_free(avctx);
>
> Unnecessary, as this decoder has the FF_CODEC_CAP_INIT_CLEANUP set. In
> fact, given that vvc_decode_free() uses av_free() instead of av_freep()
> for s->fcs, calling vvc_decode_free() here can lead to a use-after-free
> (namely when vvc_decode_free() is called generically later).
>
done
>
> > + return AVERROR(ENOMEM);
> > }
> >
> > const FFCodec ff_vvc_decoder = {
>
> _______________________________________________
> 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".
