[FFmpeg-devel,v4,1/1] avutils/hwcontext: When deriving a hwdevice, search for existing device in both directions

The test /libavutil/tests/hwdevice checks that when deriving a device
from a source device and then deriving back to the type of the source
device, the result is matching the original source device, i.e. the
derivation mechanism doesn't create a new device in this case.

Previously, this test was usually passed, but only due to two different
kind of flaws:

1. The test covers only a single level of derivation (and back)

It derives device Y from device X and then Y back to the type of X and
checks whether the result matches X.

What it doesn't check for, are longer chains of derivation like:

CUDA1 > OpenCL2 > CUDA3 and then back to OpenCL4

In that case, the second derivation returns the first device (CUDA3 ==
CUDA1), but when deriving OpenCL4, hwcontext.c was creating a new
OpenCL4 context instead of returning OpenCL2, because there was no link
from CUDA1 to OpenCL2 (only backwards from OpenCL2 to CUDA1)

If the test would check for two levels of derivation, it would have
failed.

This patch fixes those (yet untested) cases by introducing forward
references (derived_device) in addition to the existing back references
(source_device).

2. hwcontext_qsv didn't properly set the source_device

In case of QSV, hwcontext_qsv creates a source context internally
(vaapi, dxva2 or d3d11va) without calling av_hwdevice_ctx_create_derived
and without setting source_device.

This way, the hwcontext test ran successful, but what practically
happened, was that - for example - deriving vaapi from qsv didn't return
the original underlying vaapi device and a new one was created instead:
Exactly what the test is intended to detect and prevent. It just
couldn't do so, because the original device was hidden (= not set as the
source_device of the QSV device).

This patch properly makes these setting and fixes all derivation
scenarios.

(at a later stage, /libavutil/tests/hwdevice should be extended to check
longer derivation chains as well)

Signed-off-by: softworkz <softworkz@hotmail.com>
---
v4: Fixed braces code style

 libavutil/hwcontext.c          | 38 ++++++++++++++++++++++++++++++++++
 libavutil/hwcontext.h          |  1 +
 libavutil/hwcontext_internal.h |  6 ++++++
 libavutil/hwcontext_qsv.c      | 13 +++++++++---
 4 files changed, 55 insertions(+), 3 deletions(-)

Quoting Soft Works (2021-11-25 03:41:32)
> @@ -687,6 +720,11 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
>                      ret = AVERROR(ENOMEM);
>                      goto fail;
>                  }
> +                tmp_ctx->internal->derived_devices[type] = av_buffer_ref(dst_ref);
> +                if (!tmp_ctx->internal->derived_devices[type]) {
> +                    ret = AVERROR(ENOMEM);
> +                    goto fail;
> +                }

This means that once you derive a device of a certain type, you can
never truly close it without also killing the parent device. That
strikes me as
- potentially troublesome
- a behavior change

Also, I don't see it as completely obvious that all derivations should
always return the same child instance.

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Thursday, November 25, 2021 5:40 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> Quoting Soft Works (2021-11-25 03:41:32)
> > @@ -687,6 +720,11 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef
> **dst_ref_ptr,
> >                      ret = AVERROR(ENOMEM);
> >                      goto fail;
> >                  }
> > +                tmp_ctx->internal->derived_devices[type] =
> av_buffer_ref(dst_ref);
> > +                if (!tmp_ctx->internal->derived_devices[type]) {
> > +                    ret = AVERROR(ENOMEM);
> > +                    goto fail;
> > +                }
> 
> This means that once you derive a device of a certain type, you can
> never truly close it without also killing the parent device. That
> strikes me as
> - potentially troublesome
> - a behavior change
> 
> Also, I don't see it as completely obvious that all derivations should
> always return the same child instance.


It creates the behavior that everybody wants and expects who is working 
with HW devices derivation.

softworkz

Quoting Soft Works (2021-11-25 18:02:54)
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > Khirnov
> > Sent: Thursday, November 25, 2021 5:40 PM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> > hwdevice, search for existing device in both directions
> > 
> > Quoting Soft Works (2021-11-25 03:41:32)
> > > @@ -687,6 +720,11 @@ int av_hwdevice_ctx_create_derived_opts(AVBufferRef
> > **dst_ref_ptr,
> > >                      ret = AVERROR(ENOMEM);
> > >                      goto fail;
> > >                  }
> > > +                tmp_ctx->internal->derived_devices[type] =
> > av_buffer_ref(dst_ref);
> > > +                if (!tmp_ctx->internal->derived_devices[type]) {
> > > +                    ret = AVERROR(ENOMEM);
> > > +                    goto fail;
> > > +                }
> > 
> > This means that once you derive a device of a certain type, you can
> > never truly close it without also killing the parent device. That
> > strikes me as
> > - potentially troublesome
> > - a behavior change
> > 
> > Also, I don't see it as completely obvious that all derivations should
> > always return the same child instance.
> 
> 
> It creates the behavior that everybody wants and expects who is working 
> with HW devices derivation.

What qualifies you to speak for "everybody"? I would expect to hear some
practical arguments for such a strong claim.

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Friday, November 26, 2021 5:15 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> Quoting Soft Works (2021-11-25 18:02:54)
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > > Khirnov
> > > Sent: Thursday, November 25, 2021 5:40 PM
> > > To: ffmpeg-devel@ffmpeg.org
> > > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> deriving a
> > > hwdevice, search for existing device in both directions
> > >
> > > Quoting Soft Works (2021-11-25 03:41:32)
> > > > @@ -687,6 +720,11 @@ int
> av_hwdevice_ctx_create_derived_opts(AVBufferRef
> > > **dst_ref_ptr,
> > > >                      ret = AVERROR(ENOMEM);
> > > >                      goto fail;
> > > >                  }
> > > > +                tmp_ctx->internal->derived_devices[type] =
> > > av_buffer_ref(dst_ref);
> > > > +                if (!tmp_ctx->internal->derived_devices[type]) {
> > > > +                    ret = AVERROR(ENOMEM);
> > > > +                    goto fail;
> > > > +                }
> > >
> > > This means that once you derive a device of a certain type, you can
> > > never truly close it without also killing the parent device. That
> > > strikes me as
> > > - potentially troublesome
> > > - a behavior change
> > >
> > > Also, I don't see it as completely obvious that all derivations should
> > > always return the same child instance.
> >
> >
> > It creates the behavior that everybody wants and expects who is working
> > with HW devices derivation.
> 
> What qualifies you to speak for "everybody"? I would expect to hear some
> practical arguments for such a strong claim.

I meant "everybody who has been dealing with multi-level hw context 
derivation and talked about it here".

All of them had given specific examples of problems they were running into
or considering problematic:

- Haihao Xiang
- Wenbin Chen
- Guangxin Xu
- Lynne
- Mark Thompson
  (https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg117373.html)

Do you have any example that would be negatively affected by this patch?

Thanks,
softworkz

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Friday, November 26, 2021 5:15 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> Quoting Soft Works (2021-11-25 18:02:54)
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > > Khirnov
> > > Sent: Thursday, November 25, 2021 5:40 PM
> > > To: ffmpeg-devel@ffmpeg.org
> > > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> deriving a
> > > hwdevice, search for existing device in both directions
> > >
> > > Quoting Soft Works (2021-11-25 03:41:32)
> > > > @@ -687,6 +720,11 @@ int
> av_hwdevice_ctx_create_derived_opts(AVBufferRef
> > > **dst_ref_ptr,
> > > >                      ret = AVERROR(ENOMEM);
> > > >                      goto fail;
> > > >                  }
> > > > +                tmp_ctx->internal->derived_devices[type] =
> > > av_buffer_ref(dst_ref);
> > > > +                if (!tmp_ctx->internal->derived_devices[type]) {
> > > > +                    ret = AVERROR(ENOMEM);
> > > > +                    goto fail;
> > > > +                }
> > >
> > > This means that once you derive a device of a certain type, you can
> > > never truly close it without also killing the parent device. That

Maybe I'm missing something, but hw device contexts are refcounted.
What happens in hwdevice_ctx_free() is this:

av_buffer_unref(&ctx->internal->source_device);

Quoting Soft Works (2021-11-26 19:43:58)
> Maybe I'm missing something, but hw device contexts are refcounted.
> What happens in hwdevice_ctx_free() is this:
> 
> av_buffer_unref(&ctx->internal->source_device);

IIUC this only happens after the parent device is freed. My concern is
the following situation:
- the caller creates a parent hwdevice
- the caller derives a child from it, which may acquire some additional
  resources beyond what the parent holds
- the caller unrefs all his references to the child, but the child does
  not get freed because the parent still holds a reference to it

Since av_hwdevice_ctx_create_derived() has a flags parameter, we might
want to introduce a flag to control this behavior.

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Friday, November 26, 2021 8:12 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> Quoting Soft Works (2021-11-26 19:43:58)
> > Maybe I'm missing something, but hw device contexts are refcounted.
> > What happens in hwdevice_ctx_free() is this:
> >
> > av_buffer_unref(&ctx->internal->source_device);
> 
> IIUC this only happens after the parent device is freed. My concern is
> the following situation:
> - the caller creates a parent hwdevice
> - the caller derives a child from it, which may acquire some additional
>   resources beyond what the parent holds
> - the caller unrefs all his references to the child, but the child does
>   not get freed because the parent still holds a reference to it
> 
> Since av_hwdevice_ctx_create_derived() has a flags parameter, we might
> want to introduce a flag to control this behavior.

I understand what you mean. I'm just not sure whether a practical case
with such a requirement exists. Should that turn out to be required,
such flag can be added at any time, IMO.

Kind regards,
softworkz

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Soft Works
> Sent: Friday, November 26, 2021 8:29 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > Khirnov
> > Sent: Friday, November 26, 2021 8:12 PM
> > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving
> a
> > hwdevice, search for existing device in both directions
> >
> > Quoting Soft Works (2021-11-26 19:43:58)
> > > Maybe I'm missing something, but hw device contexts are refcounted.
> > > What happens in hwdevice_ctx_free() is this:
> > >
> > > av_buffer_unref(&ctx->internal->source_device);
> >
> > IIUC this only happens after the parent device is freed. My concern is
> > the following situation:
> > - the caller creates a parent hwdevice
> > - the caller derives a child from it, which may acquire some additional
> >   resources beyond what the parent holds
> > - the caller unrefs all his references to the child, but the child does
> >   not get freed because the parent still holds a reference to it

