1189917 - Refactor WebGLTexture::ImageInfo handling, mipmaps, and fake-black

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Existing mipmap and fake-black handling involve some optimizations which don't work with WebGL2.

Let's refactor them out and clarify a bunch of code.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-So-much.-Clarified-tex-completeness-and-FakeBlack.patch

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Redo-mipmap-handling.patch

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: 0003-Redo-FakeBlack-caching.patch

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: 0004-Get-it-building.patch

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: 0005-Fixin-bugs.patch

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: 0006-1.0.2-conformance.patch

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: 0007-1.0.3-parity.patch

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: 0008-Build-fixes.patch

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: 0009-Tex-from-video-ImageInfo-needs-effective-format.patch

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: 0010-Do-lazy-backbuffer-clearing-as-part-of-validation.patch

Comment 11

[u480271]

Comment on attachment 8641843 [details] [diff] [review]
0002-Redo-mipmap-handling.patch

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

r+ with nits.

::: dom/canvas/WebGL2ContextFramebuffers.cpp
@@ +119,3 @@
>  
>      if (fb->ColorAttachment(0).IsDefined()) {
>          const auto& attachement = fb->ColorAttachment(0);

I guess that should be `attachment` instead of `attachement`

::: dom/canvas/WebGLContextValidate.cpp
@@ +682,5 @@
>      return false;
>  }
>  
> +static inline bool
> +IsPOTAssumingNonnegative(GLsizei x)

There was another function with the exact same code in the previous patch.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +75,5 @@
>  
>  bool
>  WebGLFBAttachPoint::IsReadableFloat() const
>  {
> +    TexInternalFormat internalformat = Format();

I'm not longer a fan of `StrongGLTypes` and would prefer they be replaced with format tables.

@@ +588,5 @@
>      return hasIncomplete;
>  }
>  
> +static bool
> +MatchOrReplaceSize(const WebGLFBAttachPoint& cur, uint32_t* const out_width,

out_width and out_height are read from. Should it be asserted they are not null pointers?

::: dom/canvas/WebGLTexture.h
@@ -28,5 @@
>  } // namespace dom
>  
>  // Zero is not an integer power of two.
> -inline bool
> -IsPOTAssumingNonnegative(GLsizei x)

This version seems redundant. It correctly handles that the case were x is 0. Remove it.

