Enabled WR on a Moto G (XT1031) to see what would happen.

When I navigate to about:support, I see triangles on the page which rapidly seem to compress as I scroll the page.

If I pinch on the page, I manage to get something that looks like a page, but with corrupt text.

Kris can you triage this?

Adreno 3xx, so I assume the missing glyphs is just bug 1513185.

But that first screenshot looks fun! Is it reproducible?

(In reply to Jamie Nicol [:jnicol] from comment #4)

But that first screenshot looks fun! Is it reproducible?

Yeah, unfortunately it is impossible to use it with WR enabled. Check out example.com.

My hunch is that the triangles issue must be when compositing the picture cache tiles. We skip picture caching whilst pinch-zooming (as of bug 1589666), which would explain why the page is rendered correctly (modulo the missing glyphs) whilst zooming.

Asif, could you please confirm whether disabling picture caching fixes the triangles issue? And do you know whether this is a regression? Thanks!

(In reply to Jamie Nicol [:jnicol] from comment #6)

Asif, could you please confirm whether disabling picture caching fixes the triangles issue? And do you know whether this is a regression? Thanks!

Jamie, disabling picture caching does make the page render minus the missing glyphs. I don't think this is a regression, at least in Fenix - I went back to the earliest builds and it was basically always like this. I could try Fennec if you had a version in mind that you suspect might work.

Thanks for testing! Webrender doesn't work in fennec, so there's no need.

Had a wee look at this over the break. If you look at the triangles, they are always a solid colour, but it's always colours that are present on the page. In fact it seems that the whole triangle is given the colour of the provoking index, as if the UV varying were flat.

But the UV varying is not flat. In composite.glsl, we have this code:

flat varying float vLayer;
varying vec2 vUv;

// skip a few lines

vec4 texel = textureLod(sColor0, vec3(uv, vLayer), 0.0);

If I make vLayer not flat, then the page renders correctly. So by combining a non-flat and flat varying in to the same argument, it appears to "taint" both of them as flat.

Dzmitry, Glenn, would it be reasonable to remove the flat annotation from vLayer? How big a penalty would that be? It seems a shame to penalize all platforms due to a very old android GPU, but similarily I cannot find another workaround (and I'm not sure we want to go down the route of rewriting shaders at run time based on the GPU...)

I vaguely recall that we need that vLayer as flat in some cases for the regular WR shaders, due to some weird float accuracy issues we see on some GPUs when there is perspective involved, not just for the (potential) performance aspect.

However, it's probably fine to remove the flat from composite.glsl - the vertex counts for compositing tiles are very low, and there's never perspective involved.

A couple of things to consider:

• Is this going to show up as an issue in regular shaders with perspective on this device? (we can just keep an eye out for it in any bug reports).
• We're moving away from using texture arrays in general, since they are buggy on so many android devices. A patch to change picture cache tiles to not use arrays is probably fairly simple, and then we wouldn't have a vLayer param at all in any of the picture cache / composite.glsl code.

Yes, I'm also curious about whether this same bug affects other shaders, not just when the layer varying is flat, but potentially varyings completely unrelated to texture sampling. It seems likely, but we can't really tell at the moment because this obscures all other possible bugs.

I think as a first step we simply remove the flat from vLayer in composite.glsl. This will allow users on Adreno 3xx to start using webrender, and we can do some testing ourselves / wait for the bug reports to roll in. We may glean more information about the underlying bug which allows for a more specific fix. If it's limited to texture sampling then we probably want to completely move away from texture arrays and remove all layer varyings from all shaders. If it affects other shaders in other ways then we can deal with that then.

May I suggest doing something like

#ifdef ANDROID
#define FLAT
#else
#define FLAT flat
#endif

If flat doesn't work reliably on some platforms, we can't just get away with removing it on composite shaders, we'd have to take care of all shaders. In general, flat can make shaders faster. For example, the AMD driver can place this data into scalar registers, which reduces the pressure on vector registers in the shaders and therefore increases occupancy. Of course there are terms and conditions (e.g. we may need to change it from float to integer), but the general picture is there.

I don't think we can #ifdef on ANDROID due to the glslopt pass, unfortunately. We could do on GL_ES though.

If flat doesn't work reliably on some platforms, we can't just get away with removing it on composite shaders, we'd have to take care of all shaders.

Agreed, it seems likely this will affect other shaders too. However, the composite bug specifically makes webrender unusable on affected devices, so we cannot tell what else is buggy. I see removing it from composite.glsl as a temporary solution which will allow us to find out more information.

But GL_ES would also affect all the Windows, which is not desired. We could also have a WR shader feature called FLAT_INTERPOLATION or something, which is provided by all the platforms except those affected.

Glenn, with your recent render task work, is there anything that we still use texture arrays for? How would we feel about ripping out support for arrays from our shaders, or at least the composite one? If we remove vLayer this bug goes away. (The underlying bug might still bite us elsewhere, but I haven't seen any other issues from some basic testing.)

It would be great to rip out support for arrays from as many places as possible! (the changes in question have been in m-c for a few weeks now, and seem stable).

I think there might be some cases where external textures provided by Gecko are in a texture array - but this is just a vague recollection, I have no data to back this up. Even if it is the case, it might only be because that's what WR previously preferred, and something we can change in Gecko?

If we can sort out the above, I think we can remove layer stuff altogether, from everywhere. We definitely should be able to remove it from all the composite shaders, and also from everything that reads / writes from render task data.

Talked to mstange and sotaro in matrix - seems that we don't use array textures for external textures at all. In which case, array texture shaders should never be getting created (we could do a try run and assert on this?), and we should be able to remove all array texture support from WR.

This is fixed now that bug 1694909 has landed

Hmm, so the composite shader indeed no longer renders everything as triangles, but it appears the brush image shader still does. So I'll re-open this

The problem in the brush_image shader is v_perspective being flat. Like vLayer in the composite shader (prior to removing texture array support), both are a flat scalar which are used in the calculation of the UV coordinate. My theory is that the driver is attempting to be clever and do as much of the calculation as possible "flatly", but it gets this wrong. So it calculates the UV coordinates for the provoking vertex only, and uses it for every fragment in the triangle.

I tried playing around with the shader to make it think the calculation is not flat (eg using an if statement or ternary instead of the mix() which uses v_perspective). But it seems pretty "clever" about determining if it should be flat. Using a flat int also didn't help, however, using a flat vec2 did. I think that's probably a better workaround than making the varying non-flat. Hopefully most drivers can optimize out the unused y component of the vector (Mali seems to at least), and I'll make it only apply to Android.

It seems like this probably only applies to UV coordinate calculations: There are plenty of instances in our shaders of flat scalars being used to calculate the output fragment colour, but v_perspective in several shaders may be the only case where it affects UV coordinates. It seems plausible that the driver handles them differently than the rest of the shader.

I've only seen this in brush_image (and previously composite), but I plan to add the workaround for all cases of v_perspective in our shaders.

On Adreno 3xx we have observed that the v_perspective varying in
brush_image being a flat float results in the entire UV coordinate
calculation being performed as if flat. This results in images being
rendered as solid color triangles. (The color coming from the texture
sample at the UV coordinate calculated for the provoking vertex.)

Packing the varying in a vector works around this bug. This is seen as
preferrable to making the varying non-flat.

This only appears to affect UV coordinate calculations, rather than
any calculation affecting the fragment output color.

https://hg.mozilla.org/mozilla-central/rev/968d60e42f62

For posterity, this issue has arisen again in bug 1705433. Adding this comment here so we can link to a single bug (this one) in the source code and find out all the information.

It seems to not only affect varyings which are used to calculate UV coordinates, but also just varying which are used to calculate the output color. I know that vClipMode in cs_clip_box_shadow.glsl, v_gradient_repeat in gradient.glsl (shared between the various gradient shaders), and vFloat0 in cs_svg_filter.glsl are all affected too. (From inspecting wrench test failures).