[vlc-devel] [PATCH 3/8] picture_pool: use the internal condition/lock indirectly

Rémi Denis-Courmont remi at remlab.net
Tue Jul 28 17:02:26 CEST 2020


Le tiistaina 28. heinäkuuta 2020, 8.33.49 EEST Steve Lhomme a écrit :
> >> In pull model it was easy. The pool belonged to the vout so it was not
> >> tied to such lock. It was possible to cancel the pool regardless of who
> >> is waiting. Now that pools are in many decoders (and in many forms) it
> >> becomes tricky to do this. But we need a solution anyway.
> > 
> > You can't expect to be able to pass a "cancellable" to each and every
> > place
> > that might sleep. Experience has shown that this just doesn't work.
> 
> Experience is a personal thing. I have never experienced anyone trying
> this before.
> My intuition tells me it's ugly, I agree. The "cancellable" objet can
> only be used by one pool (maybe it would need some attach/detach to
> ensure this). It could be recycled between pools of the same decoder.

I am referring to the project's interrupt framework, what led to that and what 
was there before. Specifically, you can't impose a specific mean of waiting, nor 
a specific instance of it. And once you fix that, you end up with exactly the 
same thing as the interrupt framework, including with all its intrinsic 
problems. In particular, you can only really interrupt for final stop/destroy, 
even though there are typically other wait scenarii that would need to be 
interruptible.

Besides, the picture pool used to be a mandatory interface between the core 
and the video output. But nowadays, it's just a convenience for allocating and 
recycling main RAM picture buffers. So if it does not suit a given use case, 
then there are no reasons to use it. And indeed, opaque decoder surfaces have 
always been managed by ad-hoc means, since the picture pool cannot allocate 
them. It makes even less sense to use the picture pool for that now than 
before.

> > If a module has a private object upon which it sleeps, it is responsible
> > for not locking itself up. For instance, access and access_demux modules
> > are responsible for stopping any asynchronous or threaded activities as
> > needed. A contrario, stream filter and demuxer modules rely on the
> > interrupt framework - because the sleeping happens within the underlying
> > stream that is given to them.
> 
> The picture pool isn't locked during picture_pool_Wait(). So it works as
> designed.
> 
> The problematic lock is `p_sys->lock` in avcodec/video.c.

In other words, it's a bug in the avcodec plugin. The decoder core and the 
pool helpers have nothing to do with it.

> When the picture_pool_Wait() is called, that lock is held. I don't think
> there's a way around this.

Well the obvious way around is not tot use picture_pool_Wait() or even 
picture_pool_t if it does not suit the given use case.

And a decoder cannot block the ES output or the video output on pause anyway. 
It's not only about pause then stop; it's pause then anything (other than 
resume).

(...)
> That would mean, contrary to what we said, decoders cannot handle their
> own picture pool.

I don't think we meant that decoders must use picture_pool_t. picture_pool_t 
is potentially convenient for software decoders. It never was convenient and 
probably never will be for hardware decoders whereby it's just a glorified 
free-list. You could just as well use a vlc_queue_t or a list and a lock to 
implement the picture pool as a *concept*.

If there was a problem with locking in the decoder core, then we'd have a 
problem. But this looks like a problem within avcodec here.

-- 
Rémi Denis-Courmont
http://www.remlab.net/





More information about the vlc-devel mailing list