922810 - WebGL: defer/avoid doing any texture image initialization, instead extend the 'fake black textures' system

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

The WebGL spec, 4.1 Resource Restrictions, says that any image data that would be uninitialized in the corresponding OpenGL situation, must be initialized by zero. So far, here is how we have implemented these initalization semantics:
 - for renderbuffers, we have been tracking whether they were initialized, and we initialized them at the last minute by doing a glClear on the already-assembled FBO.
 - for textures, we were initializing them immediately with a calloc'd temporary buffer.
 - for depth textures on ANGLE, where texImage2D can't allocate any data, in bug 783914, we've created a dummy FBO just to be able to glClear them.

Meanwhile, we already have a concept of 'fake black' textures, which are dummy 1x1 black textures, which we use to implement correct OpenGL ES semantics on incomplete textures (which ES says must sample as RGBA(0,0,0,1)).

The present patch set expands that existing 'fake black textures' system, and the existing tracking of uninitialized renderbuffers, to be able to handle uninitialized texture images. This means that in many cases we're able to avoid doing any zero-initialization of texture images, and in other cases (where we are about to be sampling from a texture that has some undefined image data and some properly initialized image data and is texture-complete) we defer the calloc'ing and texImage2D'ing to the last minute.

All of the patches here build and pass the relevant 1.0.3 conformance tests (the ones under textures/ and renderbuffers/).

Comment 1

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

Attachment: Part 1: track image initialization with a WebGLImageDataStatus enum

Start by WebGLTypes.h, which has a comment.

Comment 2

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

Attachment: Part 2: reorganize the tracking of fake back statuses

Here too, start by WebGLTypes.h, there is a comment about the two enums.

Comment 3

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

Attachment: Part 3: introduce transparent fake black textures alongside existing opaque onesnew-fake-black-textures

I know it's getting ugly: twice more custom GL textures that we create all at once (even if we only need one) and manually manage. This gets cleaned up and optimized in Part 6.

Comment 4

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

Attachment: Part 4: make BindTexture smart enough to not clear the context's mFakeBlackStatus when nothing changed

While texture object state doesn't change often, texture bindings do change often, and they were always clearing the context's fakeblackstatus, causing it to be recomputed --- which requires iterating over texture units --- at the next draw operation. This patch lets us avoid doing this useless work.

Comment 5

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

Attachment: Part 5: do not initialize depth textures before they get rendered to, use a fake black texture instead

This is the part that will hopefully make things better on bug 783914: with that patch, ANGLE_depth_texture becomes equivalent to true depth_texture support for all WebGL purposes, and we no longer have an ANGLE-specific code path there.

Comment 6

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

Attachment: Part 6: Clean up and optimize fake black textures implementation

This introduces a little helper class for fake black textures, allowing to clean up some code, and makes us not even create fake black textures that we don't use. This is starting to be worth it as there now are 4 fake black textures (2d vs cubemap, opaque vs transparent).

Comment 7

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

Attachment: Part 7: extend ScopedBindTexture to support cube maps

This is just preparation work for part 8: ScopedBindTextures didn't support cube maps.

Comment 8

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

Attachment: Part 8: Defer/avoid initialization of texture images

The special case of depth textures was handled in part 5. Now this is the generic case, where we were calloc'ing.

We can't always avoid initialization: if the user wants to sample from a texture that is complete, and that had both uninitialized and initialized images, then we have at that point to initialize the uninitialized images. Another case is when the user wants to texSubImage2D into an uninitialized texture image. That's done in the WebGLTexture::DoDeferredImageInitialization.

But in other cases (e.g. sampling from a texture with only uninitialized image data) we can avoid initialization and just use a fake black texture.

Comment 9

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

Results will be displayed on TBPL as they come in:
https://tbpl.mozilla.org/?tree=Try&rev=561b0950ae76

Once completed, builds and logs will be available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-561b0950ae76

Comment 10

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

Attachment: Part 8: Defer/avoid initialization of texture images

The previous version crashed in quickCheckAPI-S_V.html. The reason was some mixup from the ImageInfoAt face->imageTarget change.

