diff mbox series

[FFmpeg-devel,v2,1/5] avutil/hwcontext_d3d11va: fix the uninitialized texture bindflag

Message ID 20220429104505.1747-1-tong1.wu@intel.com
State New
Headers show
Series [FFmpeg-devel,v2,1/5] avutil/hwcontext_d3d11va: fix the uninitialized texture bindflag | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Wu, Tong1 April 29, 2022, 10:45 a.m. UTC
When uploading rawvideos using d3d11va hardware framecontext, the bindflag
is not initialized and will cause creating texture failure. Now fix it,
assign it the value of D3D11_BIND_RENDER_TARGET.

Fixes:
$ ffmpeg.exe -init_hw_device d3d11va=d3d11 -f lavfi -i yuvtestsrc -vf \
"format=nv12,hwupload=extra_hw_frames=16" -f null -

Signed-off-by: Tong Wu <tong1.wu@intel.com>
---
 libavutil/hwcontext_d3d11va.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Hendrik Leppkes April 29, 2022, 10:01 p.m. UTC | #1
On Fri, Apr 29, 2022 at 12:45 PM Tong Wu
<tong1.wu-at-intel.com@ffmpeg.org> wrote:
>
> When uploading rawvideos using d3d11va hardware framecontext, the bindflag
> is not initialized and will cause creating texture failure. Now fix it,
> assign it the value of D3D11_BIND_RENDER_TARGET.
>

As with similar fixes of this nature, this implicit behavior to fix
one particular bug does not seem fitting inside the hwcontext itself.
There can be a large list of usages of the hwcontext that all require
different BindFlags, but we can only define one default - why this one
specifically?

So rather:

Where is the context created?
Why is a required flag not set there? That would be better, because
that knows what flags it needs.

- Hendrik
Soft Works April 30, 2022, 1:59 p.m. UTC | #2
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Hendrik Leppkes
> Sent: Saturday, April 30, 2022 12:02 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 1/5] avutil/hwcontext_d3d11va:
> fix the uninitialized texture bindflag
> 
> On Fri, Apr 29, 2022 at 12:45 PM Tong Wu
> <tong1.wu-at-intel.com@ffmpeg.org> wrote:
> >
> > When uploading rawvideos using d3d11va hardware framecontext, the
> bindflag
> > is not initialized and will cause creating texture failure. Now fix
> it,
> > assign it the value of D3D11_BIND_RENDER_TARGET.
> >
> 
> As with similar fixes of this nature, this implicit behavior to fix
> one particular bug does not seem fitting inside the hwcontext itself.
> There can be a large list of usages of the hwcontext that all require
> different BindFlags, but we can only define one default - why this one
> specifically?

I agree that this change is not ideal. On one side, it is "safe" in a way 
that a texture is practically unusable for video processing without having 
at least one of the flags (decoder, encoder or render_target),
so this wouldn't "hurt" anybody.

> So rather:
> 
> Where is the context created?

Looking at the command line in the commit message, this is about 
standalone D3D11 context creation. 

> Why is a required flag not set there? That would be better, because
> that knows what flags it needs.

There doesn't really exist an appropriate "there". I see two options

1. Add a generic internal device creation parameter to the dictionary
   in ffmpeg_hw.c like "standalone=1" 
   (for all devices created via init_hw_device)

Some time ago, I had another case where I thought this could be useful.
Then, this could be used in d3d11va_device_create() to set an internal
field 'default_bindflags' which would be used as condition in 
d3d11va_frames_init. The situation would remain similar though, as that
when the device is used by a decoder (which sets the decoder flag)
this needs to override the default.

2. Use a device parameter specific to the D3D11 hwcontext

This would need to be specified in the command line.
Everything else like in #1

What do you think?

