938316 - Small resampling errors in Android GL layer compositing

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We have various small resampling errors on Android. Basically, some layers that should be composited with a simple pixel-aligned copy get slightly resampled, so at internal color boundaries, some pixels are rendered slightly off from the desired channel values.

I have a patch that works around the problem by detecting when we have integer-only translation transforms in CompositorOGL::DrawQuad, and in that case switching sampling to GL_NEAREST. When I apply this patch, a number of tests currently marked as failing on Android start to pass, namely
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30266/tests/layout/reftests/bugs/243519-4b.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30266/tests/layout/reftests/bugs/243519-4d.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30266/tests/layout/reftests/bugs/243519-4f.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30266/tests/layout/reftests/bugs/243519-5c.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30266/tests/layout/reftests/bugs/243519-5d.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30266/tests/layout/reftests/bugs/243519-9a.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30266/tests/layout/reftests/bugs/243519-9b.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30266/tests/layout/reftests/bugs/243519-9c.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30266/tests/layout/reftests/bugs/243519-9d.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30266/tests/layout/reftests/bugs/243519-9e.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30266/tests/layout/reftests/bugs/243519-9f.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30266/tests/layout/reftests/bugs/402807-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.163:30144/tests/layout/reftests/bugs/433700.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.163:30144/tests/layout/reftests/bugs/458487-4a.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.163:30144/tests/layout/reftests/bugs/458487-4b.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.163:30144/tests/layout/reftests/bugs/458487-4c.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.163:30144/tests/layout/reftests/bugs/586683-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.163:30144/tests/layout/reftests/bugs/602200-2.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.163:30144/tests/layout/reftests/bugs/633344-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.163:30144/tests/layout/reftests/bugs/836844-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.168:30238/tests/layout/reftests/transform-3d/scale3d-all.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.168:30238/tests/layout/reftests/transform-3d/scale3d-all-separate.html | image comparison (==)
So whatever this problem is, it's been around for a while.

This bug quite severely affects bug 876321, where we start to layerize moving absolutely-positioned elements, and a lot of existing reftests start to fail with small errors. In that bug, I did quite a lot of investigation to try to figure out what the problem is. I failed. The data we pass into GLContext looks good. Maybe I missed something or maybe the problem is in drivers. (I see different but similar failures on try vs on my Nexus S locally.)

So I propose we land my workaround to green up these tests. If someone wants to pursue the underlying problem into the guts of our GL compositor and/or Android itself, they're welcome to do so, and it will be easy enough to back out this patch for that investigation.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

In addition to this, I will need a patch that updates reftest manifests for passing tests. I'll attaach that separately once a clean try run is done.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Note that bug 937987 does not fix these issues.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 831765 [details] [diff] [review]
fix

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

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1107,5 @@
> +          aTransform.Is2DIntegerTranslation() &&
> +          textureTransform.Is2D(&textureTransform2D) &&
> +          textureTransform2D.HasOnlyIntegerTranslation()) {
> +        filter = GraphicsFilter::FILTER_NEAREST;
> +      }

I'm not really thrilled with this. It shouldn't be needed to get correct sampling. That being said I understand the motivation. Can you add a comment describing why it's there and expressing some sadness?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure.

Also, I decided to make this code Android-only to ensure that the same issues don't creep into other platforms without us noticing.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

I like that as well.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba6e69527aca

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ba6e69527aca