Comment 11

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

Pushed again to try (the previous try was orange because of that crash):

Results will be displayed on TBPL as they come in:
https://tbpl.mozilla.org/?tree=Try&rev=a51da2b55870

Once completed, builds and logs will be available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a51da2b55870

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 812788 [details] [diff] [review]
Part 1: track image initialization with a WebGLImageDataStatus enum

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3711,5 @@
>      internalformat = InternalFormatForFormatAndType(format, type, gl->IsGLES2());
>  
>      GLenum error = LOCAL_GL_NO_ERROR;
>  
> +    WebGLImageDataStatus imageInfoStatusIfSuccess = WebGLImageDataStatus::NoImageData;

Why is this 'no' and not 'uninit'?

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +73,5 @@
> +    } else if (mTexturePtr) {
> +        if (!mTexturePtr->HasImageInfoAt(mTexImageTarget, mTexImageLevel))
> +            return false;
> +        return mTexturePtr->ImageInfoAt(mTexImageTarget, mTexImageLevel).HasUninitializedImageData();
> +    } else return false;

} else
  return false

::: content/canvas/src/WebGLTexture.cpp
@@ +44,5 @@
>  }
>  
>  int64_t
>  WebGLTexture::ImageInfo::MemoryUsage() const {
> +    if (mImageDataStatus == WebGLImageDataStatus::NoImageData)

Shouldn't we return zero for uninit also, since it doesn't actually take extra memory? Otherwise we'll end up over-reporting.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 812791 [details] [diff] [review]
Part 2: reorganize the tracking of fake back statuses

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

::: content/canvas/src/WebGLTexture.cpp
@@ +26,5 @@
>      , mWrapT(LOCAL_GL_REPEAT)
>      , mFacesCount(0)
>      , mMaxLevelWithCustomImages(0)
>      , mHaveGeneratedMipmap(false)
> +    , mFakeBlackStatus(WebGLTextureFakeBlackStatus::NotNeeded)

Shouldn't this default to Unknown?
Or Incomplete, since IIRC that's what textures are before they're specified?

@@ +246,2 @@
>  
> +    if (mFakeBlackStatus == WebGLTextureFakeBlackStatus::Unknown) {

Just early-out if it's not unknown.

@@ +359,5 @@
> +                hasUninitializedImageData |= (ImageInfoAtFace(face, level).mImageDataStatus == WebGLImageDataStatus::UninitializedImageData);
> +            }
> +        }
> +        if (hasUninitializedImageData) {
> +            mFakeBlackStatus = FormatHasAlpha(ImageInfoAtFace(0, 0).mFormat)

Please use ImageInfoBase() instead of ImageInfoAtFace().

@@ +369,3 @@
>          // we have exhausted all cases where we do need fakeblack, so if the status is still unknown,
>          // that means that we do NOT need it.
> +        if (mFakeBlackStatus == WebGLTextureFakeBlackStatus::Unknown)

If we early-out non-unknown statuses above, this can be unconditional, and we can drop an unnecessary scope above.

@@ +373,3 @@
>      }
>  
> +    return mFakeBlackStatus;

Then we should explicitly assert that this is non-unknown, to make it really obvious.

