[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 1:57 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 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.

As mentioned, I don't think that would be possible easily, and from 
what I have understood, the plan is to have separate threads for decoding,
encoding and filtering but not multiple threads for filtering.


> 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

Well, multi-threading is not an issue right now, and I don't expect it
to be then.

But on a more practical take: what do you want me to do?

Guard that function with a lock? I can do this, no problem. But
none of any of the device control function does any synchronization,
so why would exactly this - and only this function need synchronization?


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

No. I say, matching device parameters when searching for an existing 
device isn't done right now, and it's outside the scope of 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.
> 
> 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.)

OK.

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

This patchset. It is used by us and by Intel for quite a while already.

Thanks,
softworkz