> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of Mark > Thompson > Sent: Tuesday, May 3, 2022 12:12 AM > To: ffmpeg-devel@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-boun...@ffmpeg.org> On Behalf Of > Mark > >> Thompson > >> Sent: Monday, May 2, 2022 12:01 AM > >> To: ffmpeg-devel@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
