685184 - WebGL's renderbufferStorage does not emulate DEPTH_STENCIL on platforms without native support

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Patrick Cozzi]

Attachment: Testcase to reproduce the problem

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110622232440

Steps to reproduce:

Called WebGL's renderbufferStorage function from JavaScript with a DEPTH_STENCIL parameter, e.g.:

   gl.renderbufferStorage(gl.RENDERBUFFER, gl.DEPTH_STENCIL, 1, 1);


Actual results:

A following call to getError() reported INVALID_ENUM.


Expected results:

No error should be generated, and the renderbuffer storage should be created.

I receive this on a Motorola Xoom Tablet.  In pure OpenGL ES, DEPTH_STENCIL is not supported, and this call should return INVALID_ENUM.  However, WebGL added the DEPTH_STENCIL attachment point, so I would expect a WebGL-compliant implementation to support it.  See Section 6.5 of the WebGL spec:  http://www.khronos.org/registry/webgl/specs/latest/

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Will probably be fixed by 707460.

Comment 2

[Kelsey Gilbert [:jgilbert]]

This WFM, but I don't have the device this was reported against.

Comment 3

[Kelsey Gilbert [:jgilbert]]

So, we don't do any emulation here, so if the hardware doesn't support it, you'll get back an INVALID_ENUM from the driver.

I just sent an email to the WebGL list asking about this, but it is likely that it is not mandatory to support DEPTH_STENCIL where the hardware does not support it. At first glance, it seems possible (though non-trivial) to emulate, so we could consider adding that anyways. (This would be a separate bug, though)

Comment 4

[Kelsey Gilbert [:jgilbert]]

Ok, what should be done is that this should not generate an INVALID_ENUM, but should fail attach to a framebuffer with FRAMEBUFFER_UNSUPPORTED.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: Emulate DEPTH_STENCIL formats until CheckFramebufferStatus can return UNSUPPORTED.

What I do here is when DEPTH_STENCIL is not supported, but requested, we use STENCIL_INDEX8 during renderbufferStorage(), and only bind to STENCIL (instead of both) when we get framebufferRenderbuffer() with DEPTH_STENCIL. Finally, in checkFramebufferStatus(), we return UNSUPPORTED if we have a DEPTH_STENCIL attachment and we do not support DEPTH_STENCIL.

Comment 6

[Kelsey Gilbert [:jgilbert]]

It looks like it's possible to query the internal format of renderbuffers. We could try to emulate that it actually is the format they asked for, but I think it could serve as a quick out if you request a depth+stencil format and find out the resulting format is not a depth+stencil format.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Well, it sounds like we actually have to support DEPTH_STENCIL everywhere, even where support is not natively provided.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Emulate depth-stencil at the WebGL layer

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Emulate depth-stencil at the WebGL layer

Removed cruft and the force-emu #if block.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Why do we care now?

What is, exactly, the hardware that doesn't support DEPTH24_STENCIL8 ?

I'm asking because this is a fairly complex patch.

Comment 11

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #10)
> Why do we care now?
> 
> What is, exactly, the hardware that doesn't support DEPTH24_STENCIL8 ?
> 
> I'm asking because this is a fairly complex patch.

We care now because the closest hardware I've found to passing 1.0.1 is the tegra (1?) in the galaxy tab 10.1.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(Definitely not Tegra 1.  It's a dual-core 1GHz Tegra 2.)

Comment 13

[Kelsey Gilbert [:jgilbert]]

The Tegra 3 in the Nexus 7 looks like it has this problem, and indeed does not appear to have the depth_stencil extension, either.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 734957 [details] [diff] [review]
patch: Emulate depth-stencil at the WebGL layer

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

So the overall approach looks good and I now agree that we want this, but:
 - there seem to be specific bugs in the WebGLContextGL.cpp part
 - This adds quite a bit of complexity and I would like some thought to go into how to make this as easy to understand as possible (at least by adding comments if nothing else).

::: content/canvas/src/WebGLContextGL.cpp
@@ +2380,5 @@
> +        if (pname == LOCAL_GL_RENDERBUFFER_DEPTH_SIZE)
> +            gl->fBindRenderbuffer(LOCAL_GL_RENDERBUFFER, mBoundRenderbuffer->mDepthStencilEmu_DepthRB);
> +        else
> +            gl->fBindRenderbuffer(LOCAL_GL_RENDERBUFFER, mBoundRenderbuffer->mDepthStencilEmu_StencilRB);
> +    }

