Closed Bug 1161670 Opened 5 years ago Closed 5 years ago

cache D3D11 ShaderResourceView on TextureSourceD3D11

Categories

(Core :: Graphics: Layers, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: vlad, Assigned: vlad)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch Cache SRVSplinter Review
We currently create a SRV every time we render a texture; we can instead keep it around on the TextureSourceD3D11.  (I sorta need this for some VR stuff where I'll have a SRV that was created elsewhere.. creating more than one SRV won't matter, and I guess we won't be using the SRV ourselves in my use case, but...)

This code can be simplified even more if we created a dummy 1x1 texture and SRV and filled it with pink when we create the device; if we fail to create a SRV, we could return that dummy SRV to avoid crashing.  That way we can add the warning/assertion in one place in GetShaderResourceView(), instead of needing to check the return value of that always.  Bas, let me know if you want me to do this as well. (Is there an easy way to fill a texture with a solid color? I can always just create a dummy texture and not fill it with anything too...)
Attachment #8601647 - Flags: review?(bas)
Comment on attachment 8601647 [details] [diff] [review]
Cache SRV

Review of attachment 8601647 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +795,5 @@
>        return;
>      }
>  
> +    ID3D11ShaderResourceView* srView = source->GetShaderResourceView();
> +    if (!srView) {

Probably a good idea to keep the comment from the FAILED in here.

@@ +796,5 @@
>      }
>  
> +    ID3D11ShaderResourceView* srView = source->GetShaderResourceView();
> +    if (!srView) {
> +      NS_WARNING("Couldn't get ShaderResourceView!");

Please replace these with gfxCriticalError().

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +72,5 @@
> +  if (!mSRV) {
> +    RefPtr<ID3D11Device> device;
> +    mTexture->GetDevice(byRef(device));
> +    HRESULT hr = device->CreateShaderResourceView(mTexture, nullptr, byRef(mSRV));
> +    if (FAILED(hr)) {

If this is compositor aware could we use CompositorD3D11::Failed?
Attachment #8601647 - Flags: review?(bas) → review+
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #0)
> Created attachment 8601647 [details] [diff] [review]
> Cache SRV
> 
> We currently create a SRV every time we render a texture; we can instead
> keep it around on the TextureSourceD3D11.  (I sorta need this for some VR
> stuff where I'll have a SRV that was created elsewhere.. creating more than
> one SRV won't matter, and I guess we won't be using the SRV ourselves in my
> use case, but...)
> 
> This code can be simplified even more if we created a dummy 1x1 texture and
> SRV and filled it with pink when we create the device; if we fail to create
> a SRV, we could return that dummy SRV to avoid crashing.  That way we can
> add the warning/assertion in one place in GetShaderResourceView(), instead
> of needing to check the return value of that always.  Bas, let me know if
> you want me to do this as well. (Is there an easy way to fill a texture with
> a solid color? I can always just create a dummy texture and not fill it with
> anything too...)

The way to do it would be to create a render target view and using ID3D11DeviceContext::ClearRenderTargetView, I wonder if we should just MOZ_CRASH in this case though.. something's pretty messed up in this situation. gfxCriticalErrors should be good in any case. Also, binding a null SRV shouldn't result in a crash but rather in just rendering nothing. Since I don't believe we deref it or anything.
Attached patch Updated patch (obsolete) — Splinter Review
Bas, here's an updated patch -- not much changes, other than checking for a non-null mTexture in CreateShaderResourceView.  However, this fails the "canvas/size-1.html" reftest, https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceadebde7743 .  Without the mTexture check, this caused a crash.  With the mTexture check, we end up not rendering the 100x30000 green canvas.

But I'm not sure how we were ever rendering it before; this patch doesn't really change any behaviour significantly here.  We get the same warnings before the render (about size too big etc.) that we do without this patch.  The only thing that really changed is that we don't early-return from DrawQuad, though I'm not sure if we ever early-returned at all (the DrawQuad error message is never seen in a debug log).  I suspect the CreateSRV succeeded before too, just returning null(?) for a null texture.

But I'm stumped as to why this changes behaviour.  Bas, any ideas?
Attachment #8636727 - Flags: review?(bas)
Attached patch Fixed patchSplinter Review
Ah, so the issue was that DataTextureSourceD3D11 was overriding GetD3D11Texture() to return a particular tile in the iterating case.  My GetShaderResourceView was always using mTexture.  This fixes it and adds an assert.
Attachment #8636727 - Attachment is obsolete: true
Attachment #8636727 - Flags: review?(bas)
Attachment #8637351 - Flags: review?(bas)
Comment on attachment 8637351 [details] [diff] [review]
Fixed patch

Review of attachment 8637351 [details] [diff] [review]:
-----------------------------------------------------------------

This is great work! I think this will have some additional benefits as we've seen some weird stuff going on with large amounts of SRVs.

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +952,5 @@
> +    if (!mTileSRVs[mCurrentTile]) {
> +      if (!mTileTextures[mCurrentTile]) {
> +        return nullptr;
> +      }
> +      

nit: whitespace
Whoops, I checked this in without the whitespace fix.  Sorry!  Will fix next time I touch that file.
https://hg.mozilla.org/mozilla-central/rev/6cbfc0575a01
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Attachment #8637351 - Flags: review?(bas) → review+
You need to log in before you can comment on or make changes to this bug.