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

[Rémi Denis-Courmont]

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.

But even if... then the per-thread state should be in the thread structure, 
and the thread index should be unnecessary.

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

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

-- 
レミ・デニ-クールモン
http://www.remlab.net/