Best regards,
softworkz
Xiang, Haihao May 1, 2022, 4:15 a.m. UTC | #3
On Sat, 2022-04-30 at 13:59 +0000, Soft Works wrote:
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Hendrik Leppkes
> > Sent: Saturday, April 30, 2022 12:02 AM
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH v2 1/5] avutil/hwcontext_d3d11va:
> > fix the uninitialized texture bindflag
> > 
> > On Fri, Apr 29, 2022 at 12:45 PM Tong Wu
> > <tong1.wu-at-intel.com@ffmpeg.org> wrote:
> > > 
> > > When uploading rawvideos using d3d11va hardware framecontext, the
> > 
> > bindflag
> > > is not initialized and will cause creating texture failure. Now fix
> > 
> > it,
> > > assign it the value of D3D11_BIND_RENDER_TARGET.
> > > 
> > 
> > As with similar fixes of this nature, this implicit behavior to fix
> > one particular bug does not seem fitting inside the hwcontext itself.
> > There can be a large list of usages of the hwcontext that all require
> > different BindFlags, but we can only define one default - why this one
> > specifically?
> 
> I agree that this change is not ideal. On one side, it is "safe" in a way 
> that a texture is practically unusable for video processing without having 
> at least one of the flags (decoder, encoder or render_target),
> so this wouldn't "hurt" anybody.
> 
> > So rather:
> > 
> > Where is the context created?
> 
> Looking at the command line in the commit message, this is about 
> standalone D3D11 context creation. 
> 
> > Why is a required flag not set there? That would be better, because
> > that knows what flags it needs.
> 
> There doesn't really exist an appropriate "there". I see two options
> 
> 1. Add a generic internal device creation parameter to the dictionary
>    in ffmpeg_hw.c like "standalone=1" 
>    (for all devices created via init_hw_device)
> 
> Some time ago, I had another case where I thought this could be useful.
> Then, this could be used in d3d11va_device_create() to set an internal
> field 'default_bindflags' which would be used as condition in 
> d3d11va_frames_init. The situation would remain similar though, as that
> when the device is used by a decoder (which sets the decoder flag)
> this needs to override the default.
> 
> 2. Use a device parameter specific to the D3D11
> hwcontextD3D11_BIND_RENDER_TARGET
> 
> This would need to be specified in the command line.
> Everything else like in #1
> 
> What do you think?

There aren't extra parameters for other standalone hwcontext creation. May we
take BindFlags=0 as the default setting and set texDesc.BindFlags to
D3D11_BIND_RENDER_TARGET directly ?

Thanks
Haihao
Soft Works May 1, 2022, 5:09 a.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Xiang, Haihao
> Sent: Sunday, May 1, 2022 6:15 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2 1/5] avutil/hwcontext_d3d11va:
> fix the uninitialized texture bindflag
> 
> On Sat, 2022-04-30 at 13:59 +0000, Soft Works wrote:
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Hendrik Leppkes
> > > Sent: Saturday, April 30, 2022 12:02 AM
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH v2 1/5]
> avutil/hwcontext_d3d11va:
> > > fix the uninitialized texture bindflag
> > >
> > > On Fri, Apr 29, 2022 at 12:45 PM Tong Wu
> > > <tong1.wu-at-intel.com@ffmpeg.org> wrote:
> > > >
> > > > When uploading rawvideos using d3d11va hardware framecontext,
> the
> > >
> > > bindflag
> > > > is not initialized and will cause creating texture failure. Now
> fix
> > >
> > > it,
> > > > assign it the value of D3D11_BIND_RENDER_TARGET.
> > > >
> > >
> > > As with similar fixes of this nature, this implicit behavior to
> fix
> > > one particular bug does not seem fitting inside the hwcontext
> itself.
> > > There can be a large list of usages of the hwcontext that all
> require
> > > different BindFlags, but we can only define one default - why this
> one
> > > specifically?
> >
> > I agree that this change is not ideal. On one side, it is "safe" in
> a way
> > that a texture is practically unusable for video processing without
> having
> > at least one of the flags (decoder, encoder or render_target),
> > so this wouldn't "hurt" anybody.
> >
> > > So rather:
> > >
> > > Where is the context created?
> >
> > Looking at the command line in the commit message, this is about
> > standalone D3D11 context creation.
> >
> > > Why is a required flag not set there? That would be better,
> because
> > > that knows what flags it needs.
> >
> > There doesn't really exist an appropriate "there". I see two options
> >
> > 1. Add a generic internal device creation parameter to the
> dictionary
> >    in ffmpeg_hw.c like "standalone=1"
> >    (for all devices created via init_hw_device)
> >
> > Some time ago, I had another case where I thought this could be
> useful.
> > Then, this could be used in d3d11va_device_create() to set an
> internal
> > field 'default_bindflags' which would be used as condition in
> > d3d11va_frames_init. The situation would remain similar though, as
> that
> > when the device is used by a decoder (which sets the decoder flag)
> > this needs to override the default.
> >
> > 2. Use a device parameter specific to the D3D11
> > hwcontextD3D11_BIND_RENDER_TARGET
> >
> > This would need to be specified in the command line.
> > Everything else like in #1
> >
> > What do you think?
> 
> There aren't extra parameters for other standalone hwcontext creation.
> May we
> take BindFlags=0 as the default setting and set texDesc.BindFlags to
> D3D11_BIND_RENDER_TARGET directly ?

