Bug 240790 – ANGLE: ERR: TextureMtl.mm:108 (GetLayerMipIndex): \t! Unreachable reached: GetLayerMipIndex(.../TextureMtl.mm:108)

• Status: RESOLVED
• Resolution: FIXED
• Component: ANGLE

[Kimmo Kinnunen]

ANGLE: ERR: TextureMtl.mm:108 (GetLayerMipIndex): \t! Unreachable reached: GetLayerMipIndex(.../TextureMtl.mm:108)


0   libANGLE-shared.dylib         	       0x106bd35cc gl::LogMessage::~LogMessage() + 292 (debug.cpp:206)
1   libANGLE-shared.dylib         	       0x106bd35bc gl::LogMessage::~LogMessage() + 276 (debug.cpp:200)
2   libANGLE-shared.dylib         	       0x106bd2f10 gl::LogMessage::~LogMessage() + 32 (debug.cpp:182)
3   libANGLE-shared.dylib         	       0x1072aff78 rx::(anonymous namespace)::GetLayerMipIndex(std::__1::shared_ptr<rx::mtl::Texture> const&, unsigned int, unsigned int) + 528 (TextureMtl.mm:108)
4   libANGLE-shared.dylib         	       0x1072afd20 rx::TextureMtl::initializeContents(gl::Context const*, unsigned int, gl::ImageIndex const&) + 1208 (TextureMtl.mm:2072)
5   libANGLE-shared.dylib         	       0x106dcc4b4 gl::FramebufferAttachmentObject::initializeContents(gl::Context const*, unsigned int, gl::ImageIndex const&) + 696 (FramebufferAttachment.cpp:346)
6   libANGLE-shared.dylib         	       0x10728cd80 gl::Texture::ensureSubImageInitialized(gl::Context const*, gl::ImageIndex const&, gl::Box const&) + 96 (Texture.cpp:2326)
7   libANGLE-shared.dylib         	       0x10728cc10 gl::Texture::setSubImage(gl::Context*, gl::PixelUnpackState const&, gl::Buffer*, gl::TextureTarget, int, gl::Box const&, unsigned int, unsigned int, unsigned char const*) + 412 (Texture.cpp:1274)
8   libANGLE-shared.dylib         	       0x106b3373c gl::Context::texSubImage2D(gl::TextureTarget, int, int, int, int, int, unsigned int, unsigned int, void const*) + 304 (Context.cpp:5132)
9   libANGLE-shared.dylib         	       0x106b33844 gl::Context::texSubImage2DRobust(gl::TextureTarget, int, int, int, int, int, unsigned int, unsigned int, int, void const*) + 128 (Context.cpp:5148)
10  libANGLE-shared.dylib         	       0x106cc5d18 GL_TexSubImage2DRobustANGLE + 316 (entry_points_gles_ext_autogen.cpp:2091)
11  WebCore                       	       0x14aaf6e00 WebCore::GraphicsContextGLANGLE::texSubImage2D(unsigned int, int, int, int, int, int, unsigned int, unsigned int, GCGLSpan<void const, 18446744073709551615ul>) + 152 (GraphicsContextGLANGLE.cpp:406)
12  WebKit                        	       0x11871e738 WebKit::RemoteGraphicsContextGL::texSubImage2D0(unsigned int, int, int, int, int, int, unsigned int, unsigned int, WTF::Span<unsigned char const, 18446744073709551615ul>&&) + 272 (RemoteGraphicsContextGLFunctionsGenerated.h:794)
13  WebKit                        	       0x1188a0c38 void IPC::callMemberFunctionImpl<WebKit::RemoteGraphicsContextGL, void (WebKit::RemoteGraphicsContextGL::*)(unsigned int, int, int, int, int, int, unsigned int, unsigned int, WTF::Span<unsigned char const, 18446744073709551615ul>&&), std::__1::tuple<unsigned int, int, int, int, int, int, unsigned int, unsigned int, WTF::Span<unsigned char const, 18446744073709551615ul> >, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul>(WebKit::RemoteGraphicsContextGL*, void (WebKit::RemoteGraphicsContextGL::*)(unsigned int, int, int, int, int, int, unsigned int, unsigned int, WTF::Span<unsigned char const, 18446744073709551615ul>&&), std::__1::tuple<unsigned int, int, int, int, int, int, unsigned int, unsigned int, WTF::Span<unsigned char const, 18446744073709551615ul> >&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul>) + 420 (HandleMessage.h:131)


To repro:
1. minibrowser debug
2. develop -> experimental -> enable GPU Process for webgl
3. close window, open new window
4. https://www.colorizephoto.com/converter
5. Clear website data
6. reload https://www.colorizephoto.com/converter
7. Observe GPU Process crash

[Radar WebKit Bug Importer]

<rdar://problem/94108163>

[Kyle Piddington]

Created attachment 459976 [details]
Patch

[EWS Watchlist]

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

[Kenneth Russell]

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.

[Kyle Piddington]

Created attachment 460022 [details]
Patch

[Darin Adler]

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

[Kimmo Kinnunen]

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)

[Kyle Piddington]

Created attachment 460052 [details]
Patch

[Darin Adler]

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.

[Kyle Piddington]

Created attachment 460053 [details]
Patch

[Darin Adler]

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?

[Kyle Piddington]

(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.

[Kyle Piddington]

Created attachment 460103 [details]
Patch

[EWS]

[e-mail] does not have reviewer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json.

Rejecting attachment 460103 [details] from commit queue.

[EWS]

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].