This is causing WebGLContext::GetRenderbufferParameter to modify the underlying GL renderbuffer binding without restoring it. I assume that's a bug?

@@ +3522,5 @@
> +        gl->fGenRenderbuffers(1, &depthRB);
> +        gl->fGenRenderbuffers(1, &stencilRB);
> +
> +        gl->fBindRenderbuffer(target, depthRB);
> +        gl->fRenderbufferStorage(target, depthFormat, width, height);

The old code had very important logic here to ensure that if the size or format changes, then we check the new buffer allocation (glRenderbufferStorage) for out-of-memory errors. This ensures that we correctly record out-of-memory or other allocation failures rather than relying on the driver to behave well in such cases. See bug 665070. This looks like it should have been said in a comment --- you'd be very welcome to add a comment if you touch this code!

::: content/canvas/src/WebGLRenderbuffer.h
@@ +54,5 @@
>  
> +    bool IsEmulatingDepthStencil() const {
> +        return mDepthStencilEmu_DepthFormat != 0;
> +    }
> +    bool IsSane() const;

What does 'sane' mean here? Please use a more specific name, or if you must introduce a whole new concept of sanity, please document it here.

@@ +71,5 @@
>  
> +    WebGLenum mDepthStencilEmu_DepthFormat;
> +    WebGLuint mDepthStencilEmu_DepthRB;
> +    WebGLenum mDepthStencilEmu_StencilFormat;
> +    WebGLuint mDepthStencilEmu_StencilRB;

This looks like doing structs without structs.

Since there is a rather significant amount of code being added here, could you isolate it in such a separate "WebGLDepthStencilEmulator" class? That would make the whole thing so much easier to feel good about.

What makes me particularly hopeful that it would be possible to split this into a separate class is that the only really nontrivial logic being added here is in WebGLRenderbuffer::ReattachToFramebuffers(), so I hope that moving this method to the separate class would leave you with only few uses of the separate class's data outside of it.

@@ +73,5 @@
> +    WebGLuint mDepthStencilEmu_DepthRB;
> +    WebGLenum mDepthStencilEmu_StencilFormat;
> +    WebGLuint mDepthStencilEmu_StencilRB;
> +
> +    std::set< std::pair<WebGLFramebuffer* const, const GLenum> > mFBAttachmentSet;

It's really hard to guess the meaning of this member just from its name and type, so a comment would be needed here.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Attachment: prereq: Do framebuffer attachment lazily

Comment 16

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: emulate depth stencil

On platforms where we need this emulation, always create a secondary renderbuffer, in case we need to use it as a stencil buffer. If we don't need it as a stencil buffer, specify the secondary buffer as a 1x1 RGBA4 dummy buffer.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: emulate depth stencil

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 753278 [details] [diff] [review]
prereq: Do framebuffer attachment lazily

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

Undo whitespace changes in WebGLContextGL.cpp.

Is UpdateAttachments called?

