1251487 - Detect texture feedback loops

Status: RESOLVED DUPLICATE of bug 1136494

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Detect-and-error-on-RTT-texture-use-on-draw.patch

We need to detect and disallow feedback loops in these cases:
* Draw call that is writing to a texture that we're sampling from.
* CopyTex*Image that is copying from and to the same texImage.

These are checked in two respective tests in the 1.0.3 conformance suite.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Detect-feedback-loops-in-CopyTex-Image.patch

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8723885 [details] [diff] [review]
0001-Detect-and-error-on-RTT-texture-use-on-draw.patch

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

Can you give an overview of the overall strategy?

::: dom/canvas/WebGLActiveInfo.h
@@ +42,4 @@
>      const uint8_t mElemSize;
>      const nsCString mBaseMappedName; // Without any final "[0]".
>  
> +    const nsTArray<WebGLRefPtr<WebGLTexture> >* const mTexArrayForUniformSampler;

The space is not needed between '>'

Also why do you store this here instead of just pulling it off of the WebGL pointer as needed. That would simplify reasoning about the ownership/lifetime.

::: dom/canvas/WebGLContextDraw.cpp
@@ +194,5 @@
> +    } else {
> +        ClearBackbufferIfNeeded();
> +    }
> +
> +    // Check for sampling+RTT feedback loops.

It's not clear to me what RTT stands for. Can you choose a better name?

