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

RESOLVED FIXED

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(74 attachments)

82.42 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
48.90 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
17.91 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
34.18 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
6.55 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
29.42 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
7.51 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
5.82 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
966 bytes, patch
kamidphish
: review+
Details | Diff | Splinter Review
2.05 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
1.48 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
9.00 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
20.50 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
9.37 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
15.30 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
2.02 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
4.88 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8641841 [details] [diff] [review]
0001-So-much.-Clarified-tex-completeness-and-FakeBlack.patch
Attachment #8641841 - Flags: review?(dglastonbury)
(Assignee)

Comment 2

3 years ago
Created attachment 8641843 [details] [diff] [review]
0002-Redo-mipmap-handling.patch
Attachment #8641843 - Flags: review?(dglastonbury)
(Assignee)

Comment 3

3 years ago
Created attachment 8641844 [details] [diff] [review]
0003-Redo-FakeBlack-caching.patch
Attachment #8641844 - Flags: review?(dglastonbury)
(Assignee)

Comment 4

3 years ago
Created attachment 8641845 [details] [diff] [review]
0004-Get-it-building.patch
Attachment #8641845 - Flags: review?(dglastonbury)
(Assignee)

Comment 5

3 years ago
Created attachment 8641846 [details] [diff] [review]
0005-Fixin-bugs.patch
Attachment #8641846 - Flags: review?(dglastonbury)
(Assignee)

Comment 6

3 years ago
Created attachment 8641847 [details] [diff] [review]
0006-1.0.2-conformance.patch
Attachment #8641847 - Flags: review?(dglastonbury)
(Assignee)

Comment 7

3 years ago
Created attachment 8641848 [details] [diff] [review]
0007-1.0.3-parity.patch
Attachment #8641848 - Flags: review?(dglastonbury)
(Assignee)

Comment 8

3 years ago
Created attachment 8641849 [details] [diff] [review]
0008-Build-fixes.patch
Attachment #8641849 - Flags: review?(dglastonbury)
(Assignee)

Comment 9

3 years ago
Created attachment 8641850 [details] [diff] [review]
0009-Tex-from-video-ImageInfo-needs-effective-format.patch
Attachment #8641850 - Flags: review?(dglastonbury)
(Assignee)

Comment 10

3 years ago
Created attachment 8641851 [details] [diff] [review]
0010-Do-lazy-backbuffer-clearing-as-part-of-validation.patch
Attachment #8641851 - Flags: review?(dglastonbury)
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.
Attachment #8641843 - Flags: review?(dglastonbury) → review+
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
Attachment #8641844 - Flags: review?(dglastonbury) → review+
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.
Attachment #8641845 - Flags: review?(dglastonbury) → review+
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?
Attachment #8641846 - Flags: review?(dglastonbury) → review+
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 on attachment 8641847 [details] [diff] [review]
0006-1.0.2-conformance.patch

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

r+ with nits
Attachment #8641847 - Flags: review?(dglastonbury) → review+
Attachment #8641848 - Flags: review?(dglastonbury) → review+
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)
Attachment #8641849 - Flags: review?(dglastonbury) → review+
Attachment #8641850 - Flags: review?(dglastonbury) → review+
Attachment #8641851 - Flags: review?(dglastonbury) → review+
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?
Attachment #8641841 - Flags: review?(dglastonbury) → review+
(Assignee)

Comment 19

3 years ago
(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'.
(Assignee)

Comment 20

3 years ago
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.
(Assignee)

Comment 21

3 years ago
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.
(Assignee)

Comment 22

3 years ago
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.
(Assignee)

Comment 23

3 years ago
Created attachment 8646708 [details] [diff] [review]
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.
Attachment #8646708 - Flags: review?(dglastonbury)
(Assignee)

Comment 24

3 years ago
Created attachment 8646710 [details] [diff] [review]
0009-Do-proper-rect-subsetting-in-ANGLE-and-WebGL.patch

D3D9 seems to give a D3DERR_INVALIDCALL for out-of-bounds reads/writes here.
Attachment #8646710 - Flags: review?(dglastonbury)
(Assignee)

Comment 25

3 years ago
Created attachment 8646711 [details] [diff] [review]
0010-Switch-to-static-embedded-mImageInfoArr.patch
Attachment #8646711 - Flags: review?(dglastonbury)
(Assignee)

Comment 26

3 years ago
Created attachment 8646712 [details] [diff] [review]
0012-Make-setting-mHasUninitData-trigger-cache-invalidati.patch
Attachment #8646712 - Flags: review?(dglastonbury)
(Assignee)

Comment 27

3 years ago
Created attachment 8646714 [details] [diff] [review]
0013-s-hasUninitData-isDataInitialized.patch
Attachment #8646714 - Flags: review?(dglastonbury)
(Assignee)

Comment 28

3 years ago
Created attachment 8646715 [details] [diff] [review]
0014-Review-comments.patch
Attachment #8646715 - Flags: review?(dglastonbury)
(Assignee)

Comment 29

3 years ago
Created attachment 8646716 [details] [diff] [review]
0016-Use-IsPowerOfTwo-and-Clamp-from-MFBT-in-WebGL.patch
Attachment #8646716 - Flags: review?(dglastonbury)
(Assignee)

Updated

3 years ago
Depends on: 1193600
(Assignee)

Comment 30

3 years ago
I filed bug 1193600 to move IsPowerOfTwo and Clamp into MFBT.
Attachment #8646708 - Flags: review?(dglastonbury) → review+
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?
Attachment #8646710 - Flags: review?(dglastonbury) → review+
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)?
Attachment #8646711 - Flags: review?(dglastonbury) → review+
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.
Attachment #8646712 - Flags: review?(dglastonbury) → review+
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?
Attachment #8646714 - Flags: review?(dglastonbury) → review+
Attachment #8646715 - Flags: review?(dglastonbury) → review+
Attachment #8646716 - Flags: review?(dglastonbury) → review+
(Assignee)

