[FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions

[Mark Thompson]

On 02/05/2022 23:59, Soft Works wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Mark
>> Thompson
>> Sent: Tuesday, May 3, 2022 12:12 AM
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-
>> device function which searches for existing devices in both directions
>>
>> On 02/05/2022 09:14, Soft Works wrote:
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> Mark
>>>> Thompson
>>>> Sent: Monday, May 2, 2022 12:01 AM
>>>> To: ffmpeg-devel at ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add
>> derive-
>>>> device function which searches for existing devices in both
>> directions
>>>
>>> [..]
>>>
>>>>>> * The thread-safety properties of the hwcontext API have been
>> lost
>>>> -
>>>>>> you can no longer operate on devices independently across threads
>>>>>> (insofar as the underlying API allows that).
>>>>>>       Maybe there is an argument that derivation is something
>> which
>>>>>> should happen early on and therefore documenting it as thread-
>>>> unsafe
>>>>>> is ok, but when hwupload/hwmap can use it inside filtergraphs
>> that
>>>>>> just isn't going to happen (and will be violated in the FFmpeg
>>>> utility
>>>>>> if filters get threaded, as is being worked on).
>>>>>
>>>>>    From my understanding there will be a single separate thread
>> which
>>>>> handles all filtergraph operations.
>>>>> I don't think it would even be possible (without massive changes)
>>>>> to arbitrate filter processing in parallel.
>>>>> But even if this would be implemented: the filtergraph setup
>> (init,
>>>>> uninit, query_formats, etc.) would surely happen on a single
>> thread.
>>>>
>>>> The ffmpeg utility creates filtergraphs dynamically when the first
>>>> frame is available from their inputs, so I don't see why you
>> wouldn't
>>>> allow multiple of them to be created in parallel in that case.
>>>>
>>>> If you create all devices at the beginning and then give references
>> to
>>>> them to the various filters which need them (so never manipulate
>>>> devices dynamically within the graph) then it would be ok, but I
>> think
>>>> you've already rejected that approach.
>>>
>>> Please let's not break out of the scope of this patchset.
>>> This patchset is not about re-doing device derivation. The only
>>> small change that it does is that it returns an existing device
>>> instead of creating a new one when such device already exists
>>> in the same(!) chain.
>>>
>>> The change it makes has really nothing to do with threading or
>>> anything around it.
>>
>> The change makes existing thread-safe hwcontext APIs unsafe.  That is
>> definitely not "nothing to do with threading", and has to be resolved
>> since they can currently be called from contexts which expect thread-
>> safety (such as making filtergraphs).
> 
> As I said, I'm not aware that filtergraph creation would be a context
> which requires thread-safety, even when considering frames which get
> pushed into a filtergraph (like from a decoder).

User programs can do whatever they like within the API constraints.

> Could you please show a command line which causes a situation where
> device_derive is being called from multiple threads?

Given that the ffmpeg utility is currently entirely single-threaded, this will only happen once the intended plans for adding threading to it are complete.

With that, it will be true for something which makes two filtergraphs and uses derivation in both of them.  For example:, this currently makes two independent VAAPI devices, but equally could reuse the same device:

# ffmpeg -y -f kmsgrab -i /dev/dri/card0 -vf hwmap=derive_device=vaapi,scale_vaapi=format=nv12 -c:v h264_vaapi out1.mp4 -vf hwmap=derive_device=vaapi,scale_vaapi=w=1280:h=720:format=nv12 -c:v h264_vaapi out2.mp4

