[FFmpeg-devel] [PATCH 1/3] avutils/hwcontext: add derive-device function which searches for existing devices in both directions
Soft Works
softworkz at hotmail.com
Mon May 2 11:14:28 EEST 2022
> -----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.
> >> * 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.
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.
That's why I said: yes, I understand the case you are talking
about. But it would require two separate patches, one for
enabling extension options and another one for matching
extension options when deriving a device. This would make
sense, but:
It has nothing to do with this patchset.
(it could be done before or afterwards)
I'm open to discuss this (separately), because it opens up
a range of questions how it could be done.
> >>> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> >>> index ab9ad3703e..1aea7dd5c3 100644
> >>> --- a/libavutil/hwcontext.c
> >>> +++ b/libavutil/hwcontext.c
> >>> @@ -123,6 +123,7 @@ static const AVClass hwdevice_ctx_class = {
> >>> static void hwdevice_ctx_free(void *opaque, uint8_t *data)
> >>> {
> >>> AVHWDeviceContext *ctx = (AVHWDeviceContext*)data;
> >>> + int i;
> >>>
> >>> /* uninit might still want access the hw context and the
> user
> >>> * free() callback might destroy it, so uninit has to be
> >> called first */
> >>> @@ -133,6 +134,8 @@ static void hwdevice_ctx_free(void *opaque,
> >> uint8_t *data)
> >>> ctx->free(ctx);
> >>>
> >>> av_buffer_unref(&ctx->internal->source_device);
> >>> + for (i = 0; i < AV_HWDEVICE_TYPE_NB; i++)
> >>> + av_buffer_unref(&ctx->internal->derived_devices[i]);
> >>>
> >>> av_freep(&ctx->hwctx);
> >>> av_freep(&ctx->internal->priv);
> >>> @@ -644,10 +647,31 @@ fail:
> >>> return ret;
> >>> }
> >>>
> >>> -int av_hwdevice_ctx_create_derived_opts(AVBufferRef
> **dst_ref_ptr,
> >>> - enum AVHWDeviceType type,
> >>> - AVBufferRef *src_ref,
> >>> - AVDictionary *options,
> int
> >> flags)
> >>> +static AVBufferRef* find_derived_hwdevice_ctx(AVBufferRef
> *src_ref,
> >> enum AVHWDeviceType type)
> >>> +{
> >>> + AVBufferRef *tmp_ref;
> >>> + AVHWDeviceContext *src_ctx;
> >>> + int i;
> >>> +
> >>> + src_ctx = (AVHWDeviceContext*)src_ref->data;
> >>> + if (src_ctx->type == type)
> >>> + return src_ref;
> >>> +
> >>> + for (i = 0; i < AV_HWDEVICE_TYPE_NB; i++)
> >>> + if (src_ctx->internal->derived_devices[i]) {
> >>> + tmp_ref = find_derived_hwdevice_ctx(src_ctx-
> >internal-
> >>> derived_devices[i], type);
> >>> + if (tmp_ref)
> >>> + return tmp_ref;
> >>> + }
> >>> +
> >>> + return NULL;
> >>> +}
> >>> +
> >>> +static int hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
> >>> + enum AVHWDeviceType type,
> >>> + AVBufferRef *src_ref,
> >>> + AVDictionary *options, int
> >> flags,
> >>> + int get_existing)
> >>> {
> >>> AVBufferRef *dst_ref = NULL, *tmp_ref;
> >>> AVHWDeviceContext *dst_ctx, *tmp_ctx;
> >>> @@ -667,6 +691,18 @@ int
> >> av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> >>> tmp_ref = tmp_ctx->internal->source_device;
> >>> }
> >>>
> >>> + if (get_existing) {
> >>> + tmp_ref = find_derived_hwdevice_ctx(src_ref, type);
> >>> + if (tmp_ref) {
> >>> + dst_ref = av_buffer_ref(tmp_ref);
> >>> + if (!dst_ref) {
> >>> + ret = AVERROR(ENOMEM);
> >>> + goto fail;
> >>> + }
> >>> + goto done;
> >>> + }
> >>> + }
> >>> +
> >>> dst_ref = av_hwdevice_ctx_alloc(type);
> >>> if (!dst_ref) {
> >>> ret = AVERROR(ENOMEM);
> >>> @@ -688,6 +724,13 @@ int
> >> av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> >>> ret = AVERROR(ENOMEM);
> >>> goto fail;
> >>> }
> >>> + if (!tmp_ctx->internal->derived_devices[type]) {
> >>
> >> I wonder whether you only want to do this when the user made the
> new
> >> call, not the old one?
> >>
> >> The semantics would perhaps feel clearer as "get or create the
> shared
> >> derived device" rather than "get the first device derived or create
> a
> >> new one if not".
> >
> > I've been there for a moment, and then I thought that when the API
> > consumer would mix API calls, e.g. first without 'get' and second
> > with 'get', then the second call would not produce the expected
> > result.
> >
> > Let me know what you think, I have no strong opinion about this.
>
> 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.
Otherwise, it will create a new device of the requested type
and sets C as its parent.
This is the behavior of the new function.
All that it changes is that it searches the full tree instead
of the parents only.
Now back to your original question:
>>> + if (!tmp_ctx->internal->derived_devices[type]) {
>>
>> I wonder whether you only want to do this when the user made the new
>> call, not the old one?
>>
>> The semantics would perhaps feel clearer as "get or create the shared
>> derived device" rather than "get the first device derived or create a
>> new one if not".
The line of code you commented on, is about adding a newly created
derived device to the child collection of its parent.
>> I wonder whether you only want to do this when the user made the new
>> call, not the old one?
The difference between old and new is that old searches only parents
and new searches the full tree.
But should calling the old function also control whether a newly created
derived device is added to the child collection of its parent?
It might be confusing behavior when you first call the old function
and later call the new function but you would get the device only
when it's a parent but not when it's a down-tree-child of any parent.
And that's where I said:
> Let me know what you think, I have no strong opinion about this.
Best regards,
softworkz
More information about the ffmpeg-devel
mailing list