BTW, that's the current behavior anyway. The only change that this patch 
is adding is to find _all_ existing derived contexts, by iterating through
the whole tree of derivations instead of just following the parent direction.

softworkz

On Fri, 2021-11-26 at 19:29 +0000, Soft Works wrote:
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > Khirnov
> > Sent: Friday, November 26, 2021 8:12 PM
> > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving
> > a
> > hwdevice, search for existing device in both directions
> > 
> > Quoting Soft Works (2021-11-26 19:43:58)
> > > Maybe I'm missing something, but hw device contexts are refcounted.
> > > What happens in hwdevice_ctx_free() is this:
> > > 
> > > av_buffer_unref(&ctx->internal->source_device);
> > 
> > IIUC this only happens after the parent device is freed. My concern is
> > the following situation:
> > - the caller creates a parent hwdevice
> > - the caller derives a child from it, which may acquire some additional
> >   resources beyond what the parent holds
> > - the caller unrefs all his references to the child, but the child does
> >   not get freed because the parent still holds a reference to it
> > 
> > Since av_hwdevice_ctx_create_derived() has a flags parameter, we might
> > want to introduce a flag to control this behavior.
> 
> I understand what you mean. I'm just not sure whether a practical case
> with such a requirement exists. Should that turn out to be required,
> such flag can be added at any time, IMO.


I agree we may add such flag later if required. May we merge this patch to fix
the annoying derivation limitation if no other concern ? 

Thanks
Haihao

On Thu, 2021-12-23 at 14:01 +0000, Xiang, Haihao wrote:
> On Fri, 2021-11-26 at 19:29 +0000, Soft Works wrote:
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > > Khirnov
> > > Sent: Friday, November 26, 2021 8:12 PM
> > > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> > > deriving
> > > a
> > > hwdevice, search for existing device in both directions
> > > 
> > > Quoting Soft Works (2021-11-26 19:43:58)
> > > > Maybe I'm missing something, but hw device contexts are refcounted.
> > > > What happens in hwdevice_ctx_free() is this:
> > > > 
> > > > av_buffer_unref(&ctx->internal->source_device);
> > > 
> > > IIUC this only happens after the parent device is freed. My concern is
> > > the following situation:
> > > - the caller creates a parent hwdevice
> > > - the caller derives a child from it, which may acquire some additional
> > >   resources beyond what the parent holds
> > > - the caller unrefs all his references to the child, but the child does
> > >   not get freed because the parent still holds a reference to it
> > > 
> > > Since av_hwdevice_ctx_create_derived() has a flags parameter, we might
> > > want to introduce a flag to control this behavior.
> > 
> > I understand what you mean. I'm just not sure whether a practical case
> > with such a requirement exists. Should that turn out to be required,
> > such flag can be added at any time, IMO.
> 
> 
> I agree we may add such flag later if required. May we merge this patch to fix
> the annoying derivation limitation if no other concern ? 

Any other comment for this patch version? I'll add the note pointed out by Lynne
and push this patch if no objection. 

Thanks
Haihao

On 25/11/2021 02:41, Soft Works wrote:
> The test /libavutil/tests/hwdevice checks that when deriving a device
> from a source device and then deriving back to the type of the source
> device, the result is matching the original source device, i.e. the
> derivation mechanism doesn't create a new device in this case.
> 
> Previously, this test was usually passed, but only due to two different
> kind of flaws:
> 
> 1. The test covers only a single level of derivation (and back)
> 
> It derives device Y from device X and then Y back to the type of X and
> checks whether the result matches X.
> 
> What it doesn't check for, are longer chains of derivation like:
> 
> CUDA1 > OpenCL2 > CUDA3 and then back to OpenCL4
> 
> In that case, the second derivation returns the first device (CUDA3 ==
> CUDA1), but when deriving OpenCL4, hwcontext.c was creating a new
> OpenCL4 context instead of returning OpenCL2, because there was no link
> from CUDA1 to OpenCL2 (only backwards from OpenCL2 to CUDA1)

Yes, this is exactly what I expect.

Because of how these APIs work, device derivation is always one-way - you can make an OpenCL device from a D3D11 one, but not the other direction.  I don't think there is any case which allows both directions (nor should there be, in general), so the possibilities form an acyclic graph.

I do expect the user to know here what the possible-derivation graph looks like for the cases they want to use (since only a few cases are meaningful), though maybe it would be worth explicitly documenting it somewhere.

There are then two non-overlapping operations combined in the device derivation function (when A -> B is the meaningful direction):
* Given a context of type A, create a context of type B using the same physical device.
* Given a context of type B, retrieve the original context of type A it was created from (if it exists).

(Maybe they should have been separate functions to avoid this confusion, but it did fit rather nicely in how hwmap works.

Saying that derivation from A should always return the same B is not intended, nor do I think it should be.

The problem with doing that is that components can no longer tell whether the device they have is shared with someone else, whether they are in ffmpeg, in unrelated libraries, or in user code.

What that actually means depends on the API, but in general it would ban doing anything which might affect another user of the same device, such as setting any global options or calling anything which might not be thread-safe.

(Note that the global options argument in av_hwdevice_create_derived_opts() already violates this constraint, making the patch wrong as-is even if you do think it is reasonable to impose this constraint on all users.)

> If the test would check for two levels of derivation, it would have
> failed.
> 
> This patch fixes those (yet untested) cases by introducing forward
> references (derived_device) in addition to the existing back references
> (source_device).
> 
> 2. hwcontext_qsv didn't properly set the source_device
> 
> In case of QSV, hwcontext_qsv creates a source context internally
> (vaapi, dxva2 or d3d11va) without calling av_hwdevice_ctx_create_derived
> and without setting source_device.

What would go wrong if it did?

> This way, the hwcontext test ran successful, but what practically
> happened, was that - for example - deriving vaapi from qsv didn't return
> the original underlying vaapi device and a new one was created instead:

Er... how would it create a new device?  The only type VAAPI can derive from is DRM.

> Exactly what the test is intended to detect and prevent. It just
> couldn't do so, because the original device was hidden (= not set as the
> source_device of the QSV device).
> 
> This patch properly makes these setting and fixes all derivation
> scenarios.
> 
> (at a later stage, /libavutil/tests/hwdevice should be extended to check
> longer derivation chains as well)
> 
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
> v4: Fixed braces code style
> 
>   libavutil/hwcontext.c          | 38 ++++++++++++++++++++++++++++++++++
>   libavutil/hwcontext.h          |  1 +
>   libavutil/hwcontext_internal.h |  6 ++++++
>   libavutil/hwcontext_qsv.c      | 13 +++++++++---
>   4 files changed, 55 insertions(+), 3 deletions(-)

- Mark

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Thursday, December 30, 2021 12:04 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> On 25/11/2021 02:41, Soft Works wrote:
> > The test /libavutil/tests/hwdevice checks that when deriving a device
> > from a source device and then deriving back to the type of the source
> > device, the result is matching the original source device, i.e. the
> > derivation mechanism doesn't create a new device in this case.
> >
> > Previously, this test was usually passed, but only due to two different
> > kind of flaws:
> >
> > 1. The test covers only a single level of derivation (and back)
> >
> > It derives device Y from device X and then Y back to the type of X and
> > checks whether the result matches X.
> >
> > What it doesn't check for, are longer chains of derivation like:
> >
> > CUDA1 > OpenCL2 > CUDA3 and then back to OpenCL4
> >
> > In that case, the second derivation returns the first device (CUDA3 ==
> > CUDA1), but when deriving OpenCL4, hwcontext.c was creating a new
> > OpenCL4 context instead of returning OpenCL2, because there was no link
> > from CUDA1 to OpenCL2 (only backwards from OpenCL2 to CUDA1)
> 
> Yes, this is exactly what I expect.
> 
> Because of how these APIs work, device derivation is always one-way - you can
> make an OpenCL device from a D3D11 one, but not the other direction.  I don't
> think there is any case which allows both directions

hwmap=reverse=1


> Saying that derivation from A should always return the same B is not
> intended, nor do I think it should be.

Why not?

Looking at the reality of API users:

- I'm covering a wide range of different processing pipelines and 
  found that this behavior is crucial to make important and relevant
  processing pipelines work

- Intel have three different workaround-patches in their backlog/queue
  of ffmpeg patches to get certain processing setups working

- The developers working on Vulkan have confirmed that this change
  is necessary and crucial for certain setups to work

- Nobody has named any case or scenario that would be negatively
  affected by this change

Given that situation, I don't think it's useful to talk about 
theoretical implications.

Kind regards,
softworkz

On 30/12/2021 00:29, Soft Works wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>> Thompson
>> Sent: Thursday, December 30, 2021 12:04 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
>> hwdevice, search for existing device in both directions
>>
>> On 25/11/2021 02:41, Soft Works wrote:
>>> The test /libavutil/tests/hwdevice checks that when deriving a device
>>> from a source device and then deriving back to the type of the source
>>> device, the result is matching the original source device, i.e. the
>>> derivation mechanism doesn't create a new device in this case.
>>>
>>> Previously, this test was usually passed, but only due to two different
>>> kind of flaws:
>>>
>>> 1. The test covers only a single level of derivation (and back)
>>>
>>> It derives device Y from device X and then Y back to the type of X and
>>> checks whether the result matches X.
>>>
>>> What it doesn't check for, are longer chains of derivation like:
>>>
>>> CUDA1 > OpenCL2 > CUDA3 and then back to OpenCL4
>>>
>>> In that case, the second derivation returns the first device (CUDA3 ==
>>> CUDA1), but when deriving OpenCL4, hwcontext.c was creating a new
>>> OpenCL4 context instead of returning OpenCL2, because there was no link
>>> from CUDA1 to OpenCL2 (only backwards from OpenCL2 to CUDA1)
>>
>> Yes, this is exactly what I expect.
>>
>> Because of how these APIs work, device derivation is always one-way - you can
>> make an OpenCL device from a D3D11 one, but not the other direction.  I don't
>> think there is any case which allows both directions
> 
> hwmap=reverse=1

Indeed, hwmap reverse exists because mapping is one-way and sometimes a filter graph wants to use it in the other direction.

>> Saying that derivation from A should always return the same B is not
>> intended, nor do I think it should be.
> 
> Why not?
> 
> Looking at the reality of API users:
> 
> - I'm covering a wide range of different processing pipelines and
>    found that this behavior is crucial to make important and relevant
>    processing pipelines work
> 
> - Intel have three different workaround-patches in their backlog/queue
>    of ffmpeg patches to get certain processing setups working
> 
> - The developers working on Vulkan have confirmed that this change
>    is necessary and crucial for certain setups to work
> 
> - Nobody has named any case or scenario that would be negatively
>    affected by this change
> 
> Given that situation, I don't think it's useful to talk about
> theoretical implications.