>>>>>> * I'm not sure that it is reasonable to ignore options.  If an
>>>>>> unrelated component derived a device before you with special
>>>> options,
>>>>>> you might get that device even if you have incompatible different
>>>>>> options.
>>>>>
>>>>> I understand what you mean, but this is outside the scope of
>>>>> this patchset, because when you would want to do this, it
>>>>> would need to be implemented for derivation in general, not
>>>>> in this patchset which only adds reverse-search to the
>>>>> existing derivation functionality.
>>>>
>>>> I'm not sure what you mean by that?  The feature already exists;
>> here
>>>> is a concrete example of where you would get the wrong result:
>>>>
>>>> Start with VAAPI device A.
>>>>
>>>> Component P derives Vulkan device B with some extension options X.
>>>>
>>>> Component Q wants the same device as P, so it derives again with
>>>> extension options X and gets B.
>>>>
>>>> Everything works fine for a while.
>>>>
>>>> Later, unrelated component R is inserted before P and Q.  It wants
>> a
>>>> Vulkan device C with extension options Y, so it derives that.
>>>>
>>>> Now component Q is broken because it gets C instead of B and has
>> the
>>>> wrong extensions enabled.
>>>
>>> As per your request, this patchset's changes are now limited to
>>> use ffmpeg cli. And there is currently no support for "extension
>>> options" when deriving a device.
>>
>> Yes there is - see the "instance_extensions" and "device_extensions"
>> options to Vulkan device derivation.  (Hence the VAAPI+Vulkan
>> example.)
> 
> OK, but when deriving a device via command line, it doesn't consider
> such parameters in the current device_derive function, and hence it's
> not something that can be burdened onto this patchset.

Then it sounds like you want this function to be part of the ffmpeg utility, not inside one of the libraries which have other users.

>>> What I meant above is this:
>>>
>>> Assume this patchset wouldn't be applied, but a patchset would
>>> be applied that allows to specify "extension options".
>>> Then, even without this patchset, I could construct a similar
>>> example, where you would get the same device when deriving
>>> two times from the same device but with different extension
>>> options.
>>
>> How?
> 
> VAAPI >> QSV(Param1) >> OpenCL
> 
> Now, from OpenCL, you want to derive QSV but with different parameters
> (Param2). You won't get a new device, you get the existing QSV device.

This doesn't make sense - remember that device derivation is unidirectional, and OpenCL is a leaf API which can only derive from other things.  The "derive" operation there has to be interpreted as "return the device this was derived from", in which case options don't make sense.

(Indeed, it seems like a good idea to disallow options in that case.  I will prepare a patch to that effect.)

>> The existing derivation setup always makes a new device, so you can't
>> accidentally get an old one.
> 
> No, not always, see above.
> 
>> The existing way of reusing devices is to keep the reference and reuse
>> it directly, which means you need to pass the reference around
>> explicitly and there is no problem.
> 
> You can do this as API user, but this patch is about ffmpeg cli and as
> per your request limited to ffmpeg cli usage.
> 
> 
>>>> Can you explain your example further?
>>>
>>> Maybe we should get clear about what this patchset does exactly.
>>> Let's look at the following derivation chain of devices:
>>>
>>> A
>>> ├─ X
>>> │  └─ Y
>>> ├─ B
>>> │  └─ C
>>> └─ V
>>>      └─ W
>>>
>>> The meaning is:
>>>
>>> - Y is derived from X, X is derived from A
>>> - C is derived from B, B is derived from A
>>> - W is derived from V, V is derived from A
>>>
>>> In the existing implementation, each device "knows" its parent
>>> (via the 'source_device' field).
>>>
>>> When you call av_hwdevice_ctx_create_derived() and specify "C"
>>> as the source device, then it will iterate the tree upwards,
>>> so when B is of the requested type, it will return B or if
>>> A is of the requested type, it will return A.
>>> Otherwise, it will create a new device of the requested type
>>> and sets C as its parent.
>>>
>>> But it doesn't return X, Y, V or W (when any would match the
>>> requested type).
>>>
>>> This is the current behavior.
>>>
>>>
>>> What this patchset does is that we also store the derived
>>> children for each device (derived_devices array).
>>>
>>> In the example above, it means hat A has references to
>>> X, B and V. X to Y, B to C and V to W.
>>>
>>> The behavior of the new function is as follows:
>>>
>>> When you call av_hwdevice_ctx_get_or_create_derived() and specify
>> "C"
>>> as the source device, then it will iterate the tree upwards,
>>> so when B is of the requested type, it will return B or if
>>> A is of the requested type, it will return A (like before).
>>>
>>> Additionally, it will also iterate all through other children
>>> of B and other children of A. Which means that if X, Y, V or W
>>> matches the requested type, it would return it.
>>
>> No it doesn't?  Your new function find_derived_hwdevice_ctx() is
>> called only for the original source device, and recurses into its
>> (transitive) children.  It can't return any of X, Y, V or W when
>> starting from C.
> 
> OK, that's my mistake. It's been a while...
> What I described is the original behavior. There were reasons
> to limit it this way.

Well, which one is is actually intended then?

- Mark