The ffmpeg cli is not the only way how ffmpeg libs are being used.
That's why we shouldn't make assumptions which might apply when used in 
the context of ffmpeg cli.
I think that's what Hendrik wanted to point out as far as I understood.

Kind regards,
softworkz
Hendrik Leppkes May 1, 2022, 3:48 p.m. UTC | #5
On Sun, May 1, 2022 at 7:09 AM Soft Works <softworkz@hotmail.com> wrote:
> I think that's what Hendrik wanted to point out as far as I understood.
>

Basically, I want explicit behavior, not implicit defaults. Anytime a
hwcontext is created, something should tell it what its going to be
used for, so we can determine which flags make sense (or ideally, it
should just specify the flags).

This patch creates an implicit default for one use-case, is this going
to work with all others? No, of course not, because it has to know
what flags to set. Thats why explicitly setting those flags is
important, instead of just fixing one implicit case.

- Hendrik
Hendrik Leppkes May 1, 2022, 3:53 p.m. UTC | #6
On Sun, May 1, 2022 at 5:48 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> On Sun, May 1, 2022 at 7:09 AM Soft Works <softworkz@hotmail.com> wrote:
> > I think that's what Hendrik wanted to point out as far as I understood.
> >
>
> Basically, I want explicit behavior, not implicit defaults. Anytime a
> hwcontext is created, something should tell it what its going to be
> used for, so we can determine which flags make sense (or ideally, it
> should just specify the flags).
>
> This patch creates an implicit default for one use-case, is this going
> to work with all others? No, of course not, because it has to know
> what flags to set. Thats why explicitly setting those flags is
> important, instead of just fixing one implicit case.
>

On that note, the example commandline it fixes just does hwupload and
nothing else - does that even require render target to be flagged?
From what I can tell it uses a simple
ID3D11DeviceContext::CopySubresourceRegion to copy from the staging
texture, which should not require any particular bind flags. Bind
Flags in turn would then depend on what you are actually trying to do
with the texture (shader input, etc), in this example... nothing?

- Hendrik
Soft Works May 2, 2022, 6:40 a.m. UTC | #7
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Hendrik Leppkes
> Sent: Sunday, May 1, 2022 5:54 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 1/5] avutil/hwcontext_d3d11va:
> fix the uninitialized texture bindflag
> 
> On Sun, May 1, 2022 at 5:48 PM Hendrik Leppkes <h.leppkes@gmail.com>
> wrote:
> >
> > On Sun, May 1, 2022 at 7:09 AM Soft Works <softworkz@hotmail.com>
> wrote:
> > > I think that's what Hendrik wanted to point out as far as I
> understood.
> > >
> >
> > Basically, I want explicit behavior, not implicit defaults. Anytime
> a
> > hwcontext is created, something should tell it what its going to be
> > used for, so we can determine which flags make sense (or ideally, it
> > should just specify the flags).
> >
> > This patch creates an implicit default for one use-case, is this
> going
> > to work with all others? No, of course not, because it has to know
> > what flags to set. Thats why explicitly setting those flags is
> > important, instead of just fixing one implicit case.

I said I agree with you - basically, just that we need to differentiate
between the use case:

1. Use via API
   => No defaults should be applied, caller is responsible for specifying
      the flags

2. Use via ffmpeg cli
   => Applying the render target flag would be safe here.
      We could require this to set via parameter, but there won't ever
      be a case to specify something different than the render target flag

Why? Let's look at the possible flags:

D3D11_BIND_DECODER
In all decoding cases, this flag is set explicitly, so it overrides
any default we would set

D3D11_BIND_VIDEO_ENCODER
Set explicitly when required, so it overrides any default we would set

