In bug 1555479 I made the wrench reftests run on a Pixel 2 device. On debug wrench builds, I see some tests triggering panics on calls to draw_elements_instanced. On release wrench builds those tests generally just fail, probably due to the call not doing what it's supposed to. The set of tests on which this happens overlaps with, but isn't quite the same as, the set of tests for which glBlitFramebuffer was failing in the emulator.

Anyway, this needs fixing since it appears to affect correctness on a real device.

Are there steps to reproduce locally?

STR:

• Apply patches from bug 1555479 (or wait for them to land)
• Remove the skip_on(android) annotations from gfx/wr/wrench/reftests/blend/reftest.list
• Build wrench in debug mode per instructions
• Plug in Pixel 2 device and export DEVICE_SERIAL=<deviceserial>
• Run tests per instructions

(The instructions in android.txt get updated as part of bug 1555479 to include those last two steps)

Got them running. It's actually DEVICE_SERIAL that needs exported in case anyone else follows these instructions and it keeps launching an emulator instead of on their device.

Whoops, sorry about that! I'll edit my comment just in case.

We hit this error on the subsequent draw call after setting an advanced blend equation.

In the documentation for glBlendEquation [1] it says:

Advanced blending equations are supported only when rendering to a single color buffer using fragment color zero. If any non-NONE draw buffer uses an advanced blend equation the error INVALID_OPERATION is generated by glDrawArrays or any other drawing command.

I'm not aware of us using fancy draw buffers, so don't think that's it.

But in the documentation for KHR_blend_equation_advanced [2] I can also see:

Advanced blending equations require the use of a fragment shader with a matching "blend_support" layout qualifier. If the current blend equation is found in table X.1 or X.2, and the active fragment shader does not include the layout qualifier matching the blend equation or "blend_support_all_equations", the error INVALID_OPERATION is generated by [[Compatibility Profile: Begin or any operation that implicitly calls Begin (such as DrawElements)]] [[Core Profile and OpenGL ES: DrawArrays and the other drawing commands defined in section 2.8.3]] The set of layout qualifiers supported in fragment shaders is specified in sectino 4.3.8.2 of the OpenGL Shading Language Specification.

Which hurts to read, but seems more likely! Sure enough I can't find any relevant layout qualifiers in our shader code, and if I add it to out vec4 oFragColor in shared.glsl (and addGL_KHR_blend_equation_advanced : enable to the top) then the error goes away. The blend suite runs to completion, though there are some failures.

I need to figure out exactly where is the right place to add that qualifier, rather than including it for all shaders. But this looks like the solution. What I don't understand is why we're not running in to this problem on desktop. ¯\_(ツ)_/¯

[1] https://www.khronos.org/registry/OpenGL-Refpages/es3/html/glBlendEquation.xhtml
[2] https://www.khronos.org/registry/OpenGL/extensions/KHR/KHR_blend_equation_advanced.txt

The reason this doesn't fail on desktop seems to be that the test machines (linux, mac and windows) don't support KHR_blend_equation_advanced, so we don't attempt to use them. We also only seem to enable advanced blend in wrench and not in firefox.

If I run the wrench reftests on my local linux machine, which does support the extension, I get the same error in draw_elements_instanced.

I have a fix, but the thing I'm unsure about is whether setting layout (blend_support_all_equations) has a negative effect when it is unneeded. So do we want to try to avoid setting it if we won't use it?

Either we set the layout #if defined(WR_FEATURE_ALPHA_PASS) && defined(GL_KHR_blend_equation_advanced). ie For all alpha shaders if the extension is supported. But this will mean it affects all non-none blend modes, not just advanced. Alternatively, we can create an additional advanced_blend version of the shaders (behind a WR_FEATURE_ADVANCED_BLEND flag) and only set the layout for those.

My patches fix the blend tests on my Pixel 2 running android 9, but fail on try's (which are android 8). [1]

So I've done some testing on the phones on my desk:

• OnePlus 6 (Adreno 630), or Pixel 2 (Adreno 5xx) with Android 9: Tests pass
• Pixel 2 (Adreno 5xx) Android 8: Tests fail
• Moto G6 (Adreno 5xx) Android 8: Tests fail
• Samsung S6 (Mali T760): Tests pass [2]

Doing some googling, it seems Skia have disabled advanced blending on Adrenos [3]. So it looks like we might want to do that also on Adrenos running Android less than 9.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4432718715aac7e2a0391e7d76add4ab40c48d51
[2] Actually I hit a shader compiler error when adding the layout specifier specifically to oFragColor on the S6. But it works fine if simply do layout(...) out;, rather than layout(...) out vec4 oFragColor;. Both versions compile fine on other devices.
[3] https://skia.googlesource.com/skia.git/+/0ea7d43b2022299acbaab6accca1b33c4b3645b8/src/gpu/gl/GrGLCaps.cpp#972

When using an advanced blend equation, fragment shader output must be
marked with a matching layout qualifier. Not doing so was causing
subsequent glDraw* operations to fail.

This patch adds a new shader feature, WR_FEATURE_ADVANCED_BLEND, which
requires the necessary extension and adds the qualifier. Variants of
the brush_image shaders are created with this feature, and are used
whenever a brush_image shader is requested for BlendMode::Advanced.

There appears to be a driver bug on android 8 and older where it does
not render correctly.

Depends on D34617

These now work on actual devices now, but must remain disabled on the
emulator until bug 1555002 is fixed.

Depends on D34618

https://hg.mozilla.org/mozilla-central/rev/501698e75525
https://hg.mozilla.org/mozilla-central/rev/6ac6838b4cd8
https://hg.mozilla.org/mozilla-central/rev/76571050b8ff