You are not talking about API users at all.  When does an API user ever want this patch?  From their point of view it is surprising and unwanted - if they want the same device again, they just use the same device again.

You are talking about users of the ffmpeg utility.  The change is a library hack to work around the inability to select devices per-filter in the ffmpeg utility.

Please, just implement device selection for filters in ffmpeg rather than adding unexpected behaviour elsewhere.  libavfilter has supported it for API users for a long time, no library changes should be needed.

- Mark

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Thursday, December 30, 2021 12:22 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> On 30/12/2021 00:29, Soft Works wrote:
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> >> Thompson
> >> Sent: Thursday, December 30, 2021 12:04 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> deriving a
> >> hwdevice, search for existing device in both directions
> >>
> >> On 25/11/2021 02:41, Soft Works wrote:
> >>> The test /libavutil/tests/hwdevice checks that when deriving a device
> >>> from a source device and then deriving back to the type of the source
> >>> device, the result is matching the original source device, i.e. the
> >>> derivation mechanism doesn't create a new device in this case.
> >>>
> >>> Previously, this test was usually passed, but only due to two different
> >>> kind of flaws:
> >>>
> >>> 1. The test covers only a single level of derivation (and back)
> >>>
> >>> It derives device Y from device X and then Y back to the type of X and
> >>> checks whether the result matches X.
> >>>
> >>> What it doesn't check for, are longer chains of derivation like:
> >>>
> >>> CUDA1 > OpenCL2 > CUDA3 and then back to OpenCL4
> >>>
> >>> In that case, the second derivation returns the first device (CUDA3 ==
> >>> CUDA1), but when deriving OpenCL4, hwcontext.c was creating a new
> >>> OpenCL4 context instead of returning OpenCL2, because there was no link
> >>> from CUDA1 to OpenCL2 (only backwards from OpenCL2 to CUDA1)
> >>
> >> Yes, this is exactly what I expect.
> >>
> >> Because of how these APIs work, device derivation is always one-way - you
> can
> >> make an OpenCL device from a D3D11 one, but not the other direction.  I
> don't
> >> think there is any case which allows both directions
> >
> > hwmap=reverse=1
> 
> Indeed, hwmap reverse exists because mapping is one-way and sometimes a
> filter graph wants to use it in the other direction.


=> mapping is not only one-way


> >> Saying that derivation from A should always return the same B is not
> >> intended, nor do I think it should be.
> >
> > Why not?
> >
> > Looking at the reality of API users:
> >
> > - I'm covering a wide range of different processing pipelines and
> >    found that this behavior is crucial to make important and relevant
> >    processing pipelines work
> >
> > - Intel have three different workaround-patches in their backlog/queue
> >    of ffmpeg patches to get certain processing setups working
> >
> > - The developers working on Vulkan have confirmed that this change
> >    is necessary and crucial for certain setups to work
> >
> > - Nobody has named any case or scenario that would be negatively
> >    affected by this change
> >
> > Given that situation, I don't think it's useful to talk about
> > theoretical implications.
> 
> You are not talking about API users at all.  When does an API user ever want
> this patch?  From their point of view it is surprising and unwanted - if they
> want the same device again, they just use the same device again.

No. Not when using libavfilter with a filter graph.

=> It affects API users in the same way

> 
> You are talking about users of the ffmpeg utility.  

No. See above.

> The change is a library
> hack to work around the inability to select devices per-filter in the ffmpeg
> utility.

No, it's not a hack, it's about making libavfilter hw context deriving and reverse
mapping working as expected and needed.

> Please, just implement device selection for filters in ffmpeg rather than
> adding unexpected behaviour elsewhere.  libavfilter has supported it for API
> users for a long time, no library changes should be needed.

I'm afraid but I disagree. I stick to the proposed solution; this change
is welcomed and needed by many which are having problems with the current 
implementation.
I still haven't seen any (real-world and not theoretical constructed) scenario
where this would have a negative effect or break something, neither did I see
anybody else objecting.

Anyway, I have no intentions to propose a different solution. This is what
I'm using and what others want and need. 

Thanks,
softworkz

On 12/27/2021 12:08 AM, Xiang, Haihao wrote:
> On Thu, 2021-12-23 at 14:01 +0000, Xiang, Haihao wrote:
>> On Fri, 2021-11-26 at 19:29 +0000, Soft Works wrote:
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
>>>> Khirnov
>>>> Sent: Friday, November 26, 2021 8:12 PM
>>>> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
>>>> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
>>>> deriving
>>>> a
>>>> hwdevice, search for existing device in both directions
>>>>
>>>> Quoting Soft Works (2021-11-26 19:43:58)
>>>>> Maybe I'm missing something, but hw device contexts are refcounted.
>>>>> What happens in hwdevice_ctx_free() is this:
>>>>>
>>>>> av_buffer_unref(&ctx->internal->source_device);
>>>>
>>>> IIUC this only happens after the parent device is freed. My concern is
>>>> the following situation:
>>>> - the caller creates a parent hwdevice
>>>> - the caller derives a child from it, which may acquire some additional
>>>>    resources beyond what the parent holds
>>>> - the caller unrefs all his references to the child, but the child does
>>>>    not get freed because the parent still holds a reference to it
>>>>
>>>> Since av_hwdevice_ctx_create_derived() has a flags parameter, we might
>>>> want to introduce a flag to control this behavior.
>>>
>>> I understand what you mean. I'm just not sure whether a practical case
>>> with such a requirement exists. Should that turn out to be required,
>>> such flag can be added at any time, IMO.
>>
>>
>> I agree we may add such flag later if required. May we merge this patch to fix
>> the annoying derivation limitation if no other concern ?
> 
> Any other comment for this patch version? I'll add the note pointed out by Lynne
> and push this patch if no objection.
> 
> Thanks
> Haihao

Why was this pushed? There were objections.

On Wed, 2022-01-05 at 00:19 -0300, James Almer wrote:
> 
> On 12/27/2021 12:08 AM, Xiang, Haihao wrote:
> > On Thu, 2021-12-23 at 14:01 +0000, Xiang, Haihao wrote:
> > > On Fri, 2021-11-26 at 19:29 +0000, Soft Works wrote:
> > > > > -----Original Message-----
> > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > > > Anton
> > > > > Khirnov
> > > > > Sent: Friday, November 26, 2021 8:12 PM
> > > > > To: FFmpeg development discussions and patches <
> > > > > ffmpeg-devel@ffmpeg.org>
> > > > > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> > > > > deriving
> > > > > a
> > > > > hwdevice, search for existing device in both directions
> > > > > 
> > > > > Quoting Soft Works (2021-11-26 19:43:58)
> > > > > > Maybe I'm missing something, but hw device contexts are refcounted.
> > > > > > What happens in hwdevice_ctx_free() is this:
> > > > > > 
> > > > > > av_buffer_unref(&ctx->internal->source_device);
> > > > > 
> > > > > IIUC this only happens after the parent device is freed. My concern is
> > > > > the following situation:
> > > > > - the caller creates a parent hwdevice
> > > > > - the caller derives a child from it, which may acquire some
> > > > > additional
> > > > >    resources beyond what the parent holds
> > > > > - the caller unrefs all his references to the child, but the child
> > > > > does
> > > > >    not get freed because the parent still holds a reference to it
> > > > > 
> > > > > Since av_hwdevice_ctx_create_derived() has a flags parameter, we might
> > > > > want to introduce a flag to control this behavior.
> > > > 
> > > > I understand what you mean. I'm just not sure whether a practical case
> > > > with such a requirement exists. Should that turn out to be required,
> > > > such flag can be added at any time, IMO.
> > > 
> > > 
> > > I agree we may add such flag later if required. May we merge this patch to
> > > fix
> > > the annoying derivation limitation if no other concern ?
> > 
> > Any other comment for this patch version? I'll add the note pointed out by
> > Lynne
> > and push this patch if no objection.
> > 
> > Thanks
> > Haihao
> 
> Why was this pushed? There were objections.

I apologize that I missed the new discussion on ML, thought no objection since
my email, so I pushed Softworks' patch as this patch really fixed some issues for me and others. I'll revert it and check the ML more carefully. 

Thanks
Haihao


> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

On 05/01/2022 03:38, Xiang, Haihao wrote:
> ... this patch really fixed some issues for me and others.

Can you explain this in more detail?

I'd like to understand whether the issues you refer to are something which would be fixed by the ffmpeg utility allowing selection of devices for libavfilter, or whether they are something unrelated.