Updated

3 years ago
Depends on: 1221822
(Assignee)

Updated

3 years ago
Blocks: 1221822
No longer depends on: 1221822
(Assignee)

Comment 35

3 years ago
Created 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/
(Assignee)

Comment 36

3 years ago
Created 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/
(Assignee)

Comment 37

3 years ago
Created 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/
(Assignee)

Comment 38

3 years ago
Created 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/
(Assignee)

Comment 39

3 years ago
Created 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/
(Assignee)

Comment 40

3 years ago
Created 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/
(Assignee)

Comment 41

3 years ago
Created attachment 8699708 [details]
MozReview Request: Misc fixes.

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

Comment 42

3 years ago
Created attachment 8699709 [details]
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/
(Assignee)

Comment 43

3 years ago
Created attachment 8699710 [details]
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/
(Assignee)

Comment 44

3 years ago
Created attachment 8699711 [details]
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/
(Assignee)

Comment 45

3 years ago
Created attachment 8699713 [details]
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/
(Assignee)

Comment 46

3 years ago
Created attachment 8699714 [details]
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/
(Assignee)

Comment 48

3 years ago
Created attachment 8699716 [details]
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/
(Assignee)

Comment 49

3 years ago
Created attachment 8699717 [details]
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/
(Assignee)

Comment 50

3 years ago
Created attachment 8699718 [details]
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/
(Assignee)

Comment 51

3 years ago
Created attachment 8699719 [details]
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/
(Assignee)

Comment 52

3 years ago
Created attachment 8699720 [details]
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/
(Assignee)

Comment 53

3 years ago
Created attachment 8699721 [details]
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/
(Assignee)

Comment 54

3 years ago
Created attachment 8699722 [details]
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/
(Assignee)

Comment 55

3 years ago
Created attachment 8699723 [details]
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/
(Assignee)

Comment 56

3 years ago
Created attachment 8699724 [details]
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/
(Assignee)

Comment 58

3 years ago
Created attachment 8699726 [details]
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/
(Assignee)

Comment 60

3 years ago
Created attachment 8699728 [details]
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/
(Assignee)

Comment 61

3 years ago
Created attachment 8699730 [details]
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/
(Assignee)

Comment 62

3 years ago
Created attachment 8699731 [details]
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/
(Assignee)

Comment 63

3 years ago
Created attachment 8699732 [details]
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/
(Assignee)

Comment 64

3 years ago
Created attachment 8699733 [details]
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/
(Assignee)

Comment 65

3 years ago
Created attachment 8699734 [details]
MozReview Request: Review nits.

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

Comment 66

3 years ago
Created attachment 8699735 [details]
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/
(Assignee)

Comment 68

3 years ago
Created attachment 8699737 [details]
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/
(Assignee)

Comment 69

3 years ago
Created attachment 8699738 [details]
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/
(Assignee)

Comment 70

3 years ago
Created attachment 8699739 [details]
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/
(Assignee)

Comment 71

3 years ago
Created attachment 8699740 [details]
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/
(Assignee)

Comment 72

3 years ago
Created attachment 8699741 [details]
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/
(Assignee)

Comment 73

3 years ago
Created attachment 8699742 [details]
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/
(Assignee)

Comment 74

3 years ago
Created attachment 8699743 [details]
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/
(Assignee)

Comment 75

3 years ago
Created attachment 8699744 [details]
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/
(Assignee)

Comment 76

3 years ago
Created attachment 8699745 [details]
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/
(Assignee)

Comment 77

3 years ago
Created attachment 8699746 [details]
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/
(Assignee)

Comment 78

3 years ago
Created attachment 8699747 [details]
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/
(Assignee)

Comment 79

3 years ago
Created attachment 8699748 [details]
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/
(Assignee)

Comment 80

3 years ago
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/
(Assignee)

Comment 81

3 years ago
Created attachment 8699749 [details]
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/
(Assignee)

Comment 82

3 years ago
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/
(Assignee)

Comment 83

3 years ago
Created attachment 8699750 [details]
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/
(Assignee)

Comment 84

3 years ago
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/
(Assignee)

Comment 85

3 years ago
Created attachment 8699751 [details]
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/
(Assignee)

Comment 86

3 years ago
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/
(Assignee)

Comment 87

3 years ago
Created attachment 8699752 [details]
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/
(Assignee)

Comment 88

3 years ago
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/
(Assignee)

Comment 89

3 years ago
Created attachment 8699753 [details]
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/
(Assignee)

Comment 90

3 years ago
Created attachment 8699754 [details]
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/
(Assignee)

Comment 91

3 years ago
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/
(Assignee)

Comment 92

3 years ago
Created attachment 8699755 [details]
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/
(Assignee)

Comment 93

3 years ago
Created attachment 8699756 [details]
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/
(Assignee)

Comment 94

3 years ago
Created attachment 8699757 [details]
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/
(Assignee)

Comment 95

3 years ago
Created attachment 8699758 [details]
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/
(Assignee)

Comment 96

3 years ago
Created attachment 8699759 [details]
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/
(Assignee)

Comment 97

3 years ago
Created attachment 8699760 [details]
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/
(Assignee)

Comment 98

3 years ago
This has all landed.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.