@@ -30,5 @@
>  // Zero is not an integer power of two.
> -inline bool
> -IsPOTAssumingNonnegative(GLsizei x)
> -{
> -    MOZ_ASSERT(x >= 0);

x == 0 is handled. Why assert?

@@ +314,4 @@
>      public:
>          size_t MaxMipmapLevels() const {
>              // GLES 3.0.4, 3.8 - Mipmapping: `floor(log2(largest_of_dims)) + 1`
> +            uint32_t largest = std::max(std::max(mWidth, mHeight), mDepth);

Shouldn't we use GLuint instead of uint32_t? Function still return size_t; that should be changed too.

Comment 12

[u480271]

Comment on attachment 8641844 [details] [diff] [review]
0003-Redo-FakeBlack-caching.patch

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

::: dom/canvas/WebGLTexture.cpp
@@ +808,4 @@
>              break;
>          }
>  
> +        InvalidateFakeBlackCache();

Better

Comment 13

[u480271]

Comment on attachment 8641845 [details] [diff] [review]
0004-Get-it-building.patch

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

::: dom/canvas/WebGLContextGL.cpp
@@ +1524,4 @@
>          // Zero the whole pixel dest area in the destination buffer.
>          memset(data, 0, checked_neededByteLength.value());
>  
> +        if (   x >= int32_t(srcWidth)

Can't this be kept unsigned, since -ve values are excluded as invalid.

Comment 14

[u480271]

Comment on attachment 8641846 [details] [diff] [review]
0005-Fixin-bugs.patch

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

::: dom/canvas/WebGLTextureUpload.cpp
@@ +243,5 @@
> +#error The spec doesn't say to only use UNSIGNED_BYTE, except in that that's what ReadPixels does.
> +#error Is that enough?
> +#error Likely, ensure (in ANGLE?) that CopyTexImage from RGBA16F is supposed to be 16F.
> +#error I suspect that we should derive the effective internal format resulting from CopyTexImage
> +#error from the default packFormat/packType for ReadPixels for the given FB effIntFormat.

Does this even compile?

Comment 15

[u480271]

Comment on attachment 8641847 [details] [diff] [review]
0006-1.0.2-conformance.patch

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

r+ with nits

::: dom/canvas/WebGLTexture.cpp
@@ +37,5 @@
> +{
> +    Mutable(mFormat) = a.mFormat;
> +    Mutable(mWidth) = a.mWidth;
> +    Mutable(mHeight) = a.mHeight;
> +    Mutable(mDepth) = a.mDepth;

o_O you like making life hard with const.

@@ +55,5 @@
> +    const auto pair = mAttachPoints.insert(attachPoint);
> +    const bool& didInsert = pair.second;
> +
> +    MOZ_ASSERT(didInsert);
> +    mozilla::unused << didInsert;

DebugOnly on didInsert?

@@ +115,4 @@
>  void
>  WebGLTexture::Delete()
>  {
> +    mImageInfoMap.clear(); // Invalidate all ImageInfos.

Does this comment add anymore clarity?

@@ -62,5 @@
> -{
> -    if (!IsDefined())
> -        return 0;
> -
> -    size_t bitsPerTexel = GetBitsPerTexel(mFormat);

Why do we return Bits when we almost always go and convert into bytes as the next operation?

@@ +620,4 @@
>                                         imageInfo.mWidth);
>          if (cleared) {
>              imageInfo.mHasUninitData = false;
> +            InvalidateFakeBlackCache();

these two lines appear together further down. Does that the operations "joined"? Does that mean these two lines a good candidate for a function?

::: dom/canvas/WebGLTextureUpload.cpp
@@ +175,4 @@
>                                  height == levelInfo.mHeight;
>          if (coversWholeImage) {
>              levelInfo.mHasUninitData = false;
> +            InvalidateFakeBlackCache();

More of this:

    levelInfo.mHasUninitData = false;
    InvalidateFakeBlackCache();

pattern. I definitely think I would be a good idea to implement it as a function so you only have to update in one place.

@@ +191,4 @@
>  WebGLTexture::CopyTexSubImage2D_base(TexImageTarget texImageTarget, GLint level,
>                                       GLenum rawInternalFormat,
>                                       GLint xOffset, GLint yOffset, GLint x,
> +                                     GLint y, GLsizei width, GLsizei height, GLint border,

Why pass a border when it has to be 0?

@@ +423,4 @@
>                                  height == texHeight;
>          if (coversWholeImage) {
>              imageInfo.mHasUninitData = false;
> +            InvalidateFakeBlackCache();

And again.

@@ +814,4 @@
>                                  height == imageInfo.mHeight;
>          if (coversWholeImage) {
>              imageInfo.mHasUninitData = false;
> +            InvalidateFakeBlackCache();

And again.

@@ +1424,4 @@
>                                  depth == imageInfo.mDepth;
>          if (coversWholeImage) {
>              imageInfo.mHasUninitData = false;
> +            InvalidateFakeBlackCache();

And again.

Comment 16

[u480271]

Comment on attachment 8641847 [details] [diff] [review]
0006-1.0.2-conformance.patch

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

r+ with nits

Comment 17

[u480271]

Comment on attachment 8641849 [details] [diff] [review]
0008-Build-fixes.patch

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

::: dom/canvas/WebGLTextureUpload.cpp
@@ +212,5 @@
>      if (levelInfo.mHasUninitData) {
>          bool coversWholeImage = xOffset == 0 &&
>                                  yOffset == 0 &&
> +                                uint32_t(width) == levelInfo.mWidth &&
> +                                uint32_t(height) == levelInfo.mHeight;

Shouldn't these be signed or unsigned, and preferably of what ever type GL specifies them as (I guess that is GLint or GLsizei)

Comment 18

[u480271]

Comment on attachment 8641841 [details] [diff] [review]
0001-So-much.-Clarified-tex-completeness-and-FakeBlack.patch

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

r+ begrudgingly. I wanted to r- because of mImageInfoMap. I feel that's completely the wrong data structure to use, especially since it's indexed by `level` like an array. Switch to std::vector to get the helpers and C++11 for behaviour.

::: dom/canvas/WebGLContextDraw.cpp
@@ +690,4 @@
>              continue;
>          }
>  
> +        const auto& imageInfo = boundTexturesArray[i]->BaseImageInfo();

On dev-platform, Wasn't this one of the case that is was cautioned against using auto?

@@ +694,2 @@
>          bool alpha = s == WebGLTextureFakeBlackStatus::UninitializedImageData &&
> +                     FormatHasAlpha(imageInfo.mFormat);

Really prefer () to separate the boolean expression from assignment.  [...] = s == [...] looks weird.

::: dom/canvas/WebGLContextTextures.cpp
@@ +206,5 @@
>  
>      MakeContextCurrent();
>  
> +    if (newTex) {
> +        if (!newTex->BindTexture(texTarget))

Nested `if` of ugly+1

::: dom/canvas/WebGLFramebuffer.cpp
@@ +40,5 @@
>  WebGLFBAttachPoint::IsDefined() const
>  {
> +    return (Renderbuffer() && Renderbuffer()->IsDefined()) ||
> +           (Texture() && Texture()->ImageInfoAt(mTexImageTarget,
> +                                                mTexImageLevel).IsDefined());

:thumbsup

Pretty sure this is what I need for FramebufferTextureLayer checking.

::: dom/canvas/WebGLTexture.cpp
@@ +490,3 @@
>          return true;
>  
>      mContext->MakeContextCurrent();

Is this required here? Should it be called by ClearWithTempFB?

@@ +543,2 @@
>  
>      mContext->GetAndFlushUnderlyingGLErrors();

Or call mContext->MakeContextCurrent() here, just before the first real GL call.

@@ +589,5 @@
>  }
>  
> +template<typename T>
> +static T
> +Clamp(T val, T min, T max) {

Seems like something that might be suited to mfbt.

@@ +604,5 @@
> +    // "For immutable-format textures, `level_base` is clamped to the range
> +    //  `[0, levels-1]`, `level_max` is then clamped to the range `
> +    //  `[level_base, levels-1]`, where `levels` is the parameter passed to
> +    //   TexStorage* for the texture object."
> +    mBaseMipmapLevel = Clamp(mBaseMipmapLevel, size_t(0), mImmutableLevelCount - 1);

Should this be a function that returns a value for the clamped levels, instead of modifying state?
Or is state cached values anyway?

@@ +605,5 @@
> +    //  `[0, levels-1]`, `level_max` is then clamped to the range `
> +    //  `[level_base, levels-1]`, where `levels` is the parameter passed to
> +    //   TexStorage* for the texture object."
> +    mBaseMipmapLevel = Clamp(mBaseMipmapLevel, size_t(0), mImmutableLevelCount - 1);
> +    mMaxMipmapLevel = Clamp(mMaxMipmapLevel, mBaseMipmapLevel,

That doesn't seem right to me. Maybe it's changed in a following patch.

@@ +620,5 @@
>      if (IsDeleted())
>          return false;
>  
> +    const bool isFirstBinding = !HasEverBeenBound();
> +    if (!isFirstBinding && mTarget != texTarget) {

double negative. IMO, isFirstBinding is a poor name. But I see below that it's used for first binding. Meh, much of a muchness.

@@ +655,4 @@
>  void
>  WebGLTexture::GenerateMipmap(TexTarget texTarget)
>  {
> +    if (mBaseMipmapLevel > mMaxMipmapLevel) {

Stylistically, I think it would be nice to have these checks in a separate validate function.

::: dom/canvas/WebGLTexture.h
@@ +35,5 @@
>      return x && (x & (x-1)) == 0;
>  }
>  
> +inline bool
> +IsPowerOfTwo(size_t x)

Another good candidate for mfbt?

@@ +64,1 @@
>  public:

blank line before public:

@@ +73,5 @@
> +    TexTarget mTarget;
> +    uint8_t mFaceCount; // 6 for cube maps, 1 otherwise.
> +    static const uint8_t kMaxFaceCount = 6;
> +
> +    std::map<size_t, Face> mImageInfoMap;

Why a map and not an array?

@@ +286,2 @@
>              , mDepth(0)
> +            , mHasUninitData(true)

, mHasInitializedData(false)??? Or is Initalized Data vs Uninitialized Data not an strict `not` relationship? (ImageInfo is for one texture image, so it's either going to be initialized or not, right?)

@@ +384,5 @@
> +
> +    ImageInfo& ImageInfoAtFace(uint8_t face, size_t level) {
> +        MOZ_ASSERT(face < mFaceCount);
> +
> +        auto itr = mImageInfoMap.find(level);

Why use a map when you can use an array?

Comment 19

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #18)
> Comment on attachment 8641841 [details] [diff] [review]
> 0001-So-much.-Clarified-tex-completeness-and-FakeBlack.patch
> 
> Review of attachment 8641841 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ begrudgingly. I wanted to r- because of mImageInfoMap. I feel that's
> completely the wrong data structure to use, especially since it's indexed by
> `level` like an array. Switch to std::vector to get the helpers and C++11
> for behaviour.

We talked on IRC, but I'll post it here too.

Both level=0 and level=INT32_MAX are valid[1], so we want a sparse array. `unordered_map` is probably better, but since we're generally dealing with N=10 or so, log(N) isn't that big of a deal, particularly since we cache the resulting status.

[1] It's valid to initialize level INT32_MAX, and set `level_base` and `level_max` to INT32_MAX, and get normal rendering. I fearfully anticipate driver bugs in this area. (OOM, or generally large allocations from array-length-ensure implementations)

> 
> ::: dom/canvas/WebGLContextDraw.cpp
> @@ +690,4 @@
> >              continue;
> >          }
> >  
> > +        const auto& imageInfo = boundTexturesArray[i]->BaseImageInfo();
> 
> On dev-platform, Wasn't this one of the case that is was cautioned against
> using auto?

I don't think so, why? Eliding the type doesn't seem to give us less info. Specification of the type would make this more fragile to changes in the called function, but changes to the called function often need the callsites re-inspected anyway.

> 
> @@ +694,2 @@
> >          bool alpha = s == WebGLTextureFakeBlackStatus::UninitializedImageData &&
> > +                     FormatHasAlpha(imageInfo.mFormat);
> 
> Really prefer () to separate the boolean expression from assignment.  [...]
> = s == [...] looks weird.

Agreed.

> 
> ::: dom/canvas/WebGLContextTextures.cpp
> @@ +206,5 @@
> >  
> >      MakeContextCurrent();
> >  
> > +    if (newTex) {
> > +        if (!newTex->BindTexture(texTarget))
> 
> Nested `if` of ugly+1

Without this format, you get:
if (newTex && !newTex->BindTexture(texTarget)
    return;
else if (!newTex) {...}

Conceptually, the split is warranted: The first branch is "depending on whether newTex is null, we want to take two different paths." The second is "Since newTex is non-null, if we call BindTexture() and it fails, return early".

> ::: dom/canvas/WebGLTexture.cpp
> @@ +490,3 @@
> >          return true;
> >  
> >      mContext->MakeContextCurrent();
> 
> Is this required here? Should it be called by ClearWithTempFB?

It should probably be in the _base function, instead of spread around to be called multiple times.

> @@ +589,5 @@
> >  }
> >  
> > +template<typename T>
> > +static T
> > +Clamp(T val, T min, T max) {
> 
> Seems like something that might be suited to mfbt.

Yes, but I'm trying to avoid prereqs here.

> @@ +604,5 @@
> > +    // "For immutable-format textures, `level_base` is clamped to the range
> > +    //  `[0, levels-1]`, `level_max` is then clamped to the range `
> > +    //  `[level_base, levels-1]`, where `levels` is the parameter passed to
> > +    //   TexStorage* for the texture object."
> > +    mBaseMipmapLevel = Clamp(mBaseMipmapLevel, size_t(0), mImmutableLevelCount - 1);
> 
> Should this be a function that returns a value for the clamped levels,
> instead of modifying state?
> Or is state cached values anyway?

I think the spec has the internal values clamped.

GLES 3.0.4, p158:
"The values of `level_base` and `level_max` may be respecified for a specific texture by calling TexParameter[if] with `pname` set to TEXTURE_BASE_LEVEL or TEXTURE_MAX_LEVEL respectively."
Above that, it has the text reproduced in the comment. Below *that* is:
"Otherwise `p = floor(log2(maxsize)) + level_base`, and all arrays from `level_base` through `q = min{p, level_max}` must be defined[.]"
If they weren't actually clamped, I would expect the text to give equations specifying `p` from the clamped values, instead of clamping the values themselves.

> @@ +605,5 @@
> > +    //  `[0, levels-1]`, `level_max` is then clamped to the range `
> > +    //  `[level_base, levels-1]`, where `levels` is the parameter passed to
> > +    //   TexStorage* for the texture object."
> > +    mBaseMipmapLevel = Clamp(mBaseMipmapLevel, size_t(0), mImmutableLevelCount - 1);
> > +    mMaxMipmapLevel = Clamp(mMaxMipmapLevel, mBaseMipmapLevel,
> 
> That doesn't seem right to me. Maybe it's changed in a following patch.

This is not changed in a following patch.

> @@ +620,5 @@
> >      if (IsDeleted())
> >          return false;
> >  
> > +    const bool isFirstBinding = !HasEverBeenBound();
> > +    if (!isFirstBinding && mTarget != texTarget) {
> 
> double negative. IMO, isFirstBinding is a poor name. But I see below that
> it's used for first binding. Meh, much of a muchness.

We can probably just use HasEverBeenBound().

> @@ +655,4 @@
> >  void
> >  WebGLTexture::GenerateMipmap(TexTarget texTarget)
> >  {
> > +    if (mBaseMipmapLevel > mMaxMipmapLevel) {
> 
> Stylistically, I think it would be nice to have these checks in a separate
> validate function.

It's only ever tested twice: Once in GenerateMipmap and once in IsMipmapComplete. It's simple enough that I think it may be harder to read if we pull it out into a different function.

> ::: dom/canvas/WebGLTexture.h
> @@ +35,5 @@
> >      return x && (x & (x-1)) == 0;
> >  }
> >  
> > +inline bool
> > +IsPowerOfTwo(size_t x)
> 
> Another good candidate for mfbt?

Yes.

> @@ +64,1 @@
> >  public:
> 
> blank line before public:

When interleaving public and protected sections of the member declarations, in trying to keep related member decls together, I'll not leave a blank line before the access qualifier (re)specification.

> 
> @@ +286,2 @@
> >              , mDepth(0)
> > +            , mHasUninitData(true)
> 
> , mHasInitializedData(false)??? Or is Initalized Data vs Uninitialized Data
> not an strict `not` relationship? (ImageInfo is for one texture image, so
> it's either going to be initialized or not, right?)

It could be `mIsDataInitialized` for TexImages. In WebGL, it's mutually exclusive for TexImages, since we either are completely uninitialized and using fake-black, fully-initially-initialized, or ensure-initialized into being fully-initialized.

However, for the whole mip-chain, it's not mutually-exclusive. Some levels may be initialized, and some may not. Thus these textures 'have uninitialized data', even though they also 'have initialized data'.

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8641847 [details] [diff] [review]
0006-1.0.2-conformance.patch

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

::: dom/canvas/WebGLTexture.cpp
@@ +620,4 @@
>                                         imageInfo.mWidth);
>          if (cleared) {
>              imageInfo.mHasUninitData = false;
> +            InvalidateFakeBlackCache();

InvalidateFakeBlackCache() is on its own most of the time, but perhaps, yeah. Generally when we change imageInfo.mHasUninitData, we need to invalidate the caches.

::: dom/canvas/WebGLTextureUpload.cpp
@@ +191,4 @@
>  WebGLTexture::CopyTexSubImage2D_base(TexImageTarget texImageTarget, GLint level,
>                                       GLenum rawInternalFormat,
>                                       GLint xOffset, GLint yOffset, GLint x,
> +                                     GLint y, GLsizei width, GLsizei height, GLint border,

It keeps the validation in the same place.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8641843 [details] [diff] [review]
0002-Redo-mipmap-handling.patch

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

::: dom/canvas/WebGLContextValidate.cpp
@@ +682,5 @@
>      return false;
>  }
>  
> +static inline bool
> +IsPOTAssumingNonnegative(GLsizei x)

I'm going to dump this in MFBT.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +75,5 @@
>  
>  bool
>  WebGLFBAttachPoint::IsReadableFloat() const
>  {
> +    TexInternalFormat internalformat = Format();

Me too, but we're not there yet!

::: dom/canvas/WebGLTexture.h
@@ +314,4 @@
>      public:
>          size_t MaxMipmapLevels() const {
>              // GLES 3.0.4, 3.8 - Mipmapping: `floor(log2(largest_of_dims)) + 1`
> +            uint32_t largest = std::max(std::max(mWidth, mHeight), mDepth);

In one of the later patches I convert everything to uint32_t instead of size_t.

I think uint32_t is better than GLsizei, as outside of validation, you can ignore whether GLsizei is unsigned or not (it's signed). It's one less thing to remember.

Comment 22

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8641845 [details] [diff] [review]
0004-Get-it-building.patch

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

::: dom/canvas/WebGLContextGL.cpp
@@ +1524,4 @@
>          // Zero the whole pixel dest area in the destination buffer.
>          memset(data, 0, checked_neededByteLength.value());
>  
> +        if (   x >= int32_t(srcWidth)

No, because they aren't. Negative x (and y) is valid here.

Comment 23

[Kelsey Gilbert [:jgilbert]]

Attachment: 0008-Don-t-pass-insane-CopyTexImage-x-and-y-to-the-driver.patch

This and the next patch fix an issue on D3D9 both locally an on Try.

Comment 24

[Kelsey Gilbert [:jgilbert]]

Attachment: 0009-Do-proper-rect-subsetting-in-ANGLE-and-WebGL.patch

D3D9 seems to give a D3DERR_INVALIDCALL for out-of-bounds reads/writes here.

Comment 25

[Kelsey Gilbert [:jgilbert]]

Attachment: 0010-Switch-to-static-embedded-mImageInfoArr.patch

Comment 26

[Kelsey Gilbert [:jgilbert]]

Attachment: 0012-Make-setting-mHasUninitData-trigger-cache-invalidati.patch

Comment 27

[Kelsey Gilbert [:jgilbert]]

Attachment: 0013-s-hasUninitData-isDataInitialized.patch

Comment 28

[Kelsey Gilbert [:jgilbert]]

Attachment: 0014-Review-comments.patch

Comment 29

[Kelsey Gilbert [:jgilbert]]

Attachment: 0016-Use-IsPowerOfTwo-and-Clamp-from-MFBT-in-WebGL.patch

Comment 30

[Kelsey Gilbert [:jgilbert]]

I filed bug 1193600 to move IsPowerOfTwo and Clamp into MFBT.

Comment 31

[u480271]

Comment on attachment 8646710 [details] [diff] [review]
0009-Do-proper-rect-subsetting-in-ANGLE-and-WebGL.patch

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

r+ with nit about IntersectRect

::: dom/canvas/WebGLTexture.cpp
@@ +267,4 @@
>  
>      // ALPHA *is* a "color format"!
>      return unsizedformat != LOCAL_GL_DEPTH_COMPONENT &&
> +           unsizedformat != LOCAL_GL_DEPTH_STENCIL &&

Yeah, nice one.

::: dom/canvas/WebGLTextureUpload.cpp
@@ +364,5 @@
> +        GLsizei trimmedWidth = width;
> +        GLsizei trimmedHeight = height;
> +
> +        if (x < 0) {
> +            GLint diff = 0 - x;

is 0 - x more important than just -x?

@@ +366,5 @@
> +
> +        if (x < 0) {
> +            GLint diff = 0 - x;
> +            MOZ_ASSERT(diff > 0);
> +            trimmedX += diff;

So trimmedX = 0?

::: gfx/angle/src/libGLESv2/renderer/d3d/d3d9/Blit9.cpp
@@ +544,5 @@
>  
> +    D3DSURFACE_DESC destDesc;
> +    textureSurface->GetDesc(&destDesc);
> +
> +    RECT subsetSourceRect = sourceRect;

Is this code calculating the intersection of sourceRect and destRect? Can't this code be replaced with IntersectRect?

@@ +592,5 @@
>      SafeRelease(textureSurface);
>  
>      if (FAILED(result))
>      {
> +        if (!(result == D3DERR_OUTOFVIDEOMEMORY || result == E_OUTOFMEMORY)) {

Is this debugging code, or should it be kept?

Comment 32

[u480271]

Comment on attachment 8646711 [details] [diff] [review]
0010-Switch-to-static-embedded-mImageInfoArr.patch

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

::: dom/canvas/WebGLTexture.cpp
@@ +38,5 @@
> +        return;
> +
> +    OnRespecify();
> +
> +    Mutable(mFormat) = LOCAL_GL_NONE;

I feel the XXX(mFormat) should sound like a verb. Mutate(mFormat)?

Comment 33

[u480271]

Comment on attachment 8646712 [details] [diff] [review]
0012-Make-setting-mHasUninitData-trigger-cache-invalidati.patch

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

::: dom/canvas/WebGLFramebuffer.cpp
@@ +139,5 @@
>      if (!HasImage())
>          return false;
>  
> +    if (mRenderbufferPtr
> +        return mRenderbufferPtr)->HasUninitializedImageData();

That doesn't look right.

Comment 34

[u480271]

Comment on attachment 8646714 [details] [diff] [review]
0013-s-hasUninitData-isDataInitialized.patch

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

::: dom/canvas/WebGLFramebuffer.cpp
@@ +139,5 @@
>      if (!HasImage())
>          return false;
>  
> +    if (mRenderbufferPtr)
> +        return mRenderbufferPtr->HasUninitializedImageData();

Aah, fixed it. Squash into previous patch before landing?

Comment 35

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Bug 1189917 - Refactor ImageInfo handling, mipmaps, fake-black. -r=kamidphish

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

Comment 36

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Pull in aggressive refactor from tex-format-tables.

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

Comment 37

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Optimize good FB status instead of bad, and mark depth24 renderable.

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

Comment 38

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Add 2D_ARRAY textures and fix FormatAuthority startup for WebGL2.

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

Comment 39

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Misc from initial review vidyo.

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

Comment 40

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: WebGL2 (Get)TexParam, and more TEX_2D_ARRAY. Also no 'required' format exts for WebGL2.

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

Comment 41

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Misc fixes.

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

Comment 42

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix local conformance tests.

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

Comment 43

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Compile on Linux Clang 3.6.2 and GCC 4.8.

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

Comment 44

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Check that target is valid for formats.

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

Comment 45

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fixes from Try.

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

Comment 46

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Try to fix static-analysis errors re:lambdas and refcounteds.

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

Comment 47

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Touchup.

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

Comment 48

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Implement swizzling for L/A/LA.

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

Comment 49

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix ScopedCopyTex* and fix build error on Mac.

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

Comment 50

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: 1.0.4 parity

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

Comment 51

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: More fixes from review.

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

Comment 52

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Don't require draw_buffers as a native ext for WebGL2.

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

Comment 53

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: No INCOMPLETE_DIMENSIONS in WebGL 2.

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

Comment 54

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix conformance2/renderbuffers/invalidate-framebuffer.

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

Comment 55

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Remove 'native extensions' from WebGL 2.

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

Comment 56

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix build issues.

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

Comment 57

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Try fixes.

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

Comment 58

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix conformance2/textures/tex-mipmap-levels.

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

Comment 59

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Try fixes.

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

Comment 60

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix conformance/textures/misc/tex-storage-2d.

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

Comment 61

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix conformance2/textures/misc/tex-storage-compressed-formats.

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

Comment 62

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix conformance/textures/misc/texture-npot.

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

Comment 63

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Mark L/A/LA as filterable for WebGL 2.

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

Comment 64

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Support webgl2 tex format tests.

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

Comment 65

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Review nits.

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

Comment 66

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix GetFBAttach behavior for null attachments.

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

Comment 67

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix sRGB.

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

Comment 68

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix tex-(half-)float exts for WebGL1 on !GLES.

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

Comment 69

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Isolate OSX assert.

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

Comment 70

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix OSX IsCurrent issue.

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

Comment 71

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix OSX MakeCurrent and fix obj-del-behav test.

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

Comment 72

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix refcounted lambda captures, and fix implicit_color_buffer_float.

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

Comment 73

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Add more spew and try to fix implicit color-buffer-half-float.

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

Comment 74

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix FB completeness for WebGL2.

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

Comment 75

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Allow SurfaceFromElement to return the Image.

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

Comment 76

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix fast video uploads.

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

Comment 77

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Compressed texture formats should be filterable.

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

Comment 78

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Add pref for using unsized float/half-float formats with WebGL2.

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

Comment 79

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Build on Linux.

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

Comment 80

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8699702 [details]
MozReview Request: Bug 1189917 - Refactor ImageInfo handling, mipmaps, fake-black. -r=kamidphish

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

Comment 81

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Update comment for SFEResult.

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

Comment 82

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8699703 [details]
MozReview Request: Pull in aggressive refactor from tex-format-tables.

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

Comment 83

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix ordering of SFEResult checking. (CORS/write-only)

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

Comment 84

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8699704 [details]
MozReview Request: Optimize good FB status instead of bad, and mark depth24 renderable.

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

Comment 85

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Update tests based on feedback from WG.

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

Comment 86

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8699705 [details]
MozReview Request: Add 2D_ARRAY textures and fix FormatAuthority startup for WebGL2.

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

Comment 87

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Clarify some WebGLFormat funcs and support INVALID_ENUM checking.

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

Comment 88

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8699706 [details]
MozReview Request: Misc from initial review vidyo.

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

Comment 89

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Skip web-platform test compressedTexSubImage2D until it works

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

Comment 90

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix format code and add more error checking to the sRGB test.

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

Comment 91

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8699707 [details]
MozReview Request: WebGL2 (Get)TexParam, and more TEX_2D_ARRAY. Also no 'required' format exts for WebGL2.

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

Comment 92

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Address review comments.

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

Comment 93

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Remove unused function.

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

Comment 94

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix build error.

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

Comment 95

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Check for swizzle if we need it.

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

Comment 96

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Fix WebGLFormats and simplify L/A/LA test.

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

Comment 97

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Don't ask the driver about FB attachment color encoding.

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

Comment 98

[Kelsey Gilbert [:jgilbert]]

This has all landed.