(For library users the currently-supported way of getting the same device again is to keep a reference to the device and reuse it.  If there is some case where you can't do that then it would be useful to hear about it.)

Thanks,

- Mark

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Sunday, January 9, 2022 7:39 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> On 05/01/2022 03:38, Xiang, Haihao wrote:
> > ... this patch really fixed some issues for me and others.
> 
> Can you explain this in more detail?
> 
> I'd like to understand whether the issues you refer to are something which
> would be fixed by the ffmpeg utility allowing selection of devices for
> libavfilter, or whether they are something unrelated.
> 
> (For library users the currently-supported way of getting the same device
> again is to keep a reference to the device and reuse it.  If there is some
> case where you can't do that then it would be useful to hear about it.)

Hi Mark,

they have 3 workaround patches on their staging repo, but I'll let Haihao 
answer in detail.

I have another question. I've been searching high and low, yet I can't
find the message. Do you remember that patch discussion from (quite a 
few) months ago, where it was about another QSV change (something about
device creation from the command line, IIRC). There was a command line
example with QSV and you correctly remarked something like:
"Do you even know that just for this command line, there are 5 device
creations happening in the background, implicit and explicit, and in
one case (or 2), it's not even creating the specified device but
a session for the default device instead"
(just roughly from memory)

Do you remember - or was it Philip?

Anyway, this is something that the patch will improve. There has been one
other commit since that time regarding explicit device creation from 
Haihao (iirc), which already reduced the device creation and fixed the
incorrect default session creation. 

My patch tackles this from another side: at that time, you (or Philip) 
explained that the secondary context that QSV requires (VAAPI, D3Dx)
and that is initially created when setting up the QSV device, does not
even get used when subsequently deriving to a context of that kind. 
Instead, a new device is being created in this case.

That's another scenario which is fixed by this patch.


It's a hybrid device context, that's the reason why QSV is more affected 
than all other hwaccels as it consists of a QSV session already DERIVED 
from a VAAPI or D3Dx device. 

Example (let's assume Windows with D3D9): You go into decoding with a 
QSV decoder in a QSV context and then you want to use an OpenCL filter. 
This requires an OpenCL context, and of course you want to share the 
frames memory. For memory sharing, OpenCL requires the underlying context 
of the QSV session - in this example D3D9.

Before this patch - like you said - deriving devices was usually (except 
reverse hwmap) forward-only. That means - you are stuck in this situation:
you could (forward-)derive to a D3D9 context, but that doesn't help: for
sharing the memory, you need to provide the original hw device to OpenCL,
you can't supply just another newly derived device of the same type.
And there is (was) no way to get the original hw context.


Anyway I'm wondering whether it can even be logically valid to derive
from one device to another and then to another instance of the previous
device type.
From my understanding, "deriving" or "hw mapping" from one device to 
another means to establish a different way or accessor to a common
resource/data, which means that you can access the data in one or the 
other way.

Now let's assume a forward device-derivation chain like this:

D3D_1  >>  OpenCL_1  >>  D3D_2

We have D3D surfaces, then we share them with OpenCL. Both *_1
contexts provide access to the same data.
Then we derive again "forward only" and we get a new D3D_2
context. It is derived from OpenCL_1, so it must provide
access to the same data as OpenCL_1 AND D3D_1.

Now we have two different D3D contexts which are supposed to
provide access to the same data!


1. This doesn't even work technically
   - neither from D3D (iirc)
   - nor from ffmpeg (not cleanly)

2. This doesn't make sense at all. There should always be
   only a single device context of a device type for the same 
   resource

3. Why would somebody even want this - what kind of use case?


Regards,
softworkz

On 09/01/2022 21:15, Soft Works wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>> Thompson
>> Sent: Sunday, January 9, 2022 7:39 PM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
>> hwdevice, search for existing device in both directions
>>
>> On 05/01/2022 03:38, Xiang, Haihao wrote:
>>> ... this patch really fixed some issues for me and others.
>>
>> Can you explain this in more detail?
>>
>> I'd like to understand whether the issues you refer to are something which
>> would be fixed by the ffmpeg utility allowing selection of devices for
>> libavfilter, or whether they are something unrelated.
>>
>> (For library users the currently-supported way of getting the same device
>> again is to keep a reference to the device and reuse it.  If there is some
>> case where you can't do that then it would be useful to hear about it.)
> 
> Hi Mark,
> 
> they have 3 workaround patches on their staging repo, but I'll let Haihao
> answer in detail.
> 
> I have another question. I've been searching high and low, yet I can't
> find the message. Do you remember that patch discussion from (quite a
> few) months ago, where it was about another QSV change (something about
> device creation from the command line, IIRC). There was a command line
> example with QSV and you correctly remarked something like:
> "Do you even know that just for this command line, there are 5 device
> creations happening in the background, implicit and explicit, and in
> one case (or 2), it's not even creating the specified device but
> a session for the default device instead"
> (just roughly from memory)
> 
> Do you remember - or was it Philip?

<https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2021-March/277731.html>

> Anyway, this is something that the patch will improve. There has been one
> other commit since that time regarding explicit device creation from
> Haihao (iirc), which already reduced the device creation and fixed the
> incorrect default session creation.

Yes, the special ffmpeg utility code to work around the lack of AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX in the libmfx decoders caused confusion by working differently to everything else - implementing that and getting rid of the workarounds was definitely a good thing.

> My patch tackles this from another side: at that time, you (or Philip)
> explained that the secondary context that QSV requires (VAAPI, D3Dx)
> and that is initially created when setting up the QSV device, does not
> even get used when subsequently deriving to a context of that kind.
> Instead, a new device is being created in this case.
> 
> That's another scenario which is fixed by this patch.

It does sound like you just always want a libmfx device to be derived from the thing which is really there sitting underneath it.

Then it would be clear that derivation in the other direction would always have the retrieve-original-device meaning, rather than a fiction that you are deriving a new D3D device from a libmfx one which doesn't match what is actually happening.

> It's a hybrid device context, that's the reason why QSV is more affected
> than all other hwaccels as it consists of a QSV session already DERIVED
> from a VAAPI or D3Dx device.
> 
> Example (let's assume Windows with D3D9): You go into decoding with a
> QSV decoder in a QSV context and then you want to use an OpenCL filter.
> This requires an OpenCL context, and of course you want to share the
> frames memory. For memory sharing, OpenCL requires the underlying context
> of the QSV session - in this example D3D9.
> 
> Before this patch - like you said - deriving devices was usually (except
> reverse hwmap) forward-only. That means - you are stuck in this situation:
> you could (forward-)derive to a D3D9 context, but that doesn't help: for
> sharing the memory, you need to provide the original hw device to OpenCL,
> you can't supply just another newly derived device of the same type.
> And there is (was) no way to get the original hw context.

If you are a library user then you get the original hw context by reusing the reference to it that you made when you created it.  This includes libavfilter users, who can provide a hw device to each hwmap separately.

If you are an ffmpeg utility user then I agree there isn't currently a way to do this for filter graphs, hence the solution of providing an a way in the ffmpeg utility to set hw devices per-filter.

> Anyway I'm wondering whether it can even be logically valid to derive
> from one device to another and then to another instance of the previous
> device type.
>  From my understanding, "deriving" or "hw mapping" from one device to
> another means to establish a different way or accessor to a common
> resource/data, which means that you can access the data in one or the
> other way.
> 
> Now let's assume a forward device-derivation chain like this:
> 
> D3D_1  >>  OpenCL_1  >>  D3D_2

You can't do this because device derivation is unidirectional (and acyclic) - you can only derive an OpenCL device from D3D (9 or 11), not the other way around.

Similarly, you can only map frames from D3D to OpenCL.  That's why the hwmap reverse option exists, because of cases where you actually want the other direction which doesn't really exist.

(There are some cases like VAAPI <-> DRM which do offer frame mapping in both directions, but that's really because DRM does not itself have any frame management.  Device derivation still only makes sense in one direction, as DRM sits beneath VAAPI.)

> We have D3D surfaces, then we share them with OpenCL. Both *_1
> contexts provide access to the same data.
> Then we derive again "forward only" and we get a new D3D_2
> context. It is derived from OpenCL_1, so it must provide
> access to the same data as OpenCL_1 AND D3D_1.
> 
> Now we have two different D3D contexts which are supposed to
> provide access to the same data!
> 
> 
> 1. This doesn't even work technically
>     - neither from D3D (iirc)
>     - nor from ffmpeg (not cleanly)
> 
> 2. This doesn't make sense at all. There should always be
>     only a single device context of a device type for the same
>     resource
> 
> 3. Why would somebody even want this - what kind of use case?

The multiple derivation case is for when a single device doesn't work.  Typically that involves multiple separate components which don't want to interact with the others, for example:

* When something thread-unsafe might happen, so different threads need separate instances to work with.
* When global options have to be set on a device, so a component which does that needs its own instance to avoid interfering with anyone else.
* When some code (an external library, say) requires ownership of a device instance, so you need a new one to give to it.
* When components are independent and your code is just simpler that way.

Thanks,

- Mark

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Monday, January 10, 2022 12:13 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> On 09/01/2022 21:15, Soft Works wrote:
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> >> Thompson
> >> Sent: Sunday, January 9, 2022 7:39 PM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> deriving a
> >> hwdevice, search for existing device in both directions
> >>
> >> On 05/01/2022 03:38, Xiang, Haihao wrote:
> >>> ... this patch really fixed some issues for me and others.
> >>
> >> Can you explain this in more detail?
> >>
> >> I'd like to understand whether the issues you refer to are something which
> >> would be fixed by the ffmpeg utility allowing selection of devices for
> >> libavfilter, or whether they are something unrelated.
> >>
> >> (For library users the currently-supported way of getting the same device
> >> again is to keep a reference to the device and reuse it.  If there is some
> >> case where you can't do that then it would be useful to hear about it.)
> >
> > Hi Mark,
> >
> > they have 3 workaround patches on their staging repo, but I'll let Haihao
> > answer in detail.
> >
> > I have another question. I've been searching high and low, yet I can't
> > find the message. Do you remember that patch discussion from (quite a
> > few) months ago, where it was about another QSV change (something about
> > device creation from the command line, IIRC). There was a command line
> > example with QSV and you correctly remarked something like:
> > "Do you even know that just for this command line, there are 5 device
> > creations happening in the background, implicit and explicit, and in
> > one case (or 2), it's not even creating the specified device but
> > a session for the default device instead"
> > (just roughly from memory)
> >
> > Do you remember - or was it Philip?
> 
> <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2021-March/277731.html>
> 
> > Anyway, this is something that the patch will improve. There has been one
> > other commit since that time regarding explicit device creation from
> > Haihao (iirc), which already reduced the device creation and fixed the
> > incorrect default session creation.
> 
> Yes, the special ffmpeg utility code to work around the lack of
> AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX in the libmfx decoders caused
> confusion by working differently to everything else - implementing that and
> getting rid of the workarounds was definitely a good thing.
> 
> > My patch tackles this from another side: at that time, you (or Philip)
> > explained that the secondary context that QSV requires (VAAPI, D3Dx)
> > and that is initially created when setting up the QSV device, does not
> > even get used when subsequently deriving to a context of that kind.
> > Instead, a new device is being created in this case.
> >
> > That's another scenario which is fixed by this patch.
> 
> It does sound like you just always want a libmfx device to be derived from
> the thing which is really there sitting underneath it.

"That's another scenario which is fixed by this patch"

Things stop working as expected as soon as you are working with 3 or more
derived hw devices and neither hwmap nor hwmap-revere can get you to the 
context you want.



> If you are a library user then you get the original hw context by reusing the
> reference to it that you made when you created it.  This includes libavfilter
> users, who can provide a hw device to each hwmap separately.
> 
> If you are an ffmpeg utility user then I agree there isn't currently a way to
> do this for filter graphs, hence the solution of providing an a way in the
> ffmpeg utility to set hw devices per-filter.

just setting the context on a filter doesn't make any sense, because you need 
the mapping. It only makes sense for the hwmap and hwupload filters.


> > Anyway I'm wondering whether it can even be logically valid to derive
> > from one device to another and then to another instance of the previous
> > device type.
> >  From my understanding, "deriving" or "hw mapping" from one device to
> > another means to establish a different way or accessor to a common
> > resource/data, which means that you can access the data in one or the
> > other way.
> >
> > Now let's assume a forward device-derivation chain like this:
> >
> > D3D_1  >>  OpenCL_1  >>  D3D_2
> 
> You can't do this because device derivation is unidirectional (and acyclic) -
> you can only derive an OpenCL device from D3D (9 or 11), not the other way
> around.
> 
> Similarly, you can only map frames from D3D to OpenCL.  That's why the hwmap
> reverse option exists, because of cases where you actually want the other
> direction which doesn't really exist.

Yes, all true, but my point is something else: you can't have several context
of the same type in a derivation chain.

And that's exactly what this patch addresses: it makes sure that you'll get 
an existing context instead of ffmpeg trying to derive to a new hw device
which doesn't work anyway.


> > We have D3D surfaces, then we share them with OpenCL. Both *_1
> > contexts provide access to the same data.
> > Then we derive again "forward only" and we get a new D3D_2
> > context. It is derived from OpenCL_1, so it must provide
> > access to the same data as OpenCL_1 AND D3D_1.
> >
> > Now we have two different D3D contexts which are supposed to
> > provide access to the same data!
> >
> >
> > 1. This doesn't even work technically
> >     - neither from D3D (iirc)
> >     - nor from ffmpeg (not cleanly)
> >
> > 2. This doesn't make sense at all. There should always be
> >     only a single device context of a device type for the same
> >     resource
> >
> > 3. Why would somebody even want this - what kind of use case?
> 
> The multiple derivation case is for when a single device doesn't work.
> Typically that involves multiple separate components which don't want to
> interact with the others, for example:
> 
> * When something thread-unsafe might happen, so different threads need
> separate instances to work with.

Derivation means accessing shared resources (computing and memory), and 
you can't solve a concurrency problem by having two devices accessing 
the same resources - this makes it even worse (assuming a device would 
even allow this).

> * When global options have to be set on a device, so a component which does
> that needs its own instance to avoid interfering with anyone else.

This is NOT derivation. This case is not affected. 


I see. There's a misunderstanding. You seem to assume that this patch
is throwing all hw devices into a collection and the returns always 
the same of a certain kind.

But that's not the case. This patch is only about tracking derivation
chains from which multiple can exist and will be treated separately.

None of your examples are affected by the patch.

Kind regards,
softworkz

On 09/01/2022 23:36, Soft Works wrote:>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>> Thompson
>> Sent: Monday, January 10, 2022 12:13 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
>> hwdevice, search for existing device in both directions
>>
>> On 09/01/2022 21:15, Soft Works wrote:
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>>>> Thompson
>>>> Sent: Sunday, January 9, 2022 7:39 PM
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
>> deriving a
>>>> hwdevice, search for existing device in both directions
>>>>
>>>> On 05/01/2022 03:38, Xiang, Haihao wrote:
>>>>> ... this patch really fixed some issues for me and others.
>>>>
>>>> Can you explain this in more detail?
>>>>
>>>> I'd like to understand whether the issues you refer to are something which
>>>> would be fixed by the ffmpeg utility allowing selection of devices for
>>>> libavfilter, or whether they are something unrelated.
>>>>
>>>> (For library users the currently-supported way of getting the same device
>>>> again is to keep a reference to the device and reuse it.  If there is some
>>>> case where you can't do that then it would be useful to hear about it.)
>>>
>>> Hi Mark,
>>>
>>> they have 3 workaround patches on their staging repo, but I'll let Haihao
>>> answer in detail.
>>>
>>> I have another question. I've been searching high and low, yet I can't
>>> find the message. Do you remember that patch discussion from (quite a
>>> few) months ago, where it was about another QSV change (something about
>>> device creation from the command line, IIRC). There was a command line
>>> example with QSV and you correctly remarked something like:
>>> "Do you even know that just for this command line, there are 5 device
>>> creations happening in the background, implicit and explicit, and in
>>> one case (or 2), it's not even creating the specified device but
>>> a session for the default device instead"
>>> (just roughly from memory)
>>>
>>> Do you remember - or was it Philip?
>>
>> <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2021-March/277731.html>
>>
>>> Anyway, this is something that the patch will improve. There has been one
>>> other commit since that time regarding explicit device creation from
>>> Haihao (iirc), which already reduced the device creation and fixed the
>>> incorrect default session creation.
>>
>> Yes, the special ffmpeg utility code to work around the lack of
>> AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX in the libmfx decoders caused
>> confusion by working differently to everything else - implementing that and
>> getting rid of the workarounds was definitely a good thing.
>>
>>> My patch tackles this from another side: at that time, you (or Philip)
>>> explained that the secondary context that QSV requires (VAAPI, D3Dx)
>>> and that is initially created when setting up the QSV device, does not
>>> even get used when subsequently deriving to a context of that kind.
>>> Instead, a new device is being created in this case.
>>>
>>> That's another scenario which is fixed by this patch.
>>
>> It does sound like you just always want a libmfx device to be derived from
>> the thing which is really there sitting underneath it.
> 
> "That's another scenario which is fixed by this patch"
> 
> Things stop working as expected as soon as you are working with 3 or more
> derived hw devices and neither hwmap nor hwmap-revere can get you to the
> context you want.

And your situation is sufficiently complex that specifying devices explicitly is probably what you want, rather that trying to trick some implicit route into returning the answer you already know.

>> If you are a library user then you get the original hw context by reusing the
>> reference to it that you made when you created it.  This includes libavfilter
>> users, who can provide a hw device to each hwmap separately.
>>
>> If you are an ffmpeg utility user then I agree there isn't currently a way to
>> do this for filter graphs, hence the solution of providing an a way in the
>> ffmpeg utility to set hw devices per-filter.
> 
> just setting the context on a filter doesn't make any sense, because you need
> the mapping. It only makes sense for the hwmap and hwupload filters.

Yes?  The filters you need to give it to are the hwmap and hwupload filters, the others indeed don't matter (though they are blindly set at the moment because there is no way to know they don't need it).

>>> Anyway I'm wondering whether it can even be logically valid to derive
>>> from one device to another and then to another instance of the previous
>>> device type.
>>>   From my understanding, "deriving" or "hw mapping" from one device to
>>> another means to establish a different way or accessor to a common
>>> resource/data, which means that you can access the data in one or the
>>> other way.
>>>
>>> Now let's assume a forward device-derivation chain like this:
>>>
>>> D3D_1  >>  OpenCL_1  >>  D3D_2
>>
>> You can't do this because device derivation is unidirectional (and acyclic) -
>> you can only derive an OpenCL device from D3D (9 or 11), not the other way
>> around.
>>
>> Similarly, you can only map frames from D3D to OpenCL.  That's why the hwmap
>> reverse option exists, because of cases where you actually want the other
>> direction which doesn't really exist.
> 
> Yes, all true, but my point is something else: you can't have several context
> of the same type in a derivation chain.

That's a consequence of unidirectionality + acyclity, yes.

> And that's exactly what this patch addresses: it makes sure that you'll get
> an existing context instead of ffmpeg trying to derive to a new hw device
> which doesn't work anyway.

I'm still only seeing one case where this bizarre operation is wanted: the ffmpeg utility user trying to get devices into the right place in their filter graphs, who I still think would be better served by being able to set the right device directly on hwmap rather than implicitly through searching derivation chains.

>>> We have D3D surfaces, then we share them with OpenCL. Both *_1
>>> contexts provide access to the same data.
>>> Then we derive again "forward only" and we get a new D3D_2
>>> context. It is derived from OpenCL_1, so it must provide
>>> access to the same data as OpenCL_1 AND D3D_1.
>>>
>>> Now we have two different D3D contexts which are supposed to
>>> provide access to the same data!
>>>
>>>
>>> 1. This doesn't even work technically
>>>      - neither from D3D (iirc)
>>>      - nor from ffmpeg (not cleanly)
>>>
>>> 2. This doesn't make sense at all. There should always be
>>>      only a single device context of a device type for the same
>>>      resource
>>>
>>> 3. Why would somebody even want this - what kind of use case?
>>
>> The multiple derivation case is for when a single device doesn't work.
>> Typically that involves multiple separate components which don't want to
>> interact with the others, for example:
>>
>> * When something thread-unsafe might happen, so different threads need
>> separate instances to work with.
> 
> Derivation means accessing shared resources (computing and memory), and
> you can't solve a concurrency problem by having two devices accessing
> the same resources - this makes it even worse (assuming a device would
> even allow this).

Device derivation means making a compatible context of a different type on the same physical device.

Now that's probably intended because you are going to want to share some particular resources, but exactly what can be shared and what is possible is dependent on the setup.

Similarly, any rules for concurrency are dependent on the setup - maybe you can't do two specific things simultaneously in the same device context and need two separate ones to solve it, but they still both want to share in some way with the different device context they were derived from.

>> * When global options have to be set on a device, so a component which does
>> that needs its own instance to avoid interfering with anyone else.
> 
> This is NOT derivation. This case is not affected.

Suppose I have some DRM frames which come from somewhere (some hardware decoder, say - doesn't matter).

I want to do Vulkan things with some of the frames, so I call av_hwdevice_ctx_create_derived_opts() to get a compatible Vulkan context.  Then I can map and do Vulkan things, yay!

Later on, I want to do some independent Vulkan thing with my DRM frames.  I do the same operation again with different options (because my new thing wants some new extensions, say).  This returns a new Vulkan context and I can work with it completely independently, yay!

Since everything was independent I definitely didn't expect the second operation to return the same device again - that wouldn't make sense.

> I see. There's a misunderstanding. You seem to assume that this patch
> is throwing all hw devices into a collection and the returns always
> the same of a certain kind.
> 
> But that's not the case. This patch is only about tracking derivation
> chains from which multiple can exist and will be treated separately.

It is effectively equivalent to that in the simple cases I'm talking about here.
> None of your examples are affected by the patch.

I disagree.

- Mark

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Monday, January 10, 2022 1:57 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> On 09/01/2022 23:36, Soft Works wrote:>> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> >> Thompson
> >> Sent: Monday, January 10, 2022 12:13 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> deriving a
> >> hwdevice, search for existing device in both directions
> >>
> >> On 09/01/2022 21:15, Soft Works wrote:
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> >>>> Thompson
> >>>> Sent: Sunday, January 9, 2022 7:39 PM
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> >> deriving a
> >>>> hwdevice, search for existing device in both directions
> >>>>
> >>>> On 05/01/2022 03:38, Xiang, Haihao wrote:
> >>>>> ... this patch really fixed some issues for me and others.
> >>>>
> >>>> Can you explain this in more detail?
> >>>>
> >>>> I'd like to understand whether the issues you refer to are something
> which
> >>>> would be fixed by the ffmpeg utility allowing selection of devices for
> >>>> libavfilter, or whether they are something unrelated.
> >>>>
> >>>> (For library users the currently-supported way of getting the same
> device
> >>>> again is to keep a reference to the device and reuse it.  If there is
> some
> >>>> case where you can't do that then it would be useful to hear about it.)
> >>>
> >>> Hi Mark,
> >>>
> >>> they have 3 workaround patches on their staging repo, but I'll let Haihao
> >>> answer in detail.
> >>>
> >>> I have another question. I've been searching high and low, yet I can't
> >>> find the message. Do you remember that patch discussion from (quite a
> >>> few) months ago, where it was about another QSV change (something about
> >>> device creation from the command line, IIRC). There was a command line
> >>> example with QSV and you correctly remarked something like:
> >>> "Do you even know that just for this command line, there are 5 device
> >>> creations happening in the background, implicit and explicit, and in
> >>> one case (or 2), it's not even creating the specified device but
> >>> a session for the default device instead"
> >>> (just roughly from memory)
> >>>
> >>> Do you remember - or was it Philip?
> >>
> >> <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2021-March/277731.html>
> >>
> >>> Anyway, this is something that the patch will improve. There has been one
> >>> other commit since that time regarding explicit device creation from
> >>> Haihao (iirc), which already reduced the device creation and fixed the
> >>> incorrect default session creation.
> >>
> >> Yes, the special ffmpeg utility code to work around the lack of
> >> AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX in the libmfx decoders caused
> >> confusion by working differently to everything else - implementing that
> and
> >> getting rid of the workarounds was definitely a good thing.
> >>
> >>> My patch tackles this from another side: at that time, you (or Philip)
> >>> explained that the secondary context that QSV requires (VAAPI, D3Dx)
> >>> and that is initially created when setting up the QSV device, does not
> >>> even get used when subsequently deriving to a context of that kind.
> >>> Instead, a new device is being created in this case.
> >>>
> >>> That's another scenario which is fixed by this patch.
> >>
> >> It does sound like you just always want a libmfx device to be derived from
> >> the thing which is really there sitting underneath it.
> >
> > "That's another scenario which is fixed by this patch"
> >
> > Things stop working as expected as soon as you are working with 3 or more
> > derived hw devices and neither hwmap nor hwmap-revere can get you to the
> > context you want.
> 
> And your situation is sufficiently complex that specifying devices explicitly
> is probably what you want, rather that trying to trick some implicit route
> into returning the answer you already know.
> 
> >> If you are a library user then you get the original hw context by reusing
> the
> >> reference to it that you made when you created it.  This includes
> libavfilter
> >> users, who can provide a hw device to each hwmap separately.
> >>
> >> If you are an ffmpeg utility user then I agree there isn't currently a way
> to
> >> do this for filter graphs, hence the solution of providing an a way in the
> >> ffmpeg utility to set hw devices per-filter.
> >
> > just setting the context on a filter doesn't make any sense, because you
> need
> > the mapping. It only makes sense for the hwmap and hwupload filters.
> 
> Yes?  The filters you need to give it to are the hwmap and hwupload filters,
> the others indeed don't matter (though they are blindly set at the moment
> because there is no way to know they don't need it).
> 
> >>> Anyway I'm wondering whether it can even be logically valid to derive
> >>> from one device to another and then to another instance of the previous
> >>> device type.
> >>>   From my understanding, "deriving" or "hw mapping" from one device to
> >>> another means to establish a different way or accessor to a common
> >>> resource/data, which means that you can access the data in one or the
> >>> other way.
> >>>
> >>> Now let's assume a forward device-derivation chain like this:
> >>>
> >>> D3D_1  >>  OpenCL_1  >>  D3D_2
> >>
> >> You can't do this because device derivation is unidirectional (and
> acyclic) -
> >> you can only derive an OpenCL device from D3D (9 or 11), not the other way
> >> around.
> >>
> >> Similarly, you can only map frames from D3D to OpenCL.  That's why the
> hwmap
> >> reverse option exists, because of cases where you actually want the other
> >> direction which doesn't really exist.
> >
> > Yes, all true, but my point is something else: you can't have several
> context
> > of the same type in a derivation chain.
> 
> That's a consequence of unidirectionality + acyclity, yes.
> 
> > And that's exactly what this patch addresses: it makes sure that you'll get
> > an existing context instead of ffmpeg trying to derive to a new hw device
> > which doesn't work anyway.
> 
> I'm still only seeing one case where this bizarre operation is wanted: the
> ffmpeg utility user trying to get devices into the right place in their
> filter graphs, who I still think would be better served by being able to set
> the right device directly on hwmap rather than implicitly through searching
> derivation chains.
> 
> >>> We have D3D surfaces, then we share them with OpenCL. Both *_1
> >>> contexts provide access to the same data.
> >>> Then we derive again "forward only" and we get a new D3D_2
> >>> context. It is derived from OpenCL_1, so it must provide
> >>> access to the same data as OpenCL_1 AND D3D_1.
> >>>
> >>> Now we have two different D3D contexts which are supposed to
> >>> provide access to the same data!
> >>>
> >>>
> >>> 1. This doesn't even work technically
> >>>      - neither from D3D (iirc)
> >>>      - nor from ffmpeg (not cleanly)
> >>>
> >>> 2. This doesn't make sense at all. There should always be
> >>>      only a single device context of a device type for the same
> >>>      resource
> >>>
> >>> 3. Why would somebody even want this - what kind of use case?
> >>
> >> The multiple derivation case is for when a single device doesn't work.
> >> Typically that involves multiple separate components which don't want to
> >> interact with the others, for example:
> >>
> >> * When something thread-unsafe might happen, so different threads need
> >> separate instances to work with.
> >
> > Derivation means accessing shared resources (computing and memory), and
> > you can't solve a concurrency problem by having two devices accessing
> > the same resources - this makes it even worse (assuming a device would
> > even allow this).
> 
> Device derivation means making a compatible context of a different type on
> the same physical device.
> 
> Now that's probably intended because you are going to want to share some
> particular resources, but exactly what can be shared and what is possible is
> dependent on the setup.
> 
> Similarly, any rules for concurrency are dependent on the setup - maybe you
> can't do two specific things simultaneously in the same device context and
> need two separate ones to solve it, but they still both want to share in some
> way with the different device context they were derived from.
> 
> >> * When global options have to be set on a device, so a component which
> does
> >> that needs its own instance to avoid interfering with anyone else.
> >
> > This is NOT derivation. This case is not affected.
> 
> Suppose I have some DRM frames which come from somewhere (some hardware
> decoder, say - doesn't matter).
> 
> I want to do Vulkan things with some of the frames, so I call
> av_hwdevice_ctx_create_derived_opts() to get a compatible Vulkan context.
> Then I can map and do Vulkan things, yay!
> 
> Later on, I want to do some independent Vulkan thing with my DRM frames.  I
> do the same operation again with different options (because my new thing
> wants some new extensions, say).  This returns a new Vulkan context and I can
> work with it completely independently, yay!

You are describing the creation of a Vulkan context with other parameters
with which you can work independently.

That's not my understanding of deriving a context. I don't the implementation
in case of Vulkan, but in case of the others, deriving is about sharing 
resources. And when you share resources, you can't "work with it independently",
so what you're talking about is not a scenario of deriving a context.


To wrap things up a bit: 

- you want an approach which requires even more complicated filter 
  command lines.
  I have understood that. It is a possible addition for filter command lines
  and I would even welcome somebody who would implement precise hw context 
  selection for hwdownload  and also for hwmapn for (rare) cases where this 
  might be needed. (that somebody won't be me, though)

- but this is not what this patchset is about. it is about having things
  working nicely and automatically in a way as one would expect it instead 
  of failing. this patchset only touches and changes behavior in those cases 
  that are currently failing anyway

- Or can you name any realistic use case that this patchset would break?
  (if yes, let's please go through a specific example with pseudo code)


Maybe Haihao's reply will be more convincing.
It might also be interesting what the Vulkan guys are thinking about it
(as there has been some feedback from that side earlier).

Kind regards,
softworkz

On Mon, 2022-01-10 at 01:40 +0000, Soft Works wrote:
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> > Thompson
> > Sent: Monday, January 10, 2022 1:57 AM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving
> > a
> > hwdevice, search for existing device in both directions
> > 
> > On 09/01/2022 23:36, Soft Works wrote:>> -----Original Message-----
> > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> > > > Thompson
> > > > Sent: Monday, January 10, 2022 12:13 AM
> > > > To: ffmpeg-devel@ffmpeg.org
> > > > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> > 
> > deriving a
> > > > hwdevice, search for existing device in both directions
> > > > 
> > > > On 09/01/2022 21:15, Soft Works wrote:
> > > > > > -----Original Message-----
> > > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > > > > Mark
> > > > > > Thompson
> > > > > > Sent: Sunday, January 9, 2022 7:39 PM
> > > > > > To: ffmpeg-devel@ffmpeg.org
> > > > > > Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> > > > 
> > > > deriving a
> > > > > > hwdevice, search for existing device in both directions
> > > > > > 
> > > > > > On 05/01/2022 03:38, Xiang, Haihao wrote:
> > > > > > > ... this patch really fixed some issues for me and others.
> > > > > > 
> > > > > > Can you explain this in more detail?
> > > > > > 
> > > > > > I'd like to understand whether the issues you refer to are something
> > 
> > which
> > > > > > would be fixed by the ffmpeg utility allowing selection of devices
> > > > > > for
> > > > > > libavfilter, or whether they are something unrelated.
> > > > > > 
> > > > > > (For library users the currently-supported way of getting the same
> > 
> > device
> > > > > > again is to keep a reference to the device and reuse it.  If there
> > > > > > is
> > 
> > some
> > > > > > case where you can't do that then it would be useful to hear about
> > > > > > it.)
> > > > > 
> > > > > Hi Mark,
> > > > > 
> > > > > they have 3 workaround patches on their staging repo, but I'll let
> > > > > Haihao
> > > > > answer in detail.
> > > > > 
> > > > > I have another question. I've been searching high and low, yet I can't
> > > > > find the message. Do you remember that patch discussion from (quite a
> > > > > few) months ago, where it was about another QSV change (something
> > > > > about
> > > > > device creation from the command line, IIRC). There was a command line
> > > > > example with QSV and you correctly remarked something like:
> > > > > "Do you even know that just for this command line, there are 5 device
> > > > > creations happening in the background, implicit and explicit, and in
> > > > > one case (or 2), it's not even creating the specified device but
> > > > > a session for the default device instead"
> > > > > (just roughly from memory)
> > > > > 
> > > > > Do you remember - or was it Philip?
> > > > 
> > > > <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2021-March/277731.html>
> > > > 
> > > > > Anyway, this is something that the patch will improve. There has been
> > > > > one
> > > > > other commit since that time regarding explicit device creation from
> > > > > Haihao (iirc), which already reduced the device creation and fixed the
> > > > > incorrect default session creation.
> > > > 
> > > > Yes, the special ffmpeg utility code to work around the lack of
> > > > AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX in the libmfx decoders caused
> > > > confusion by working differently to everything else - implementing that
> > 
> > and
> > > > getting rid of the workarounds was definitely a good thing.
> > > > 
> > > > > My patch tackles this from another side: at that time, you (or Philip)
> > > > > explained that the secondary context that QSV requires (VAAPI, D3Dx)
> > > > > and that is initially created when setting up the QSV device, does not
> > > > > even get used when subsequently deriving to a context of that kind.
> > > > > Instead, a new device is being created in this case.
> > > > > 
> > > > > That's another scenario which is fixed by this patch.
> > > > 
> > > > It does sound like you just always want a libmfx device to be derived
> > > > from
> > > > the thing which is really there sitting underneath it.
> > > 
> > > "That's another scenario which is fixed by this patch"
> > > 
> > > Things stop working as expected as soon as you are working with 3 or more
> > > derived hw devices and neither hwmap nor hwmap-revere can get you to the
> > > context you want.
> > 
> > And your situation is sufficiently complex that specifying devices
> > explicitly
> > is probably what you want, rather that trying to trick some implicit route
> > into returning the answer you already know.
> > 
> > > > If you are a library user then you get the original hw context by
> > > > reusing
> > 
> > the
> > > > reference to it that you made when you created it.  This includes
> > 
> > libavfilter
> > > > users, who can provide a hw device to each hwmap separately.
> > > > 
> > > > If you are an ffmpeg utility user then I agree there isn't currently a
> > > > way
> > 
> > to
> > > > do this for filter graphs, hence the solution of providing an a way in
> > > > the
> > > > ffmpeg utility to set hw devices per-filter.
> > > 
> > > just setting the context on a filter doesn't make any sense, because you
> > 
> > need
> > > the mapping. It only makes sense for the hwmap and hwupload filters.
> > 
> > Yes?  The filters you need to give it to are the hwmap and hwupload filters,
> > the others indeed don't matter (though they are blindly set at the moment
> > because there is no way to know they don't need it).
> > 
> > > > > Anyway I'm wondering whether it can even be logically valid to derive
> > > > > from one device to another and then to another instance of the
> > > > > previous
> > > > > device type.
> > > > >   From my understanding, "deriving" or "hw mapping" from one device to
> > > > > another means to establish a different way or accessor to a common
> > > > > resource/data, which means that you can access the data in one or the
> > > > > other way.
> > > > > 
> > > > > Now let's assume a forward device-derivation chain like this:
> > > > > 
> > > > > D3D_1  >>  OpenCL_1  >>  D3D_2
> > > > 
> > > > You can't do this because device derivation is unidirectional (and
> > 
> > acyclic) -
> > > > you can only derive an OpenCL device from D3D (9 or 11), not the other
> > > > way
> > > > around.
> > > > 
> > > > Similarly, you can only map frames from D3D to OpenCL.  That's why the
> > 
> > hwmap
> > > > reverse option exists, because of cases where you actually want the
> > > > other
> > > > direction which doesn't really exist.
> > > 
> > > Yes, all true, but my point is something else: you can't have several
> > 
> > context
> > > of the same type in a derivation chain.
> > 
> > That's a consequence of unidirectionality + acyclity, yes.
> > 
> > > And that's exactly what this patch addresses: it makes sure that you'll
> > > get
> > > an existing context instead of ffmpeg trying to derive to a new hw device
> > > which doesn't work anyway.
> > 
> > I'm still only seeing one case where this bizarre operation is wanted: the
> > ffmpeg utility user trying to get devices into the right place in their
> > filter graphs, who I still think would be better served by being able to set
> > the right device directly on hwmap rather than implicitly through searching
> > derivation chains.
> > 
> > > > > We have D3D surfaces, then we share them with OpenCL. Both *_1
> > > > > contexts provide access to the same data.
> > > > > Then we derive again "forward only" and we get a new D3D_2
> > > > > context. It is derived from OpenCL_1, so it must provide
> > > > > access to the same data as OpenCL_1 AND D3D_1.
> > > > > 
> > > > > Now we have two different D3D contexts which are supposed to
> > > > > provide access to the same data!
> > > > > 
> > > > > 
> > > > > 1. This doesn't even work technically
> > > > >      - neither from D3D (iirc)
> > > > >      - nor from ffmpeg (not cleanly)
> > > > > 
> > > > > 2. This doesn't make sense at all. There should always be
> > > > >      only a single device context of a device type for the same
> > > > >      resource
> > > > > 
> > > > > 3. Why would somebody even want this - what kind of use case?
> > > > 
> > > > The multiple derivation case is for when a single device doesn't work.
> > > > Typically that involves multiple separate components which don't want to
> > > > interact with the others, for example:
> > > > 
> > > > * When something thread-unsafe might happen, so different threads need
> > > > separate instances to work with.
> > > 
> > > Derivation means accessing shared resources (computing and memory), and
> > > you can't solve a concurrency problem by having two devices accessing
> > > the same resources - this makes it even worse (assuming a device would
> > > even allow this).
> > 
> > Device derivation means making a compatible context of a different type on
> > the same physical device.
> > 
> > Now that's probably intended because you are going to want to share some
> > particular resources, but exactly what can be shared and what is possible is
> > dependent on the setup.
> > 
> > Similarly, any rules for concurrency are dependent on the setup - maybe you
> > can't do two specific things simultaneously in the same device context and
> > need two separate ones to solve it, but they still both want to share in
> > some
> > way with the different device context they were derived from.
> > 
> > > > * When global options have to be set on a device, so a component which
> > 
> > does
> > > > that needs its own instance to avoid interfering with anyone else.
> > > 
> > > This is NOT derivation. This case is not affected.
> > 
> > Suppose I have some DRM frames which come from somewhere (some hardware
> > decoder, say - doesn't matter).
> > 
> > I want to do Vulkan things with some of the frames, so I call
> > av_hwdevice_ctx_create_derived_opts() to get a compatible Vulkan context.
> > Then I can map and do Vulkan things, yay!
> > 
> > Later on, I want to do some independent Vulkan thing with my DRM frames.  I
> > do the same operation again with different options (because my new thing
> > wants some new extensions, say).  This returns a new Vulkan context and I
> > can
> > work with it completely independently, yay!
> 
> You are describing the creation of a Vulkan context with other parameters
> with which you can work independently.
> 
> That's not my understanding of deriving a context. I don't the implementation
> in case of Vulkan, but in case of the others, deriving is about sharing 
> resources. And when you share resources, you can't "work with it
> independently",
> so what you're talking about is not a scenario of deriving a context.
> 
> 
> To wrap things up a bit: 
> 
> - you want an approach which requires even more complicated filter 
>   command lines.
>   I have understood that. It is a possible addition for filter command lines
>   and I would even welcome somebody who would implement precise hw context 
>   selection for hwdownload  and also for hwmapn for (rare) cases where this 
>   might be needed. (that somebody won't be me, though)
> 
> - but this is not what this patchset is about. it is about having things
>   working nicely and automatically in a way as one would expect it instead 
>   of failing. this patchset only touches and changes behavior in those cases 
>   that are currently failing anyway
> 
> - Or can you name any realistic use case that this patchset would break?
>   (if yes, let's please go through a specific example with pseudo code)
> 
> 
> Maybe Haihao's reply will be more convincing.
> It might also be interesting what the Vulkan guys are thinking about it
> (as there has been some feedback from that side earlier).


Hi Mark,

We want to provide a more user friendly command-line to share gfx memory between
QSV, VAAPI and other HW methods.

E.g. VAAPI provides sharpness_vaapi but QSV doesn't provide a corresponding
filter, we want to use sharpness_vaapi filter on the output from QSV decoders.
Currently the first command-line below may work, however the second command line
below can't work because QSV device is not derived from a VAAPI device
explicitly, so ffmpeg fails to derive VAAPI device from QSV device (it may
derive VAAPI device from QSV device in the first case)

$ ffmpeg -init_hw_device vaapi=intel -init_hw_device qsv=qsv@intel -hwaccel qsv
-c:v h264_qsv -i input.mp4 -vf hwmap=derive_device=vaapi,sharpness_vaapi -f null
-

$ ffmpeg -hwaccel qsv -c:v h264_qsv -i input.mp4 -vf
hwmap=derive_device=vaapi,sharpness_vaapi -f null -

After applying Softworks' patch, the above two command-lines may work well. In
addition, we may use other HW methods on QSV output without copy for gfx memory,
e.g.

$ ffmpeg -hwaccel qsv -c:v h264_qsv -i input.mp4 -vf
"hwmap=derive_device=vaapi,format=vaapi,hwmap=derive_device=vulkan,scale_vulkan=
w=1920:h=1080" -f null -


Thanks
Haihao

On 10/01/2022 01:40, Soft Works wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>> Thompson
>> Sent: Monday, January 10, 2022 1:57 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
>> hwdevice, search for existing device in both directions
>>
>> [trimmed somewhat]
>>
>> Device derivation means making a compatible context of a different type on
>> the same physical device.
>>
>> Now that's probably intended because you are going to want to share some
>> particular resources, but exactly what can be shared and what is possible is
>> dependent on the setup.
>>
>> Similarly, any rules for concurrency are dependent on the setup - maybe you
>> can't do two specific things simultaneously in the same device context and
>> need two separate ones to solve it, but they still both want to share in some
>> way with the different device context they were derived from.
>>
>>>> * When global options have to be set on a device, so a component which
>> does
>>>> that needs its own instance to avoid interfering with anyone else.
>>>
>>> This is NOT derivation. This case is not affected.
>>
>> Suppose I have some DRM frames which come from somewhere (some hardware
>> decoder, say - doesn't matter).
>>
>> I want to do Vulkan things with some of the frames, so I call
>> av_hwdevice_ctx_create_derived_opts() to get a compatible Vulkan context.
>> Then I can map and do Vulkan things, yay!
>>
>> Later on, I want to do some independent Vulkan thing with my DRM frames.  I
>> do the same operation again with different options (because my new thing
>> wants some new extensions, say).  This returns a new Vulkan context and I can
>> work with it completely independently, yay!
> 
> You are describing the creation of a Vulkan context with other parameters
> with which you can work independently.
> 
> That's not my understanding of deriving a context. I don't the implementation
> in case of Vulkan, but in case of the others, deriving is about sharing
> resources. And when you share resources, you can't "work with it independently",
> so what you're talking about is not a scenario of deriving a context.

But that's what deriving a context is as currently implemented.

In my example, the Vulkan parts don't care about where the context came from.  Derivation is used to ensure that frames can be shared, but all compute happens independently.

> To wrap things up a bit:
> 
> - you want an approach which requires even more complicated filter
>    command lines.

Ha, that characterisation isn't exactly neutral - the derivation in filter graphs is removed and replaced with explicit specification of devices.

Indeed, once device creation is explicit, command lines are clearer because you don't need to reason about creation of devices inside filter graphs and whether they are actually the same device.

>    I have understood that. It is a possible addition for filter command lines
>    and I would even welcome somebody who would implement precise hw context
>    selection for hwdownload  and also for hwmapn for (rare) cases where this
>    might be needed. (that somebody won't be me, though)
> 
> - but this is not what this patchset is about. it is about having things
>    working nicely and automatically in a way as one would expect it instead
>    of failing. this patchset only touches and changes behavior in those cases
>    that are currently failing anyway

No it doesn't; it changes the behaviour of a library API when you are talking about use-cases in the ffmpeg utility.

If you need some library assistance, perhaps by making some new concept of a bundle of related devices which have the properties you want, then maybe that's a good idea.

Perhaps you even want to add to the av_hwdevice_ctx_create_derived() API so it can take a YKNOW_ACTUALLY_DONT flag to indicate you don't want to create a new derived device, I don't know.

- Mark

On 10/01/2022 06:47, Xiang, Haihao wrote:
> 
> Hi Mark,
> 
> We want to provide a more user friendly command-line to share gfx memory between
> QSV, VAAPI and other HW methods.
> 
> E.g. VAAPI provides sharpness_vaapi but QSV doesn't provide a corresponding
> filter, we want to use sharpness_vaapi filter on the output from QSV decoders.
> Currently the first command-line below may work, however the second command line
> below can't work because QSV device is not derived from a VAAPI device
> explicitly, so ffmpeg fails to derive VAAPI device from QSV device (it may
> derive VAAPI device from QSV device in the first case)
> 
> $ ffmpeg -init_hw_device vaapi=intel -init_hw_device qsv=qsv@intel -hwaccel qsv
> -c:v h264_qsv -i input.mp4 -vf hwmap=derive_device=vaapi,sharpness_vaapi -f null
> -

With explicit device selection:

$ ffmpeg -init_hw_device vaapi=intel -init_hw_device qsv=qsv@intel -hwaccel qsv -c:v h264_qsv -i input.mp4 -vf hwmap{intel},sharpness_vaapi -f null -

(Exact syntax unknown, but I was intending something like that.)

> 
> $ ffmpeg -hwaccel qsv -c:v h264_qsv -i input.mp4 -vf
> hwmap=derive_device=vaapi,sharpness_vaapi -f null -

This is really wanting the reverse case of device derivation.  I guess this does want the libmfx hwcontext to always have the source device there, as suggested above.

> After applying Softworks' patch, the above two command-lines may work well. In
> addition, we may use other HW methods on QSV output without copy for gfx memory,
> e.g.
> 
> $ ffmpeg -hwaccel qsv -c:v h264_qsv -i input.mp4 -vf
> "hwmap=derive_device=vaapi,format=vaapi,hwmap=derive_device=vulkan,scale_vulkan=
> w=1920:h=1080" -f null -

So, we move the derivation out of the graph and get:

$ ffmpeg -init_hw_device vaapi=vadev -init_hw_device qsv=qsvdev@vadev -init_hw_device vulkan=vkdev@vadev \
   -hwaccel qsv -c:v h264_qsv -i input.mp4 -vf "hwmap{vadev},format=vaapi,hwmap{vkdev},scale_vulkan=w=1920:h=1080" -f null -

(Maybe it would help to have some shorter options for making devices like the first line there, because making a set derived in the right sequence is probably quite a common operation.  If ffmpeg knows the possible-derivations this wouldn't be hard to make.)

Making it all explicit also lets multiple physical devices work together straightforwardly, as Intel seems likely to want to support in the not-too-distant future.

- Mark

On Mon, 2022-01-10 at 21:16 +0000, Mark Thompson wrote:
> On 10/01/2022 06:47, Xiang, Haihao wrote:
> > 
> > Hi Mark,
> > 
> > We want to provide a more user friendly command-line to share gfx memory
> > between
> > QSV, VAAPI and other HW methods.
> > 
> > E.g. VAAPI provides sharpness_vaapi but QSV doesn't provide a corresponding
> > filter, we want to use sharpness_vaapi filter on the output from QSV
> > decoders.
> > Currently the first command-line below may work, however the second command
> > line
> > below can't work because QSV device is not derived from a VAAPI device
> > explicitly, so ffmpeg fails to derive VAAPI device from QSV device (it may
> > derive VAAPI device from QSV device in the first case)
> > 
> > $ ffmpeg -init_hw_device vaapi=intel -init_hw_device qsv=qsv@intel -hwaccel
> > qsv
> > -c:v h264_qsv -i input.mp4 -vf hwmap=derive_device=vaapi,sharpness_vaapi -f
> > null
> > -
> 
> With explicit device selection:
> 
> $ ffmpeg -init_hw_device vaapi=intel -init_hw_device qsv=qsv@intel -hwaccel
> qsv -c:v h264_qsv -i input.mp4 -vf hwmap{intel},sharpness_vaapi -f null -
> 
> (Exact syntax unknown, but I was intending something like that.)

I got the error below if using '-vf hwmap{intel}':

[AVFilterGraph @ 0x556e3141f750] No such filter: 'hwmap{intel}'. 

I also went through the source code for the right syntax, but failed to find it.

The working syntax for me is to use '-vf hwmap=derive_device=vaapi'

> 
> > 
> > $ ffmpeg -hwaccel qsv -c:v h264_qsv -i input.mp4 -vf
> > hwmap=derive_device=vaapi,sharpness_vaapi -f null -
> 
> This is really wanting the reverse case of device derivation.  I guess this
> does want the libmfx hwcontext to always have the source device there, as
> suggested above.


Always setting the source device for qsv device will break 'make fate-hwdevice'. 

(source device can't be set when qsv device is created via '-init_hw_device
qsv=intel)

> 
> > After applying Softworks' patch, the above two command-lines may work well.
> > In
> > addition, we may use other HW methods on QSV output without copy for gfx
> > memory,
> > e.g.
> > 
> > $ ffmpeg -hwaccel qsv -c:v h264_qsv -i input.mp4 -vf
> > "hwmap=derive_device=vaapi,format=vaapi,hwmap=derive_device=vulkan,scale_vul
> > kan=
> > w=1920:h=1080" -f null -
> 
> So, we move the derivation out of the graph and get:
> 
> $ ffmpeg -init_hw_device vaapi=vadev -init_hw_device qsv=qsvdev@vadev
> -init_hw_device vulkan=vkdev@vadev \
>    -hwaccel qsv -c:v h264_qsv -i input.mp4 -vf
> "hwmap{vadev},format=vaapi,hwmap{vkdev},scale_vulkan=w=1920:h=1080" -f null -

See above, -vf hwmap{avdev} doesn't work. 

BTW you prefer to derive a device via -init_hw_device option, right ? It seems
the command line using derive_device option is more straightforward. 

Thanks
Haihao

> 
> (Maybe it would help to have some shorter options for making devices like the
> first line there, because making a set derived in the right sequence is
> probably quite a common operation.  If ffmpeg knows the possible-derivations
> this wouldn't be hard to make.)
> 
> Making it all explicit also lets multiple physical devices work together
> straightforwardly, as Intel seems likely to want to support in the not-too-
> distant future.
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Monday, January 10, 2022 9:57 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When deriving a
> hwdevice, search for existing device in both directions
> 
> On 10/01/2022 01:40, Soft Works wrote:
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> >> Thompson
> >> Sent: Monday, January 10, 2022 1:57 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v4 1/1] avutils/hwcontext: When
> deriving a
> >> hwdevice, search for existing device in both directions
> >>
> >> [trimmed somewhat]

> > To wrap things up a bit:
> >
> > - you want an approach which requires even more complicated filter
> >    command lines.
> 
> Ha, that characterisation isn't exactly neutral - the derivation in filter
> graphs is removed and replaced with explicit specification of devices.

The comparison in your other reply wasn't realistic. Actually it would
look similar to this:

BEFORE

ffmpeg -hwaccel qsv -c:v h264_qsv -i input.mp4 -vf "hwmap=derive_device=vaapi,
format=vaapi,hwmap=derive_device=vulkan,scale_vulkan=w=1920:h=1080" -f null -

AFTER

ffmpeg -init_hw_device vaapi=vadev -init_hw_device qsv=qsvdev@vadev 
-init_hw_device vulkan=vkdev@vadev -hwaccel qsv -c:v h264_qsv -i input.mp4 
-vf "hwmap=hw_device=vadev,format=vaapi,hwmap=hw_device=vkdev,scale_vulkan=w=
1920:h=1080" -f null -


This is more like a ** to all those who are writing command lines by 
hand and surely not an improvement.
I have not been involved in the cli design, but from looking at the result
it's clear that there has always been a focus on allowing things to be 
done as easily as possible instead of needing to do extensive reading 
until you get it right (or give up).


> Perhaps you even want to add to the av_hwdevice_ctx_create_derived() API so
> it can take a YKNOW_ACTUALLY_DONT flag to indicate you don't want to create a
> new derived device, I don't know.

When you mean to add av_hwdevice_ctx_create_derived2() or 
av_hwdevice_ctx_create_derived_ex() which takes an additional parameter to
control this and which the ffmpeg-tool will use instead, that would be fine 
I think.
It would allow to make the change only for the cl tool without affecting
API consumers (regardless of whether such cases may exist or not).

softworkz