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

[Mark Thompson]

On 17/01/16 19:46, Mark Thompson wrote:
> 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.

Right, I see the problem.  The user will want to do something with the 
surface they get back under the same X11 display handle.  We can't call 
XOpenDisplay() in that case: the user has to be able to pass their own 
handle in.  So we need some other way to register that connection.

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