Re: [FFmpeg-devel] [PATCH 01/14] vvcdec: add thread executor

[Nuo Mi <nuomi2021@gmail.com>]

On Tue, May 23, 2023 at 12:29 AM Rémi Denis-Courmont <remi@remlab.net>
wrote:

> Le sunnuntaina 21. toukokuuta 2023, 17.24.56 EEST Nuo Mi a écrit :
> > > > +
> > > > +typedef struct ThreadInfo {
> > > > +    int idx;
> > > > +    VVCExecutor *e;
> > > > +    pthread_t thread;
> > > > +} ThreadInfo;
> > > > +
> > > > +struct VVCExecutor {
> > > > +    VVCTaskCallbacks cb;
> > > > +    ThreadInfo *threads;
> > > > +    uint8_t *local_contexts;
> > >
> > > It seems odd and needless complex to separate this from the thread
> info.
> > > It
> > > looks like you could simply append a pointer or a flexible array to
> > > ThreadInfo
> > > instead.
> >
> >  Do you mean in VVCTasklet? No, we do not want to expose ThreadInfo
> details
> > to users.
>
> First, it feels very weird for a generic thread pool mechanism to have
> opaque
> per-thread state at all. AFAIK, any client state should be in the jobs,
> not
> the threads.
>
Decoders need some temporary buffer.  We call it local context.
They are stateless. This means it is invalid before and after the job. This
is why we allocate it for thread instead of job.

>
> But even if... then the per-thread state should be in the thread
> structure,
> and the thread index should be unnecessary.
>
I can move local_context to ThreadInfo and remove idx if you prefer.
But this will do multiple allocations for the local context. Make the
allocation fail handling complicated.

>
> > > Signaling a condition variable without changing any state makes no
> sense.
> >
> > It's for error handling.
>
> Yeah, no.
>
> > Imaging last all thread are waiting for ready jobs except the last one.
> > The last one thread has some error, it reports an error to the caller.
> > The caller needs to wake up threads to flush the task list.
>

> That's not what this code does.
>
Currently, it is only for missed slices. all tasks for the frame will abort.
In the future, it may extend to error resilience and early quit for B frame.


> And you don't need to wake up all threads to flush the task list. There
> are two
> situations where it makes sense to wake more than one thread:
> - When you want all to terminate to destroy the pool.
> - When you have atomically queued so many jobs that you want to wake up
> idle
> runners at once, rather than signal them one at a time. (This is really
> only
> an optimisation, and only makes sense if you have really a lot of threads.)
>
It's more than this. Many blocks' condition may change, it can't handle
efficiently by one thread.
For example, If the cpu usage is high, you want to skip all queued B frames
and its tasks.
Wake Up all threads will speedup the process and free frame slot as quickly
as possible.

Thank you.

>
> --
> レミ・デニ-クールモン
> http://www.remlab.net/
>
>
>
> _______________________________________________
> 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".
>
_______________________________________________
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".