D3D11_BIND_RENDER_TARGET
All other cases require this flag (e.g. video processing)

No Flag
Dead end, texture would be unusable for any kind of video processing


> On that note, the example commandline it fixes just does hwupload and
> nothing else - does that even require render target to be flagged?
> From what I can tell it uses a simple
> ID3D11DeviceContext::CopySubresourceRegion to copy from the staging
> texture, which should not require any particular bind flags. Bind
> Flags in turn would then depend on what you are actually trying to do
> with the texture (shader input, etc), in this example... nothing?

We are still in the context of ffmpeg cli - you know that there are
no shaders or 3d operations and no etc.

But at this point, you can derive to another context or you can hwmap.
For all of those things, you need D3D11_BIND_RENDER_TARGET.


Summary

As mentioned I see two possibilities:

1. Use D3D11_BIND_RENDER_TARGET as a default when used in context of
   ffmpeg cli, otherwise no default flags

2. Require a device initialization parameter in the command line
   (but it would always be about setting the render target flag
   and there's no case where you would NOT want to set it)

Let me know what you think.

Best regards
softworkz
Wu, Tong1 May 5, 2022, 9:50 a.m. UTC | #8
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Hendrik Leppkes
> > Sent: Sunday, May 1, 2022 5:54 PM
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH v2 1/5] avutil/hwcontext_d3d11va:
> > fix the uninitialized texture bindflag
> >
> > On Sun, May 1, 2022 at 5:48 PM Hendrik Leppkes <h.leppkes@gmail.com>
> > wrote:
> > >
> > > On Sun, May 1, 2022 at 7:09 AM Soft Works <softworkz@hotmail.com>
> > wrote:
> > > > I think that's what Hendrik wanted to point out as far as I
> > understood.
> > > >
> > >
> > > Basically, I want explicit behavior, not implicit defaults. Anytime
> > a
> > > hwcontext is created, something should tell it what its going to be
> > > used for, so we can determine which flags make sense (or ideally, it
> > > should just specify the flags).
> > >
> > > This patch creates an implicit default for one use-case, is this
> > going
> > > to work with all others? No, of course not, because it has to know
> > > what flags to set. Thats why explicitly setting those flags is
> > > important, instead of just fixing one implicit case.
> 
> I said I agree with you - basically, just that we need to differentiate between
> the use case:
> 
> 1. Use via API
>    => No defaults should be applied, caller is responsible for specifying
>       the flags
> 
> 2. Use via ffmpeg cli
>    => Applying the render target flag would be safe here.
>       We could require this to set via parameter, but there won't ever
>       be a case to specify something different than the render target flag
> 
> Why? Let's look at the possible flags:
> 
> D3D11_BIND_DECODER
> In all decoding cases, this flag is set explicitly, so it overrides any default we
> would set
> 
> D3D11_BIND_VIDEO_ENCODER
> Set explicitly when required, so it overrides any default we would set
> 
> D3D11_BIND_RENDER_TARGET
> All other cases require this flag (e.g. video processing)
> 
> No Flag
> Dead end, texture would be unusable for any kind of video processing
> 
> 
> > On that note, the example commandline it fixes just does hwupload and
> > nothing else - does that even require render target to be flagged?
> > From what I can tell it uses a simple
> > ID3D11DeviceContext::CopySubresourceRegion to copy from the staging
> > texture, which should not require any particular bind flags. Bind
> > Flags in turn would then depend on what you are actually trying to do
> > with the texture (shader input, etc), in this example... nothing?
> 
> We are still in the context of ffmpeg cli - you know that there are no shaders
> or 3d operations and no etc.
> 
> But at this point, you can derive to another context or you can hwmap.
> For all of those things, you need D3D11_BIND_RENDER_TARGET.
> 
> 
> Summary
> 
> As mentioned I see two possibilities:
> 
> 1. Use D3D11_BIND_RENDER_TARGET as a default when used in context of
>    ffmpeg cli, otherwise no default flags
> 
> 2. Require a device initialization parameter in the command line
>    (but it would always be about setting the render target flag
>    and there's no case where you would NOT want to set it)
> 

Thanks for the possible solutions for this issue. The No.1 seems more reasonable because
No.2 just seems more unnecessary. But I will still need to find a better place to set the flag.
Before that I am going to resubmit the other 4 patches which have less comments and objections
for further review.

Thanks,
Tong 

> Let me know what you think.
> 
> Best regards
> softworkz
> 
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
> with subject "unsubscribe".
Soft Works May 6, 2022, 1:10 a.m. UTC | #9
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Wu,
> Tong1
> Sent: Thursday, May 5, 2022 11:50 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 1/5] avutil/hwcontext_d3d11va:
> fix the uninitialized texture bindflag
> 
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Hendrik Leppkes
> > > Sent: Sunday, May 1, 2022 5:54 PM
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH v2 1/5]
> avutil/hwcontext_d3d11va:
> > > fix the uninitialized texture bindflag
> > >
> > > On Sun, May 1, 2022 at 5:48 PM Hendrik Leppkes
> <h.leppkes@gmail.com>
> > > wrote:
> > > >
> > > > On Sun, May 1, 2022 at 7:09 AM Soft Works
> <softworkz@hotmail.com>
> > > wrote:
> > > > > I think that's what Hendrik wanted to point out as far as I
> > > understood.
> > > > >
> > > >
> > > > Basically, I want explicit behavior, not implicit defaults.
> Anytime
> > > a
> > > > hwcontext is created, something should tell it what its going to
> be
> > > > used for, so we can determine which flags make sense (or
> ideally, it
> > > > should just specify the flags).
> > > >
> > > > This patch creates an implicit default for one use-case, is this
> > > going
> > > > to work with all others? No, of course not, because it has to
> know
> > > > what flags to set. Thats why explicitly setting those flags is
> > > > important, instead of just fixing one implicit case.
> >
> > I said I agree with you - basically, just that we need to
> differentiate between
> > the use case:
> >
> > 1. Use via API
> >    => No defaults should be applied, caller is responsible for
> specifying
> >       the flags
> >
> > 2. Use via ffmpeg cli
> >    => Applying the render target flag would be safe here.
> >       We could require this to set via parameter, but there won't
> ever
> >       be a case to specify something different than the render
> target flag
> >
> > Why? Let's look at the possible flags:
> >
> > D3D11_BIND_DECODER
> > In all decoding cases, this flag is set explicitly, so it overrides
> any default we
> > would set
> >
> > D3D11_BIND_VIDEO_ENCODER
> > Set explicitly when required, so it overrides any default we would
> set
> >
> > D3D11_BIND_RENDER_TARGET
> > All other cases require this flag (e.g. video processing)
> >
> > No Flag
> > Dead end, texture would be unusable for any kind of video processing
> >
> >
> > > On that note, the example commandline it fixes just does hwupload
> and
> > > nothing else - does that even require render target to be flagged?
> > > From what I can tell it uses a simple
> > > ID3D11DeviceContext::CopySubresourceRegion to copy from the
> staging
> > > texture, which should not require any particular bind flags. Bind
> > > Flags in turn would then depend on what you are actually trying to
> do
> > > with the texture (shader input, etc), in this example... nothing?
> >
> > We are still in the context of ffmpeg cli - you know that there are
> no shaders
> > or 3d operations and no etc.
> >
> > But at this point, you can derive to another context or you can
> hwmap.
> > For all of those things, you need D3D11_BIND_RENDER_TARGET.
> >
> >
> > Summary
> >
> > As mentioned I see two possibilities:
> >
> > 1. Use D3D11_BIND_RENDER_TARGET as a default when used in context of
> >    ffmpeg cli, otherwise no default flags
> >
> > 2. Require a device initialization parameter in the command line
> >    (but it would always be about setting the render target flag
> >    and there's no case where you would NOT want to set it)
> >
> 
> Thanks for the possible solutions for this issue. The No.1 seems more
> reasonable because
> No.2 just seems more unnecessary. But I will still need to find a
> better place to set the flag.

I would favor #1 as well. 

Regarding "better place to set the flag":

The BindFlag needs to be set when initializing a FRAMES context.
But at this point, there's no way to determine whether the code is running
in an ffmpeg cli process or used by an API consumer.

But there would be an opportunity to convey this information on 
device init. The device (D3D11VA) could then set an internal flag
from device init and use this on frames init to determine which default
to use for the BindFlags value.

Remains the question how ffmpeg cli can convey this information to 
the device (for use during device init).

What I thought would be to have ffmpeg.c (or rather ffmpeg_hw.c)
to ALWAYS (for all hwaccels) add an entry to the options dictionary,
something like "cli" or similar.
This would avoid introducing a "special-case" mechanism just for 
this case (=> by making it a common behavior).

There are other cases where this might be useful in order to
discriminate API usage from cli usage.

But let's see what the others think first..


Kind regards,
softworkz
Hendrik Leppkes May 6, 2022, 5:37 a.m. UTC | #10
On Fri, May 6, 2022 at 3:11 AM Soft Works <softworkz@hotmail.com> wrote:
>
>
>
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Wu,
> > Tong1
> > Sent: Thursday, May 5, 2022 11:50 AM
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH v2 1/5] avutil/hwcontext_d3d11va:
> > fix the uninitialized texture bindflag
> >
> > > > -----Original Message-----
> > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > > Hendrik Leppkes
> > > > Sent: Sunday, May 1, 2022 5:54 PM
> > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > devel@ffmpeg.org>
> > > > Subject: Re: [FFmpeg-devel] [PATCH v2 1/5]
> > avutil/hwcontext_d3d11va:
> > > > fix the uninitialized texture bindflag
> > > >
> > > > On Sun, May 1, 2022 at 5:48 PM Hendrik Leppkes
> > <h.leppkes@gmail.com>
> > > > wrote:
> > > > >
> > > > > On Sun, May 1, 2022 at 7:09 AM Soft Works
> > <softworkz@hotmail.com>
> > > > wrote:
> > > > > > I think that's what Hendrik wanted to point out as far as I
> > > > understood.
> > > > > >
> > > > >
> > > > > Basically, I want explicit behavior, not implicit defaults.
> > Anytime
> > > > a
> > > > > hwcontext is created, something should tell it what its going to
> > be
> > > > > used for, so we can determine which flags make sense (or
> > ideally, it
> > > > > should just specify the flags).
> > > > >
> > > > > This patch creates an implicit default for one use-case, is this
> > > > going
> > > > > to work with all others? No, of course not, because it has to
> > know
> > > > > what flags to set. Thats why explicitly setting those flags is
> > > > > important, instead of just fixing one implicit case.
> > >
> > > I said I agree with you - basically, just that we need to
> > differentiate between
> > > the use case:
> > >
> > > 1. Use via API
> > >    => No defaults should be applied, caller is responsible for
> > specifying
> > >       the flags
> > >
> > > 2. Use via ffmpeg cli
> > >    => Applying the render target flag would be safe here.
> > >       We could require this to set via parameter, but there won't
> > ever
> > >       be a case to specify something different than the render
> > target flag
> > >
> > > Why? Let's look at the possible flags:
> > >
> > > D3D11_BIND_DECODER
> > > In all decoding cases, this flag is set explicitly, so it overrides
> > any default we
> > > would set
> > >
> > > D3D11_BIND_VIDEO_ENCODER
> > > Set explicitly when required, so it overrides any default we would
> > set
> > >
> > > D3D11_BIND_RENDER_TARGET
> > > All other cases require this flag (e.g. video processing)
> > >
> > > No Flag
> > > Dead end, texture would be unusable for any kind of video processing
> > >
> > >
> > > > On that note, the example commandline it fixes just does hwupload
> > and
> > > > nothing else - does that even require render target to be flagged?
> > > > From what I can tell it uses a simple
> > > > ID3D11DeviceContext::CopySubresourceRegion to copy from the
> > staging
> > > > texture, which should not require any particular bind flags. Bind
> > > > Flags in turn would then depend on what you are actually trying to
> > do
> > > > with the texture (shader input, etc), in this example... nothing?
> > >
> > > We are still in the context of ffmpeg cli - you know that there are
> > no shaders
> > > or 3d operations and no etc.
> > >
> > > But at this point, you can derive to another context or you can
> > hwmap.
> > > For all of those things, you need D3D11_BIND_RENDER_TARGET.
> > >
> > >
> > > Summary
> > >
> > > As mentioned I see two possibilities:
> > >
> > > 1. Use D3D11_BIND_RENDER_TARGET as a default when used in context of
> > >    ffmpeg cli, otherwise no default flags
> > >
> > > 2. Require a device initialization parameter in the command line
> > >    (but it would always be about setting the render target flag
> > >    and there's no case where you would NOT want to set it)
> > >
> >
> > Thanks for the possible solutions for this issue. The No.1 seems more
> > reasonable because
> > No.2 just seems more unnecessary. But I will still need to find a
> > better place to set the flag.
>
> I would favor #1 as well.
>
> Regarding "better place to set the flag":
>
> The BindFlag needs to be set when initializing a FRAMES context.
> But at this point, there's no way to determine whether the code is running
> in an ffmpeg cli process or used by an API consumer.
>
> But there would be an opportunity to convey this information on
> device init. The device (D3D11VA) could then set an internal flag
> from device init and use this on frames init to determine which default
> to use for the BindFlags value.
>
> Remains the question how ffmpeg cli can convey this information to
> the device (for use during device init).
>
> What I thought would be to have ffmpeg.c (or rather ffmpeg_hw.c)
> to ALWAYS (for all hwaccels) add an entry to the options dictionary,
> something like "cli" or similar.
> This would avoid introducing a "special-case" mechanism just for
> this case (=> by making it a common behavior).
>
> There are other cases where this might be useful in order to
> discriminate API usage from cli usage.
>
> But let's see what the others think first..
>

Think of the CLI applications as an API user, and design for that,
because thats really what they are, and thats how you actually end up
with a good API that covers more cases.
If special CLI logic is needed, it should be in the CLI applications,
and not in the libraries.

- Hendrik
Soft Works May 6, 2022, 5:50 a.m. UTC | #11
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Hendrik Leppkes
> Sent: Friday, May 6, 2022 7:38 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 1/5] avutil/hwcontext_d3d11va:
> fix the uninitialized texture bindflag
> 
> On Fri, May 6, 2022 at 3:11 AM Soft Works <softworkz@hotmail.com>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Wu,
> > > Tong1
> > > Sent: Thursday, May 5, 2022 11:50 AM
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH v2 1/5]
> avutil/hwcontext_d3d11va:
> > > fix the uninitialized texture bindflag
> > >
> > > > > -----Original Message-----
> > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> > > > > Hendrik Leppkes
> > > > > Sent: Sunday, May 1, 2022 5:54 PM
> > > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > > devel@ffmpeg.org>
> > > > > Subject: Re: [FFmpeg-devel] [PATCH v2 1/5]
> > > avutil/hwcontext_d3d11va:
> > > > > fix the uninitialized texture bindflag
> > > > >
> > > > > On Sun, May 1, 2022 at 5:48 PM Hendrik Leppkes
> > > <h.leppkes@gmail.com>
> > > > > wrote:
> > > > > >
> > > > > > On Sun, May 1, 2022 at 7:09 AM Soft Works
> > > <softworkz@hotmail.com>
> > > > > wrote:
> > > > > > > I think that's what Hendrik wanted to point out as far as
> I
> > > > > understood.
> > > > > > >
> > > > > >
> > > > > > Basically, I want explicit behavior, not implicit defaults.
> > > Anytime
> > > > > a
> > > > > > hwcontext is created, something should tell it what its
> going to
> > > be
> > > > > > used for, so we can determine which flags make sense (or
> > > ideally, it
> > > > > > should just specify the flags).
> > > > > >
> > > > > > This patch creates an implicit default for one use-case, is
> this
> > > > > going
> > > > > > to work with all others? No, of course not, because it has
> to
> > > know
> > > > > > what flags to set. Thats why explicitly setting those flags
> is
> > > > > > important, instead of just fixing one implicit case.
> > > >
> > > > I said I agree with you - basically, just that we need to
> > > differentiate between
> > > > the use case:
> > > >
> > > > 1. Use via API
> > > >    => No defaults should be applied, caller is responsible for
> > > specifying
> > > >       the flags
> > > >
> > > > 2. Use via ffmpeg cli
> > > >    => Applying the render target flag would be safe here.
> > > >       We could require this to set via parameter, but there
> won't
> > > ever
> > > >       be a case to specify something different than the render
> > > target flag
> > > >
> > > > Why? Let's look at the possible flags:
> > > >
> > > > D3D11_BIND_DECODER
> > > > In all decoding cases, this flag is set explicitly, so it
> overrides
> > > any default we
> > > > would set
> > > >
> > > > D3D11_BIND_VIDEO_ENCODER
> > > > Set explicitly when required, so it overrides any default we
> would
> > > set
> > > >
> > > > D3D11_BIND_RENDER_TARGET
> > > > All other cases require this flag (e.g. video processing)
> > > >
> > > > No Flag
> > > > Dead end, texture would be unusable for any kind of video
> processing
> > > >
> > > >
> > > > > On that note, the example commandline it fixes just does
> hwupload
> > > and
> > > > > nothing else - does that even require render target to be
> flagged?
> > > > > From what I can tell it uses a simple
> > > > > ID3D11DeviceContext::CopySubresourceRegion to copy from the
> > > staging
> > > > > texture, which should not require any particular bind flags.
> Bind
> > > > > Flags in turn would then depend on what you are actually
> trying to
> > > do
> > > > > with the texture (shader input, etc), in this example...
> nothing?
> > > >
> > > > We are still in the context of ffmpeg cli - you know that there
> are
> > > no shaders
> > > > or 3d operations and no etc.
> > > >
> > > > But at this point, you can derive to another context or you can
> > > hwmap.
> > > > For all of those things, you need D3D11_BIND_RENDER_TARGET.
> > > >
> > > >
> > > > Summary
> > > >
> > > > As mentioned I see two possibilities:
> > > >
> > > > 1. Use D3D11_BIND_RENDER_TARGET as a default when used in
> context of
> > > >    ffmpeg cli, otherwise no default flags
> > > >
> > > > 2. Require a device initialization parameter in the command line
> > > >    (but it would always be about setting the render target flag
> > > >    and there's no case where you would NOT want to set it)
> > > >
> > >
> > > Thanks for the possible solutions for this issue. The No.1 seems
> more
> > > reasonable because
> > > No.2 just seems more unnecessary. But I will still need to find a
> > > better place to set the flag.
> >
> > I would favor #1 as well.
> >
> > Regarding "better place to set the flag":
> >
> > The BindFlag needs to be set when initializing a FRAMES context.
> > But at this point, there's no way to determine whether the code is
> running
> > in an ffmpeg cli process or used by an API consumer.
> >
> > But there would be an opportunity to convey this information on
> > device init. The device (D3D11VA) could then set an internal flag
> > from device init and use this on frames init to determine which
> default
> > to use for the BindFlags value.
> >
> > Remains the question how ffmpeg cli can convey this information to
> > the device (for use during device init).
> >
> > What I thought would be to have ffmpeg.c (or rather ffmpeg_hw.c)
> > to ALWAYS (for all hwaccels) add an entry to the options dictionary,
> > something like "cli" or similar.
> > This would avoid introducing a "special-case" mechanism just for
> > this case (=> by making it a common behavior).
> >
> > There are other cases where this might be useful in order to
> > discriminate API usage from cli usage.
> >
> > But let's see what the others think first..
> >
> 
> Think of the CLI applications as an API user, and design for that,
> because thats really what they are, and thats how you actually end up
> with a good API that covers more cases.
> If special CLI logic is needed, it should be in the CLI applications,
> and not in the libraries.
> 
> - Hendrik

Thanks. When I translate that, it would be a BindFlags entry in the options
dictionary, which any API user _could_ set and which ffmpeg cli _does_
set in ffmpeg_hw.c (to render target). The D3D11VA hw context stores this
on device init and uses it as a default in frames init. 
The default of the default is 0 of course.

Does that sound ok to you?

Kind regards,
softworkz
diff mbox series

Patch

diff --git a/libavutil/hwcontext_d3d11va.c b/libavutil/hwcontext_d3d11va.c
index 8ab96bad25..db529acbb4 100644
--- a/libavutil/hwcontext_d3d11va.c
+++ b/libavutil/hwcontext_d3d11va.c
@@ -254,6 +254,11 @@  static int d3d11va_frames_init(AVHWFramesContext *ctx)
         return AVERROR(EINVAL);
     }
 
+    if (!hwctx->BindFlags) {
+        av_log(ctx, AV_LOG_DEBUG, "Add render target bindflag for texture\n");
+        hwctx->BindFlags = D3D11_BIND_RENDER_TARGET;
+    }
+
     texDesc = (D3D11_TEXTURE2D_DESC){
         .Width      = ctx->width,
         .Height     = ctx->height,