Nits below:

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +138,5 @@
>      return false;
>  }
>  
>  void
> +WebGLFramebuffer::Attachment::Attach(GLContext* gl, GLenum attachmentLoc) const {

Find a better verb that conveys "not lazy" ?

@@ +293,5 @@
>      // generate the INVALID_FRAMEBUFFER_OPERATION that we need here
>      if (HasDepthStencilConflict())
>          return false;
> +    // Ok, attach our chosen flavor of {DEPTH, STENCIL, DEPTH_STENCIL}.
> +    UpdateAttachments();

Move that below the checks.

@@ +352,5 @@
>      return true;
>  }
>  
> +void
> +WebGLFramebuffer::UpdateAttachments() const {

Since this runs on every draw call, it is scary to reattach everytime, so consider putting an invalidation flag on WebGLFramebuffer.

Also, need to unattach when !IsDefined().

::: content/canvas/src/WebGLFramebuffer.h
@@ +45,1 @@
>          WebGLenum mTextureCubeMapFace;

mTextureCubeMapFace is now redundant with mTextureTarget so please remove it. Redundant state is corruptible state.

@@ +58,5 @@
>          bool IsDeleteRequested() const;
>  
>          bool HasAlpha() const;
>  
> +        void SetTexture(WebGLTexture *tex, WebGLenum target, WebGLint level, WebGLenum face);

Likewise, face is now redundant with target.

@@ +97,5 @@
>          bool HasSameDimensionsAs(const Attachment& other) const;
>  
>          bool IsComplete() const;
> +
> +        void Attach(gl::GLContext* gl, GLenum attachmentLoc) const;

Why do you need a GLContext passed here?

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 753680 [details] [diff] [review]
patch: emulate depth stencil

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

::: content/canvas/src/WebGLRenderbuffer.cpp
@@ +66,5 @@
>      }
> +    
> +    GLenum primaryFormat = InternalFormatForGL();
> +    int64_t secondarySize = 0;
> +    if (!SupportsDepthStencil()) {

I would like if the logic that (does not support packed depth stencil) => (use secondary RB) was only in one place.

@@ +93,3 @@
>          case LOCAL_GL_RGBA8:
>          case LOCAL_GL_DEPTH24_STENCIL8:
> +            return 4*pixels + secondarySize;

Move the switch to a unified "get bpp for format" func and have a secondaryFormat instead of secondarySize.

@@ +98,1 @@
>      NS_ABORT();

Is the NS_ABORT needed?

@@ +99,5 @@
>      return 0;
>  }
>  
> +bool
> +WebGLRenderbuffer::SupportsDepthStencil() const {

Make it a static func, put 'packed' in name.

@@ +107,5 @@
> +
> +bool
> +WebGLRenderbuffer::NeedsDepthStencilEmu(GLenum internalFormat) const {
> +    if (internalFormat != LOCAL_GL_DEPTH24_STENCIL8)
> +        return false;

Assert this this is not LOCAL_GL_DEPTH_STENCIL

@@ +116,5 @@
> +void
> +WebGLRenderbuffer::BindRenderbuffer() const {
> +    // Do this explicitly here, since the meaning changes for depth-stencil emu.
> +    // During emu, we unconditionally bind the depth RB.
> +    // Bind (and unbind!) the stencil RB manually as needed.

I couldn't understand this comment but it is very much needed to have a comment here.

@@ +157,5 @@
> +    }
> +
> +    GLuint stencilRB = mGLName;
> +    if (NeedsDepthStencilEmu(InternalFormatForGL())) {
> +        printf_stderr("Using secondary buffer to emulate DepthStencil.\n");

printf

@@ +161,5 @@
> +        printf_stderr("Using secondary buffer to emulate DepthStencil.\n");
> +        MOZ_ASSERT(mSecondaryRB);
> +        stencilRB = mSecondaryRB;
> +    }
> +    gl->fFramebufferRenderbuffer(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_DEPTH_ATTACHMENT,   LOCAL_GL_RENDERBUFFER, mGLName);

Would be more clear with mGLName -> mPrimaryRB

@@ +173,5 @@
> +    switch (pname) {
> +        case LOCAL_GL_RENDERBUFFER_STENCIL_SIZE: {
> +            if (NeedsDepthStencilEmu(InternalFormatForGL())) {
> +                if (gl->WorkAroundDriverBugs()) {
> +                    // Only do this on Tegra.

FIXME

::: content/canvas/src/WebGLRenderbuffer.h
@@ +51,5 @@
>      }
>  
> +private:
> +    bool SupportsDepthStencil() const;
> +    bool NeedsDepthStencilEmu(GLenum internalFormat) const;

those two should be static in the .cpp file

@@ +54,5 @@
> +    bool SupportsDepthStencil() const;
> +    bool NeedsDepthStencilEmu(GLenum internalFormat) const;
> +
> +public:
> +    void BindRenderbuffer() const;

Check that naming is consistent with WebGLTexture::BindWhatever

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This is actually fairly important -- Tegra (1,2,3,4) doesn't have OES_packed_depth_stencil, so no content that uses depth_stencil renderbuffers runs on Tegra.

Comment 21

[Kelsey Gilbert [:jgilbert]]

I'll fix this up for landing, then.

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hrm, I had a quick look at this patch... it seems like a much simpler alternative would be:

- add a second GL name field to WebGLRenderbuffer
- if we create a DEPTH_STENCIL renderbuffer via RenderbufferStorage and OES_packed_depth_stencil isn't available, then create separate depth/stencil renderbuffers and stick both their names into the WebGLRenderbuffer
- FramebufferRenderbuffer needs to check if DEPTH_STENCIL is being emulated by the Renderbuffer object, and if so, bind them as separate depth + stencil.

Most of the code is already in place to handle this; we track all the attachment points separately in WebGLFramebuffer so we won't lose either the invalid state checking or previous depth/stencil attachment bindings.  Getters should also continue to just work.

That avoids needing to do lazy binding or any of that stuff.

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #22)
> Hrm, I had a quick look at this patch... it seems like a much simpler
> alternative would be:

Simpler from the perspective of writing this code, or from the perspective of future people reading this code? ;-)

(Just raising a generic concern --- haven't even looked back into this issue and into this proposal --- just something to keep in mind, and the general concern behind the r- on the original patch on this bug).

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Simpler for both, actually -- doing it the way I suggest keeps the "flow" identical as in the packed_depth_stencil case, and there's no lazy binding/applying of framebuffers.  So a good bit easier to read along and follow what it's doing; the only "magic" that happens is in RenderbufferStorage (where it would be a clear if block) and in FramebufferRenderbuffer (again, same clear block -- most of the block is already there even, just needs ~2 lines changed).

Comment 25

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: delay-fb-attach-choice2

Comment 26

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Differentiate textureTarget, texImageTarget, and internal 'face' variables

It currently seems messy how switch between textureTarget (TEXTURE_2D and TEXTURE_CUBE_MAP) and texImageTarget (TEXTURE_2D, but LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X, etc.), and also have a different 'face' range, where TEXTURE_2D is [0] and TEXTURE_CUBE_MAPs have range [0,5].

Patch size is fairly large, but it's largely because (Has)ImageInfoAt is used all over, and I minimized the leakage of our custom 'face' stuff, switching us from ImageInfoAt(level, face) to ImageInfoAt(texImageTarget, face) where possible.

It would be further nice to formalize the difference between `target` and `imageTarget` in the webgl code, but that can be done later.

Comment 27

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Do the emulation

An important note here is that this currently breaks webgl depth-stencil on Mac+NV, since we block packed_depth_stencil there, and the driver doesn't allow us to attach separate depth and stencil buffers to the same FB. (FRAMEBUFFER_UNSUPPORTED)

This seems strange, since we shouldn't have been using packed_depth_stencil on these blacklisted systems, but our code just tries DEPTH_STENCIL anyways. GL doesn't know we don't want to allow this, so it Just Works.

It looks like we've overblacklisted here, since it appears that the real problem with packed_depth_stencil was actually with the depth_texture extension. We should instead just block that, and unblock packed_depth_stencil.

FWIW, this was bug 814839, and I'm currently trying to work on that.

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 794880 [details] [diff] [review]
patch 1: delay-fb-attach-choice2

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

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +162,5 @@
> +                                     LOCAL_GL_RENDERBUFFER, Renderbuffer()->GLName());
> +        return;
> +    }
> +
> +    // Neither?

Just a suggestion, I would prefer "if ... else if ... else ..." instead of return statements. Just preferring the use of plain control structures over 'return'. You choose.

@@ +163,5 @@
> +        return;
> +    }
> +
> +    // Neither?
> +    MOZ_ASSERT(false, "FB attachment without a tex or RB.");

Isn't there a shortcut for MOZ_ASSERT(false, like MOZ_CRASH... ?

Comment 29

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 794884 [details] [diff] [review]
patch 2: Differentiate textureTarget, texImageTarget, and internal 'face' variables

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

::: content/canvas/src/WebGLFramebuffer.h
@@ +39,5 @@
>          // deleting a texture or renderbuffer immediately detaches it
>          WebGLRefPtr<WebGLTexture> mTexturePtr;
>          WebGLRefPtr<WebGLRenderbuffer> mRenderbufferPtr;
>          WebGLenum mAttachmentPoint;
> +        WebGLenum mTexImageTarget;

target is a property of a texture, not of a texture image, so rename that to mTexTarget?

More important question: isn't this redundant with mTexturePtr->Target() ? Redundant state is evil.

@@ +40,5 @@
>          WebGLRefPtr<WebGLTexture> mTexturePtr;
>          WebGLRefPtr<WebGLRenderbuffer> mRenderbufferPtr;
>          WebGLenum mAttachmentPoint;
> +        WebGLenum mTexImageTarget;
> +        WebGLint mTexImageLevel;

So, the question here is why did you have to add these two new members as opposed to getting the values from mTexturePtr?

r- for now, feel free to rerequest review with an explanation.

Comment 30

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #29)
> Comment on attachment 794884 [details] [diff] [review]
> patch 2: Differentiate textureTarget, texImageTarget, and internal 'face'
> variables
> 
> Review of attachment 794884 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLFramebuffer.h
> @@ +39,5 @@
> >          // deleting a texture or renderbuffer immediately detaches it
> >          WebGLRefPtr<WebGLTexture> mTexturePtr;
> >          WebGLRefPtr<WebGLRenderbuffer> mRenderbufferPtr;
> >          WebGLenum mAttachmentPoint;
> > +        WebGLenum mTexImageTarget;
> 
> target is a property of a texture, not of a texture image, so rename that to
> mTexTarget?
> 
> More important question: isn't this redundant with mTexturePtr->Target() ?
> Redundant state is evil.
Texture `target` is TEXTURE_2D or TEXTURE_CUBE_MAP, whereas texture `imageTarget` is TEXTURE_2D or TEXTURE_CUPE_MAP_{NEGATIVE,POSITIVE}_{X,Y,Z}.
> 
> @@ +40,5 @@
> >          WebGLRefPtr<WebGLTexture> mTexturePtr;
> >          WebGLRefPtr<WebGLRenderbuffer> mRenderbufferPtr;
> >          WebGLenum mAttachmentPoint;
> > +        WebGLenum mTexImageTarget;
> > +        WebGLint mTexImageLevel;
> 
> So, the question here is why did you have to add these two new members as
> opposed to getting the values from mTexturePtr?
> 
> r- for now, feel free to rerequest review with an explanation.

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 794884 [details] [diff] [review]
patch 2: Differentiate textureTarget, texImageTarget, and internal 'face' variables

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

r- again:

::: content/canvas/src/WebGLTexture.h
@@ +111,5 @@
>          bool mIsDefined;
>  
>          friend class WebGLTexture;
>      };
> +    

Watch out for trailing \w in this file (many occurences)

@@ +114,5 @@
>      };
> +    
> +private:
> +    static size_t FaceForTarget(WebGLenum target) {
> +        MOZ_ASSERT(target != LOCAL_GL_TEXTURE_CUBE_MAP);

This should also assert that target is not accidentally smaller than min(TEXTURE_2D, TEXTURE_CUBE_MAP_POSITIVE_X)... in case someone would have accidentally called ImageInfoAt with the old semantics, passing a face instead of a target!

@@ +118,5 @@
> +        MOZ_ASSERT(target != LOCAL_GL_TEXTURE_CUBE_MAP);
> +        return target == LOCAL_GL_TEXTURE_2D ? 0 : target - LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X;
> +    }
> +    
> +    ImageInfo& FaceInfoAt(size_t face, WebGLint level) {

It's not great to have a method named FaceInfoAt returning an ImageInfo. Maybe this method is no longer needed anyway.

@@ +130,5 @@
> +        return const_cast<WebGLTexture*>(this)->FaceInfoAt(face, level);
> +    }
> +    
> +public:
> +    ImageInfo& ImageInfoAt(WebGLenum imageTarget, WebGLint level) {

It is very dangerous to have this array accessor suddently change semantics, there is a big risk of out-of-bounds array addressing here, so I'm not convinced that that's a great idea; at the very least this should be covered by an assertion, see above comment on FaceForTarget.

Comment 32

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #31)
> Comment on attachment 794884 [details] [diff] [review]
> patch 2: Differentiate textureTarget, texImageTarget, and internal 'face'
> variables
> 
> Review of attachment 794884 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- again:
> 
> ::: content/canvas/src/WebGLTexture.h
> @@ +111,5 @@
> >          bool mIsDefined;
> >  
> >          friend class WebGLTexture;
> >      };
> > +    
> 
> Watch out for trailing \w in this file (many occurences)
> 
> @@ +114,5 @@
> >      };
> > +    
> > +private:
> > +    static size_t FaceForTarget(WebGLenum target) {
> > +        MOZ_ASSERT(target != LOCAL_GL_TEXTURE_CUBE_MAP);
> 
> This should also assert that target is not accidentally smaller than
> min(TEXTURE_2D, TEXTURE_CUBE_MAP_POSITIVE_X)... in case someone would have
> accidentally called ImageInfoAt with the old semantics, passing a face
> instead of a target!
Good catch!
> 
> @@ +118,5 @@
> > +        MOZ_ASSERT(target != LOCAL_GL_TEXTURE_CUBE_MAP);
> > +        return target == LOCAL_GL_TEXTURE_2D ? 0 : target - LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X;
> > +    }
> > +    
> > +    ImageInfo& FaceInfoAt(size_t face, WebGLint level) {
> 
> It's not great to have a method named FaceInfoAt returning an ImageInfo.
> Maybe this method is no longer needed anyway.
ImageInfo{By,At}Face? It's used in a number of places.
We should also probably add a ImageInfoAny(level) function, since that's used in a number of places. (We use FaceInfoAt(0, n))
> 
> @@ +130,5 @@
> > +        return const_cast<WebGLTexture*>(this)->FaceInfoAt(face, level);
> > +    }
> > +    
> > +public:
> > +    ImageInfo& ImageInfoAt(WebGLenum imageTarget, WebGLint level) {
> 
> It is very dangerous to have this array accessor suddently change semantics,
> there is a big risk of out-of-bounds array addressing here, so I'm not
> convinced that that's a great idea; at the very least this should be covered
> by an assertion, see above comment on FaceForTarget.
Good point.

Comment 33

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 794890 [details] [diff] [review]
patch 3: Do the emulation

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

Been meaning to r+ this a long time ago, but we're still waiting on the new version of the part 2 patch here.

Also, watch out for trailing whitespace.

Comment 34

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: delay FB attachment

r=bjacob

Comment 35

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Differentiate textureTarget, texImageTarget, and internal 'face' variables

Comment 36

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Do the emulation (do not land, has build error)

De-bitrot.
r=bjacob

Comment 37

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 812395 [details] [diff] [review]
patch 2: Differentiate textureTarget, texImageTarget, and internal 'face' variables

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

\o/ !!!

(Thanks for fixing the imageinfo accessors).

Comment 38

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=50a61501e549

Comment 39

[Benoit Jacob [:bjacob] (mostly away)]

Build error:

15:52.64 /hack/mozilla-central/content/canvas/src/WebGLRenderbuffer.cpp: In member function ‘int64_t mozilla::WebGLRenderbuffer::MemoryUsage() const’:
15:52.64 /hack/mozilla-central/content/canvas/src/WebGLRenderbuffer.cpp:116:9: error: multiple default labels in one switch
15:52.64 /hack/mozilla-central/content/canvas/src/WebGLRenderbuffer.cpp:114:9: error: this is the first default label

Comment 40

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 3 with build fixed

Comment 41

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=6f6531229fb6

Comment 42

[Benoit Jacob [:bjacob] (mostly away)]

A couple of WebGL 1.0.3 conformance tests are regressed by these patches, at least texture-fakeblack.html and mipmap-fbo.html.

At least in the case of texture-fakeblack.html, the problem seems to be that glFramebufferTexture2D is never called, because WebGL.framebufferTexture2D tries to defer it, but FinalizeAttachments never happens.

Comment 43

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Call FinalizeAttachments before early-return in no-uninitialized-renderbuffer case

This fixes it. We were returning early when renderbuffers looked good. Seems that it was unintentional that that was happening earlier than where we called FinalizeAttachments.

With that, 1.0.3 tests don't regress.

Comment 44

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #40)
> Created attachment 812700 [details] [diff] [review]
> Part 3 with build fixed

Oops, yeah, I fixed that locally, but evidently forgot to qref.

Comment 45

[Kelsey Gilbert [:jgilbert]]

This passes the renderbuffer section of the 1.0.1 tests on Tegra 2. \o/

Comment 46

[Kelsey Gilbert [:jgilbert]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2e250ee4aa43
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3ff685fe21
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/447244440bd0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/550a2b69c1f4

Comment 47

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2e250ee4aa43
https://hg.mozilla.org/mozilla-central/rev/9b3ff685fe21
https://hg.mozilla.org/mozilla-central/rev/447244440bd0
https://hg.mozilla.org/mozilla-central/rev/550a2b69c1f4