Bug 240790

<rdar://problem/94108163>

Created attachment 459976 [details]
Patch

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

Comment on attachment 459976 [details]
Patch

This looks good, but is there a way to produce a test for this? I can help upstream it to the WebGL conformance suite.

Created attachment 460022 [details]
Patch

Seems like the new patch includes the expected test result, but not the test.

Comment on attachment 460022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=460022&action=review

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_resources.mm:277
> +    if (!refOut || !refOut->get() || !refOut->get()->valid())

Here we deref the refOut already above, so `!refOut` does not make sense

Also if we have
A* a = new A;
then it does not make sense to check `if (a)`, that's redundant. The new never returns nullptr.

could we do
  ASSERT(refOut);
  TextureRef tex = new Texture(...);
  ANGLE_MTL_CHECK(context, tex->valid(), GL_OUT_OF_MEMORY);

  .. use tex

  *refOut = std::move(tex);
   return angle::Result::Continue;
}

> LayoutTests/fast/canvas/webgl/large-texture-creation.html:44
> +  assertMsg(err == gl.NO_ERROR || err == gl.OUT_OF_MEMORY,

glErrorShouldBe(gl, [gl.NO_ERROR, gl.OUT_OF_MEMORY], "unexpected result when creating MAX_TEXTURE_SIZE texture")

(we don't currently immediately see what's wrong with the failing tests we get in this patch)

Created attachment 460052 [details]
Patch

Comment on attachment 460052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=460052&action=review

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_resources.mm:306
> +    if (!(*refOut) || !(*refOut)->get() || !refOut->get()->valid())

Same issue here; if *refOut was null we’d have created on the call to reset above.

Created attachment 460053 [details]
Patch

Comment on attachment 460053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=460053&action=review

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_resources.mm:276
> +    ASSERT(refOut);

Not sure we need this assertion, but if we do, we probably need it in *both* overloads of MakeTexture.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_resources.mm:278
> +    ANGLE_MTL_CHECK(context, newTexture && newTexture->valid(), GL_OUT_OF_MEMORY);

Pretty sure that "new" never returns nullptr in C++, so the "newTexture &&" part of this is not needed.

> LayoutTests/fast/canvas/webgl/large-texture-creation.html:34
> +  gl.texImage2D(gl.TEXTURE_2D,

We changed two code paths, but with only one test case we must be covering only one of the two.

> LayoutTests/fast/canvas/webgl/large-texture-creation.html:44
> +  glErrorShouldBe(gl, [gl.NO_ERROR, gl.OUT_OF_MEMORY], "unexpected result when creating MAX_TEXTURE_SIZE texture")

Shouldn’t it always be OUT_OF_MEMORY? Why is NO_ERROR a possibility?

(In reply to Darin Adler from comment #11)
> Comment on attachment 460053 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=460053&action=review
> 
> > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_resources.mm:276
> > +    ASSERT(refOut);
> 
> Not sure we need this assertion, but if we do, we probably need it in *both*
> overloads of MakeTexture.

I'll update this before landing.
> 
> > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_resources.mm:278
> > +    ANGLE_MTL_CHECK(context, newTexture && newTexture->valid(), GL_OUT_OF_MEMORY);
> 
> Pretty sure that "new" never returns nullptr in C++, so the "newTexture &&"
> part of this is not needed.
Double checked this, I believe you're right. We'd get an exception instead.
> 
> > LayoutTests/fast/canvas/webgl/large-texture-creation.html:34
> > +  gl.texImage2D(gl.TEXTURE_2D,
> 
> We changed two code paths, but with only one test case we must be covering
> only one of the two.

I don't have a good way to test the IOSurface binding API from just a webgl script, I suppose I could try to create a max-sized canvas object? 

> > LayoutTests/fast/canvas/webgl/large-texture-creation.html:44
> > +  glErrorShouldBe(gl, [gl.NO_ERROR, gl.OUT_OF_MEMORY], "unexpected result when creating MAX_TEXTURE_SIZE texture")
> 
> Shouldn’t it always be OUT_OF_MEMORY? Why is NO_ERROR a possibility?

I kept no_error a possibility because some devices, especially those with larger GPUS, can actually allocate a max-sized F32 texture. a Mac pro with an AMD card, for instance, handles this fine as far as I could tell from a local build.

Created attachment 460103 [details]
Patch

kpiddington@apple.com does not have reviewer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json.

Rejecting attachment 460103 [details] from commit queue.

Committed r295429 (251435@main): <https://commits.webkit.org/251435@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 460103 [details].