::: content/canvas/src/WebGLTexture.h
@@ +233,5 @@
>                        GLenum aFormat, GLenum aType, WebGLImageDataStatus aStatus);
>  
>      void SetMinFilter(GLenum aMinFilter) {
>          mMinFilter = aMinFilter;
> +        SetFakeBlackStatus(WebGLTextureFakeBlackStatus::Unknown);

I think I prefer the old way, where we call a func that handles the detail of what to do.

::: content/canvas/src/WebGLTypes.h
@@ +57,5 @@
> +MOZ_BEGIN_ENUM_CLASS(WebGLTextureFakeBlackStatus, int)
> +  Unknown,
> +  NotNeeded,
> +  NeedOpaque,
> +  NeedTransparent

I think I'd prefer 'NeedFakeIncomplete' and 'NeedFakeUninitialized', since that is easier to think with.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 812793 [details] [diff] [review]
Part 3: introduce transparent fake black textures alongside existing opaque onesnew-fake-black-textures

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +1000,5 @@
>      for (int32_t i = 0; i < mGLMaxTextureUnits; ++i) {
>          WebGLTextureFakeBlackStatus s;
>          s = mBound2DTextures[i]
>              ? mBound2DTextures[i]->ResolvedFakeBlackStatus()
>              : WebGLTextureFakeBlackStatus::NotNeeded;

Assert that we're non-unknown.

Comment 15

[Jukka Jylänki]

(In reply to Benoit Jacob [:bjacob] from comment #11)
> Pushed again to try (the previous try was orange because of that crash):
> 
> Results will be displayed on TBPL as they come in:
> https://tbpl.mozilla.org/?tree=Try&rev=a51da2b55870
> 
> Once completed, builds and logs will be available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> a51da2b55870

I now tested this try build on Windows, and as result:
- cases 1,2,3,A and B work (as they did before) from comment 50 at https://bugzilla.mozilla.org/show_bug.cgi?id=783914#c50
- cases I and II from that same comment do not work. (they give a blankscreen without a cube)
- cases I, II and III from https://bugzilla.mozilla.org/show_bug.cgi?id=922875 do not work. (blank screen in each case)

Comment 16

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

(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> > +    WebGLImageDataStatus imageInfoStatusIfSuccess = WebGLImageDataStatus::NoImageData;
> 
> Why is this 'no' and not 'uninit'?

Because I meant for this default case to be an error case, and assert below to guard against it (we should always assign something else to imageInfoStatusIfSuccess below), but apparently I forgot. Will refresh the patch.

> > +    } else return false;
> 
> } else
>   return false

Well, I suppose that proper gecko coding style mandates braces also after the else. refreshing.

> 
> ::: content/canvas/src/WebGLTexture.cpp
> @@ +44,5 @@
> >  }
> >  
> >  int64_t
> >  WebGLTexture::ImageInfo::MemoryUsage() const {
> > +    if (mImageDataStatus == WebGLImageDataStatus::NoImageData)
> 
> Shouldn't we return zero for uninit also, since it doesn't actually take
> extra memory? Otherwise we'll end up over-reporting.

No, note that this imagedatastatus is per-imageinfo, not per-texture, and that uninitialized means "allocated, but contents left uninitialized", so we absolutely want to report the same memory usage as in the initialized case.

Comment 17

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

Attachment: Part 1: track image initialization with a WebGLImageDataStatus enum

Comment 18

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

Attachment: Part 2: reorganize the tracking of fake back statuses

(In reply to Jeff Gilbert [:jgilbert] from comment #13)
> > +    , mFakeBlackStatus(WebGLTextureFakeBlackStatus::NotNeeded)
> 
> Shouldn't this default to Unknown?

Good idea.

> Or Incomplete, since IIRC that's what textures are before they're specified?

No, see my reply below to your comment on the enum.

> > +    if (mFakeBlackStatus == WebGLTextureFakeBlackStatus::Unknown) {
> 
> Just early-out if it's not unknown.

Good idea.

> Please use ImageInfoBase() instead of ImageInfoAtFace().

OK.

> 
> @@ +369,3 @@
> >          // we have exhausted all cases where we do need fakeblack, so if the status is still unknown,
> >          // that means that we do NOT need it.
> > +        if (mFakeBlackStatus == WebGLTextureFakeBlackStatus::Unknown)
> 
> If we early-out non-unknown statuses above, this can be unconditional, and
> we can drop an unnecessary scope above.

But we don't early out on non-unknown statuses above, and I don't know that we should, so I'll leave it as-is for now.

> 
> @@ +373,3 @@
> >      }
> >  
> > +    return mFakeBlackStatus;
> 
> Then we should explicitly assert that this is non-unknown, to make it really
> obvious.

Good idea.

> 
> ::: content/canvas/src/WebGLTexture.h
> @@ +233,5 @@
> >                        GLenum aFormat, GLenum aType, WebGLImageDataStatus aStatus);
> >  
> >      void SetMinFilter(GLenum aMinFilter) {
> >          mMinFilter = aMinFilter;
> > +        SetFakeBlackStatus(WebGLTextureFakeBlackStatus::Unknown);
> 
> I think I prefer the old way, where we call a func that handles the detail
> of what to do.

I slightly prefer it that way: it's explict and doesn't require another helper function. How much do you care?

> 
> ::: content/canvas/src/WebGLTypes.h
> @@ +57,5 @@
> > +MOZ_BEGIN_ENUM_CLASS(WebGLTextureFakeBlackStatus, int)
> > +  Unknown,
> > +  NotNeeded,
> > +  NeedOpaque,
> > +  NeedTransparent
> 
> I think I'd prefer 'NeedFakeIncomplete' and 'NeedFakeUninitialized', since
> that is easier to think with.

No! Let me clarify this, as this is important to understanding more than one thing in this patch set. There is no direct mapping between Incomplete/Uninitialized, and Opaque/Transparent. It is true that Incomplete textures always need to be sampled as Opaque black. But uninitialized textures should be sampled as _either_ transparent _or_ opaque depending on whether their format has alpha. Indeed, what the spec says is they should behave as if filled with zero's. A GL_RGB texture filled with zeros is still opaque.

Comment 19

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

Attachment: Part 3: introduce transparent fake black textures alongside existing opaque onesnew-fake-black-textures

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> >          WebGLTextureFakeBlackStatus s;
> >          s = mBound2DTextures[i]
> >              ? mBound2DTextures[i]->ResolvedFakeBlackStatus()
> >              : WebGLTextureFakeBlackStatus::NotNeeded;
> 
> Assert that we're non-unknown.

OK, refreshed.

Comment 20

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

Attachment: Part 6: Clean up and optimize fake black textures implementation

Rebased.

Comment 21

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

Attachment: Part 8: Defer/avoid initialization of texture images

Rebased.

Comment 22

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

Comment on attachment 813326 [details] [diff] [review]
Part 3: introduce transparent fake black textures alongside existing opaque onesnew-fake-black-textures

carrying r+ from jgilbert.

Comment 23

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

https://tbpl.mozilla.org/?tree=Try&rev=12e8e69bf3ee

Comment 24

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

Attachment: Part 2: reorganize the tracking of fake back statuses

(In reply to Benoit Jacob [:bjacob] from comment #18)
> Created attachment 813313 [details] [diff] [review]
> Part 2: reorganize the tracking of fake back statuses
> 
> (In reply to Jeff Gilbert [:jgilbert] from comment #13)
> > > +    , mFakeBlackStatus(WebGLTextureFakeBlackStatus::NotNeeded)
> > 
> > Shouldn't this default to Unknown?
> 
> Good idea.

Actually Unknown was a bad choice, as it caused the logic in ResolvedFakeBlackStatus to be run earlier than before on the first BindTexture call (with the Part 4 patch) before the texture was properly initialized, crashing debug builds.

The right initial state is NeedOpaque, since the default state for a newly created texture is to be incomplete.

Comment 25

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

Attachment: Part 9: Fix the build on B2G (problem with MOZ_BEGIN_ENUM_CLASS)

The error we were getting is:

content/canvas/src/WebGLContextGL.cpp:261: error: operands to ?: have different types 'mozilla::WebGLTextureFakeBlackStatus' and 'mozilla::WebGLTextureFakeBlackStatus::Enum'

The problem was code using a ternary like this:

 condition ? ResolvedFakeBlackStatus() : WebGLTextureFakeBlackStatus::Foo

Ternaries want both retvals to have the same type, but in the C++98 implementation of MOZ_BEGIN_ENUM_CLASS, they haven't.

Comment 26

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

https://tbpl.mozilla.org/?tree=Try&rev=0f1fc915b6f9

Comment 27

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

This time we're green everywhere!

Comment 28

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

Review ping!

Comment 29

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #18)
> Created attachment 813313 [details] [diff] [review]
> Part 2: reorganize the tracking of fake back statuses
> 
> (In reply to Jeff Gilbert [:jgilbert] from comment #13)
> > > +    , mFakeBlackStatus(WebGLTextureFakeBlackStatus::NotNeeded)
> > 
> > Shouldn't this default to Unknown?
> 
> Good idea.
> 
> > Or Incomplete, since IIRC that's what textures are before they're specified?
> 
> No, see my reply below to your comment on the enum.
> 
> > > +    if (mFakeBlackStatus == WebGLTextureFakeBlackStatus::Unknown) {
> > 
> > Just early-out if it's not unknown.
> 
> Good idea.
> 
> > Please use ImageInfoBase() instead of ImageInfoAtFace().
> 
> OK.
> 
> > 
> > @@ +369,3 @@
> > >          // we have exhausted all cases where we do need fakeblack, so if the status is still unknown,
> > >          // that means that we do NOT need it.
> > > +        if (mFakeBlackStatus == WebGLTextureFakeBlackStatus::Unknown)
> > 
> > If we early-out non-unknown statuses above, this can be unconditional, and
> > we can drop an unnecessary scope above.
> 
> But we don't early out on non-unknown statuses above, and I don't know that
> we should, so I'll leave it as-is for now.
> 
> > 
> > @@ +373,3 @@
> > >      }
> > >  
> > > +    return mFakeBlackStatus;
> > 
> > Then we should explicitly assert that this is non-unknown, to make it really
> > obvious.
> 
> Good idea.
> 
> > 
> > ::: content/canvas/src/WebGLTexture.h
> > @@ +233,5 @@
> > >                        GLenum aFormat, GLenum aType, WebGLImageDataStatus aStatus);
> > >  
> > >      void SetMinFilter(GLenum aMinFilter) {
> > >          mMinFilter = aMinFilter;
> > > +        SetFakeBlackStatus(WebGLTextureFakeBlackStatus::Unknown);
> > 
> > I think I prefer the old way, where we call a func that handles the detail
> > of what to do.
> 
> I slightly prefer it that way: it's explict and doesn't require another
> helper function. How much do you care?
> 
> > 
> > ::: content/canvas/src/WebGLTypes.h
> > @@ +57,5 @@
> > > +MOZ_BEGIN_ENUM_CLASS(WebGLTextureFakeBlackStatus, int)
> > > +  Unknown,
> > > +  NotNeeded,
> > > +  NeedOpaque,
> > > +  NeedTransparent
> > 
> > I think I'd prefer 'NeedFakeIncomplete' and 'NeedFakeUninitialized', since
> > that is easier to think with.
> 
> No! Let me clarify this, as this is important to understanding more than one
> thing in this patch set. There is no direct mapping between
> Incomplete/Uninitialized, and Opaque/Transparent. It is true that Incomplete
> textures always need to be sampled as Opaque black. But uninitialized
> textures should be sampled as _either_ transparent _or_ opaque depending on
> whether their format has alpha. Indeed, what the spec says is they should
> behave as if filled with zero's. A GL_RGB texture filled with zeros is still
> opaque.

I understand this, but I don't think this is a distinction we should leak into the surrounding implementation. Why don't we just simply mark them as Incomplete/Uninit here, and in the central handling code later, decide whether this means opaque or transparent. This way, it's really obvious what's happening in both places: We mark it Incomp/Uninit because it's incomp/uninit, and later, we check whether it's incomp or uninit, and use the format to apply the spec language and choose the correct fake texture.

Comment 30

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 813328 [details] [diff] [review]
Part 6: Clean up and optimize fake black textures implementation

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

We shouldn't worry about handling when GenTextures or GetInteger fails, but we should init data going into them. We should probably have a helper for things like this that takes care of init, and asserts that we get something useful in debug builds.

::: content/canvas/src/WebGLContext.h
@@ +1094,5 @@
> +        gl::GLContext* mGL;
> +        GLuint mGLName;
> +
> +    public:
> +        FakeBlackTexture(gl::GLContext *gl, GLenum target, GLenum format);

Star to the left.

@@ +1099,5 @@
> +        ~FakeBlackTexture();
> +        GLuint GLName() const { return mGLName; }
> +    };
> +
> +    ScopedDeletePtr<FakeBlackTexture> mBlackOpaqueTexture2D,

This looks like it's just the MFBT version of nsAutoPtr, yes?

@@ +1104,5 @@
> +                                      mBlackOpaqueTextureCubeMap,
> +                                      mBlackTransparentTexture2D,
> +                                      mBlackTransparentTextureCubeMap;
> +
> +    void BindFakeBlackTexturesHelper(

Why don't we just pass a GLContext* and make this static in WebGLContextGL.cpp?

@@ +1106,5 @@
> +                                      mBlackTransparentTextureCubeMap;
> +
> +    void BindFakeBlackTexturesHelper(
> +        GLenum target,
> +        const nsTArray<WebGLRefPtr<WebGLTexture> > & boundTexturesArray,

What is the rationale for `foo & bar` instead of `foo& bar`?

::: content/canvas/src/WebGLContextGL.cpp
@@ +63,5 @@
> +  MOZ_ASSERT(target == LOCAL_GL_TEXTURE_2D || target == LOCAL_GL_TEXTURE_CUBE_MAP);
> +  MOZ_ASSERT(format == LOCAL_GL_RGB || format == LOCAL_GL_RGBA);
> +
> +  mGL->MakeCurrent();
> +  GLuint formerBinding;

Init me.

@@ +67,5 @@
> +  GLuint formerBinding;
> +  gl->fGetIntegerv(target == LOCAL_GL_TEXTURE_2D
> +                   ? LOCAL_GL_TEXTURE_BINDING_2D
> +                   : LOCAL_GL_TEXTURE_BINDING_CUBE_MAP,
> +                   (GLint*) &formerBinding);

We have gl->GetUInteger, also.

@@ +68,5 @@
> +  gl->fGetIntegerv(target == LOCAL_GL_TEXTURE_2D
> +                   ? LOCAL_GL_TEXTURE_BINDING_2D
> +                   : LOCAL_GL_TEXTURE_BINDING_CUBE_MAP,
> +                   (GLint*) &formerBinding);
> +  gl->fGenTextures(1, &mGLName);

Init me.

@@ +1027,5 @@
> +            continue;
> +        }
> +
> +        WebGLTextureFakeBlackStatus s
> +            = boundTexturesArray[i]->ResolvedFakeBlackStatus();

Give yourself more than 80chars, so we don't have to stoop lines so often.

Comment 31

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 813505 [details] [diff] [review]
Part 9: Fix the build on B2G (problem with MOZ_BEGIN_ENUM_CLASS)

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

Please leave a comment about this?

Comment 32

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 813329 [details] [diff] [review]
Part 8: Defer/avoid initialization of texture images

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3474,5 @@
>          }
>      }
>  
> +    if (imageInfo.HasUninitializedImageData()) {
> +        tex->DoDeferredImageInitialization(target, level);

I don't think this makes sense, does it? There are no `null` uploads for CompressedTexImage2D.

::: content/canvas/src/WebGLTexture.cpp
@@ +394,5 @@
> +            mFakeBlackStatus = WebGLTextureFakeBlackStatus::NotNeeded;
> +        } else {
> +            // The texture only contains uninitialized image data. In this case,
> +            // we can use a black texture for it.
> +            mFakeBlackStatus = FormatHasAlpha(ImageInfoAtFace(0, 0).mFormat)

s/ImageInfoAtFace(0,0)/ImageInfoBase/

@@ +441,5 @@
> +    SetImageDataStatus(imageTarget, level, WebGLImageDataStatus::InitializedImageData);
> +
> +    if (error) {
> +      // Should only be OUT_OF_MEMORY. Anyway, there's no good way to recover from this here.
> +      MOZ_CRASH(); // errors on texture upload have been related to video memory exposure in the past.

We should prefer ForceLoseContext here, shouldn't we?

Comment 33

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

Attachment: Part 2: reorganize the tracking of fake back statuses

Changed to take your proposed approach. Agree it's actually better.

Comment 34

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

Attachment: Part 6: Clean up and optimize fake black textures implementation

(In reply to Jeff Gilbert [:jgilbert] from comment #30)
> Comment on attachment 813328 [details] [diff] [review]
> Part 6: Clean up and optimize fake black textures implementation

Applied your review comments.

> > +    ScopedDeletePtr<FakeBlackTexture> mBlackOpaqueTexture2D,
> 
> This looks like it's just the MFBT version of nsAutoPtr, yes?

Exactly.

> 
> @@ +1104,5 @@
> > +                                      mBlackOpaqueTextureCubeMap,
> > +                                      mBlackTransparentTexture2D,
> > +                                      mBlackTransparentTextureCubeMap;
> > +
> > +    void BindFakeBlackTexturesHelper(
> 
> Why don't we just pass a GLContext* and make this static in
> WebGLContextGL.cpp?

Because it would still have to be declared as friend, so I'm not sure that much would be achieved by that. Don't care either way.

Comment 35

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

Attachment: Part 8: Defer/avoid initialization of texture images

(In reply to Jeff Gilbert [:jgilbert] from comment #32)
> Comment on attachment 813329 [details] [diff] [review]
> Part 8: Defer/avoid initialization of texture images
> 
> Review of attachment 813329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +3474,5 @@
> >          }
> >      }
> >  
> > +    if (imageInfo.HasUninitializedImageData()) {
> > +        tex->DoDeferredImageInitialization(target, level);
> 
> I don't think this makes sense, does it? There are no `null` uploads for
> CompressedTexImage2D.

Bah, let's keep it in case there is in the future. It doesn't hurt...

> 
> @@ +441,5 @@
> > +    SetImageDataStatus(imageTarget, level, WebGLImageDataStatus::InitializedImageData);
> > +
> > +    if (error) {
> > +      // Should only be OUT_OF_MEMORY. Anyway, there's no good way to recover from this here.
> > +      MOZ_CRASH(); // errors on texture upload have been related to video memory exposure in the past.
> 
> We should prefer ForceLoseContext here, shouldn't we?

The problem is that we would have to propagate this 'exception' to the parent calling context, which is 2 stack frames up, where we're about to go on executing the current WebGL call. This is really a use case for exceptions... OK to discuss that in a follow-up, but for now I'll rather leave it like that. But yes, I agree that ideally this is a case for losing a context.

Comment 36

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 815208 [details] [diff] [review]
Part 2: reorganize the tracking of fake back statuses

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

::: content/canvas/src/WebGLTexture.cpp
@@ +359,5 @@
> +        }
> +    }
> +    if (hasUninitializedImageData) {
> +        mFakeBlackStatus = WebGLTextureFakeBlackStatus::UninitializedImageData;
> +        return mFakeBlackStatus;

There's not much point in returning early here, but I suppose it's fine.

Comment 37

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac8abefc8dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/11edccaa9427
https://hg.mozilla.org/integration/mozilla-inbound/rev/86cc3f61e751
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce7e76613ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad866f36a4be
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f08c9622402
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7a90958681
https://hg.mozilla.org/integration/mozilla-inbound/rev/54dd7819cbde

Comment 38

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/363383856ae5

Comment 39

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/8ac8abefc8dd
https://hg.mozilla.org/mozilla-central/rev/11edccaa9427
https://hg.mozilla.org/mozilla-central/rev/86cc3f61e751
https://hg.mozilla.org/mozilla-central/rev/5ce7e76613ee
https://hg.mozilla.org/mozilla-central/rev/ad866f36a4be
https://hg.mozilla.org/mozilla-central/rev/3f08c9622402
https://hg.mozilla.org/mozilla-central/rev/4c7a90958681
https://hg.mozilla.org/mozilla-central/rev/54dd7819cbde
https://hg.mozilla.org/mozilla-central/rev/363383856ae5