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

[Soft Works]

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

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


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


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


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

Best regards
softworkz