[FFmpeg-devel] [PATCH 1/5] libavutil: Some VAAPI infrastructure

[Mark Thompson]

On 17/01/16 18:46, wm4 wrote:
>
> There are two issues:
> 1. global state in libav* which is not synchronized
> 2. thread-safety within
>
> 1. is is completely unacceptable, because it can trigger undefined
> behavior if there is more than 1 libav* user in the same process. I'm
> not really convinced that a "device string" is really reliably unique
> enough that it won't be a problem across library users. (For example,
> it's entirely possible enough to open 2 X11 Displays to the same X
> server using the same display name.)

Ok, I'm happy with the first part of that (and that it is fixable by a 
simple lock around the connection initialisation, assuming this code 
stays in libavutil).

Can you offer an example where the device strings actually create a problem?

Multiple users within the same process /must/ be given the same 
connection if they ask for the same device, because we have no way to 
distinguish different sets of instances which want to be able to work 
together.  Equally, two connections to the same device under different 
names are acceptably different, because they won't have come from the 
same instance set.

> With 2. it's a bit more complicated. There should probably indeed be
> something like a big lock around all uses of the same VADisplay, as
> long as libva exhibits this problem.

This is straightforward to do, if tedious.

Can you explain the ABI and API constraints on changes to existing 
structures?

For the existing decoders (and their users) to work, it will require either:
(a) a global list of connections somewhere to map VADisplay to lock
or
(b) an additional member in struct vaapi_context to point to the lock.

If ABI and API compatibility is required for existing users then (b) is 
out, and we have to have the global list (suitably locked).

If we can break both then the right answer is probably to pass 
hwaccel_context to encoders as well, and add a similar field to 
AVFilterContext to use there too.

If ABI compatibility is required but an API break is allowed then we 
could do horrible things to hack (b) into working.  For example, replace 
the VADisplay pointer in the first member of struct vaapi_context to 
instead point at a new structure which contains some magic bytes at the 
start.  If the magic bytes are where that pointer goes then we are using 
the new API and can lock using that, and if they are not found then it 
was a user-provided VADisplay and no locking is required.

- Mark


PS:  I have no attachment to this piece of code (around connection 
initialisation) at all; it was just required to make everything else 
work.  If you want to suggest a better and completely different approach 
then I am happy to throw it all away and start again.