@@ +206,5 @@
> +            MOZ_ASSERT(sampler->mTexArrayForUniformSampler);
> +            const auto& texArray = *(sampler->mTexArrayForUniformSampler);
> +
> +            for (const auto& texIndex : sampler->mUniformSamplerValue) {
> +                const auto& tex = texArray[texIndex];

= sampler->mTexArrayForUniformSampler[texIndex]; would be easier to read as there fewer naming indirections and texArray is not reused.

@@ +207,5 @@
> +            const auto& texArray = *(sampler->mTexArrayForUniformSampler);
> +
> +            for (const auto& texIndex : sampler->mUniformSamplerValue) {
> +                const auto& tex = texArray[texIndex];
> +                if (rttSet.find(tex.get()) != rttSetEnd) {

Can't you just use rttSet.end() here?

@@ +431,2 @@
>  
> +    if (!DoFakeVertexAttrib0(mMaxFetchedVertices))

It adds a bunch of noise for the reviewer when you include cosmetic changes in your patches. It would be a lot nicer to the reviewer if you could separate these out, especially for larger patches.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +861,4 @@
>                                        &samples](const WebGLFBAttachPoint& attach)
>      {
>          if (!attach.HasImage())
> +            return true;

It's nice to keep cosmetic changes separate.

::: dom/canvas/WebGLFramebuffer.h
@@ +176,4 @@
>  
>  private:
>      mutable bool mIsKnownFBComplete;
> +    mutable std::set<const WebGLTexture*> mCached_AttachedTextures;

Why have this a set only to change it in the following patch?

::: dom/canvas/WebGLProgram.h
@@ +63,5 @@
>      std::vector<RefPtr<WebGLActiveInfo>> transformFeedbackVaryings;
>  
> +    struct TexUnitInfo {
> +        const decltype(WebGLContext::mBound2DTextures)* const texBindingArr;
> +    };

What is this for?

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> Comment on attachment 8723885 [details] [diff] [review]
> 0001-Detect-and-error-on-RTT-texture-use-on-draw.patch
> 
> Review of attachment 8723885 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you give an overview of the overall strategy?
> 
> ::: dom/canvas/WebGLActiveInfo.h
> @@ +42,4 @@
> >      const uint8_t mElemSize;
> >      const nsCString mBaseMappedName; // Without any final "[0]".
> >  
> > +    const nsTArray<WebGLRefPtr<WebGLTexture> >* const mTexArrayForUniformSampler;
> 
> The space is not needed between '>'

Right, copied this from elsewhere. Will fix.

> 
> Also why do you store this here instead of just pulling it off of the WebGL
> pointer as needed. That would simplify reasoning about the
> ownership/lifetime.

This way saves us having to look up which array it is based on the sampler type every draw call. I'm trying to minimize the work we have to do every draw call.

> 
> ::: dom/canvas/WebGLContextDraw.cpp
> @@ +194,5 @@
> > +    } else {
> > +        ClearBackbufferIfNeeded();
> > +    }
> > +
> > +    // Check for sampling+RTT feedback loops.
> 
> It's not clear to me what RTT stands for. Can you choose a better name?

Render To Texture is the fairly common term. Wikipedia readily disambiguates this.
I hallway-tested this, and the (semi-graphical) person I asked didn't recognize it either. I'll spell it out.

> 
> @@ +206,5 @@
> > +            MOZ_ASSERT(sampler->mTexArrayForUniformSampler);
> > +            const auto& texArray = *(sampler->mTexArrayForUniformSampler);
> > +
> > +            for (const auto& texIndex : sampler->mUniformSamplerValue) {
> > +                const auto& tex = texArray[texIndex];
> 
> = sampler->mTexArrayForUniformSampler[texIndex]; would be easier to read as
> there fewer naming indirections and texArray is not reused.

It would be (*(sampler->mTexArrayForUniformSampler))[texIndex] to be unambiguous.

I would prefer to improve the 'texArray' name if you have understandability concerns.

> 
> @@ +207,5 @@
> > +            const auto& texArray = *(sampler->mTexArrayForUniformSampler);
> > +
> > +            for (const auto& texIndex : sampler->mUniformSamplerValue) {
> > +                const auto& tex = texArray[texIndex];
> > +                if (rttSet.find(tex.get()) != rttSetEnd) {
> 
> Can't you just use rttSet.end() here?

Yeah, but we access it a bunch. No I have not profiled this. But it's an easy thing to pre-hoist, in case the compiler misses it.

> 
> @@ +431,2 @@
> >  
> > +    if (!DoFakeVertexAttrib0(mMaxFetchedVertices))
> 
> It adds a bunch of noise for the reviewer when you include cosmetic changes
> in your patches. It would be a lot nicer to the reviewer if you could
> separate these out, especially for larger patches.

This was not originally a cosmetic change, it just ended up that way.

> 
> ::: dom/canvas/WebGLFramebuffer.cpp
> @@ +861,4 @@
> >                                        &samples](const WebGLFBAttachPoint& attach)
> >      {
> >          if (!attach.HasImage())
> > +            return true;
> 
> It's nice to keep cosmetic changes separate.

For larger cosmetic changes, I agree.

> 
> ::: dom/canvas/WebGLFramebuffer.h
> @@ +176,4 @@
> >  
> >  private:
> >      mutable bool mIsKnownFBComplete;
> > +    mutable std::set<const WebGLTexture*> mCached_AttachedTextures;
> 
> Why have this a set only to change it in the following patch?

Particularly for this usecase, I think the linear search through contiguous memory is probably better. Our N here is very low, but chasing pointers in a std::set is likely not worth the O(logN) vs just O(N) here.

> 
> ::: dom/canvas/WebGLProgram.h
> @@ +63,5 @@
> >      std::vector<RefPtr<WebGLActiveInfo>> transformFeedbackVaryings;
> >  
> > +    struct TexUnitInfo {
> > +        const decltype(WebGLContext::mBound2DTextures)* const texBindingArr;
> > +    };
> 
> What is this for?

Uh, it's unused. It shouldn't be there anymore.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8723886 [details] [diff] [review]
0002-Detect-feedback-loops-in-CopyTex-Image.patch

Dropping review until first patch is fixed.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Bug 1251487 - r?jrmuizel - Detect and error on RTT+texture use on

From 10348dab4268f8ee1bb037f004f42d35c187e924 Mon Sep 17 00:00:00 2001
 draw.
---
 dom/canvas/WebGLActiveInfo.cpp  |   2 +
 dom/canvas/WebGLActiveInfo.h    |   6 ++-
 dom/canvas/WebGLContext.cpp     |  34 +++++++++++++
 dom/canvas/WebGLContext.h       |   5 ++
 dom/canvas/WebGLContextDraw.cpp | 104 ++++++++++++++++++++++------------------
 dom/canvas/WebGLContextGL.cpp   |  16 +++++++
 dom/canvas/WebGLFramebuffer.cpp |  24 +++++++++-
 dom/canvas/WebGLFramebuffer.h   |   1 +
 dom/canvas/WebGLProgram.cpp     |  14 ++++--
 dom/canvas/WebGLProgram.h       |   2 +
 10 files changed, 156 insertions(+), 52 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/46271/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46271/

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: r?jrmuizel - Detect feedback loops in CopyTex*Image.

From 8de067a13046da0b45fd30b098034c28bcdf7aa8 Mon Sep 17 00:00:00 2001
---
 dom/canvas/WebGLContext.h         |  6 +++++
 dom/canvas/WebGLContextDraw.cpp   |  7 +-----
 dom/canvas/WebGLFramebuffer.cpp   | 18 ++++++++++---
 dom/canvas/WebGLFramebuffer.h     | 10 +++++++-
 dom/canvas/WebGLTextureUpload.cpp | 53 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 83 insertions(+), 11 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/46273/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46273/

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8741191 [details]
MozReview Request: Bug 1251487 - r?jrmuizel - Detect and error on RTT+texture use on

https://reviewboard.mozilla.org/r/46271/#review43087

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8741192 [details]
MozReview Request: r?jrmuizel - Detect feedback loops in CopyTex*Image.

https://reviewboard.mozilla.org/r/46273/#review43089

Comment 10

[Kelsey Gilbert [:jgilbert]]

Duping forward.