Closed Bug 739648 Opened 8 years ago Closed 8 years ago

WebGL compositing is broken on Android due to tiles being rendered at y-flipped coordinates

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 15
blocking-kilimanjaro +
Tracking Status
firefox14 --- fixed
firefox15 --- verified
firefox16 --- verified
firefox17 --- verified
blocking-fennec1.0 --- soft

People

(Reporter: martijn.martijn, Assigned: bjacob)

References

()

Details

(Keywords: regression, Whiteboard: webgl-conformance [gfx])

Attachments

(3 files)

Attached image Screenshot of bug
Tested on the Samsung Galaxy Nexus, Android 4.0.2.

See screenshot, the webgl demo seems to be split in 3 where the bottom is at the top and the top is at the bottom.

This seems to have regressed between 2012-03-22 and 2012-03-23:
http://hg.mozilla.org/mozilla-central/rev/5c13fce74f83
http://hg.mozilla.org/mozilla-central/rev/ab2ff3b5611f

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2012-03-22+00%3A00%3A00&enddate=2012-03-23+02%3A00%3A00
Assignee: nobody → bjacob
blocking-fennec1.0: ? → +
I'm running into the same bug on a Nexus S running 2.3.4 with a brand new build from mozilla-central.

This is happening for all WebGL applications for me, including http://demoseen.com/webray/ http://people.mozilla.org/~bjacob/webgl-spotlight-js-2012/webgltrianglecolor.html https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/demos/webkit/SpiritBox.html
Not knowing anything about tiling, this looks like a tiling issue.
Whiteboard: webgl-conformance
Whiteboard: webgl-conformance → webgl-conformance [gfx]
blocking-fennec1.0: + → soft
blocking-kilimanjaro: --- → ?
Do we know if this breaks a general case or any of our tier one targeted apps?
This breaks all WebGL apps on my Nexus S. 100% reproducible.
Also, Jeff is right, this is a regression from tiling.
(In reply to Benoit Jacob [:bjacob] from comment #6)
> Also, Jeff is right, this is a regression from tiling.

This is a problem in TiledTextureImage, not TiledThebesLayer. It's not really a regression, but reducing the tile size has made the problem worse.
Comment on attachment 620793 [details] [diff] [review]
add mFlags field to TextureImage; introduce GetSrcTileRect flipping tile y-coords as needed; use it in DirectUpdate().

Nice, let's land ASAP and discuss uplifting this.
Attachment #620793 - Flags: review?(bgirard) → review+
For aurora, if you want, we can do a simpler patch manually tweaking the coords in DirectUpdate, now that we know that it's the only place that needs tweaking.
Summary: WebGL aquarium demo split in 3 → WebGL rendering is broken on Android due to tiles being rendered at y-flipped coordinates
(In reply to Benoit Jacob [:bjacob] from comment #10)
> For aurora, if you want, we can do a simpler patch manually tweaking the
> coords in DirectUpdate, now that we know that it's the only place that needs
> tweaking.

I take that back: in TiledTextureImage::DirectUpdate, without my patch, we don't know if the tiling is flipped or not. Adding that cleanly is what the patch does; so there is no other way than landing it on aurora.
Comment on attachment 620793 [details] [diff] [review]
add mFlags field to TextureImage; introduce GetSrcTileRect flipping tile y-coords as needed; use it in DirectUpdate().

[Approval Request Comment]
Regression caused by (bug #): not sure. bug in TiledTextureImage.
User impact if declined: webgl unusable on many Android phones, at least Nexus S
Testing completed (on m-c, etc.): inbound
Risk to taking this patch (and alternatives if risky): this patch is not going to make us crash or cause a security issue. But in case of a bug, it could give broken rendering. That should be caught by reftests though, and some initial testing on my phone didn't surface any bugs.
String changes made by this patch: none
Attachment #620793 - Flags: approval-mozilla-aurora?
Target Milestone: Firefox 14 → ---
Attachment #620793 - Flags: approval-mozilla-aurora?
Summary: WebGL rendering is broken on Android due to tiles being rendered at y-flipped coordinates → WebGL compositing is broken on Android due to tiles being rendered at y-flipped coordinates
Strange Mac OSX Reftest oranges. New try rebased on current m-c:
https://tbpl.mozilla.org/?tree=Try&rev=d18d8bdee069
Still the same failures. The test is

http://hg.mozilla.org/mozilla-central/raw-file/9ebf3dc839c5/layout/reftests/bugs/495385-2b.html

The reftest analyzer seems to be comparing 5x5 gray images, which doesn't seem to have anything to do with that test:

https://tbpl.mozilla.org/?tree=Try&rev=d18d8bdee069
-> click the Mac orange R
-> click 'open reftest analyzer'

Any clue?
blocking-kilimanjaro: ? → +
FWIW comment 17 was just me being ignorant about reftest analyzer. What's happening is we're using linear filtering where we should be using nearest filtering. Debugging.
Cannot reproduce this reftest failure locally on a MacBook Air running Mac OS 10.7.3.
Found an issue that could well explain exactly that problem! Forgot to add the aFlags parameter to functions such as CreateBasicTextureImage in the CGL provider. So we failed to properly override these virtual functions in the CGL provider.

Waiting on try results to see if that is all,
  https://tbpl.mozilla.org/?tree=Try&rev=b58636fa1308
as I can't reproduce the failure locally. Might only be reproducible with NVIDIA driver. Indeed the issue is a fine difference in rendering with linear filtering, where NVIDIA gives (254,254,254) instead of the expected (255,255,255) and the real problem is that we shouldn't be using linear filtering here.
Attachment #622467 - Flags: review?(bgirard)
Attachment #622467 - Flags: review?(bgirard) → review+
Green! Landed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6d551fea3297

Folded the two patches into 1 cset as the second one was just adding stuff that was missing from the first and avoided a regression from it.
Target Milestone: --- → Firefox 15
Comment on attachment 620793 [details] [diff] [review]
add mFlags field to TextureImage; introduce GetSrcTileRect flipping tile y-coords as needed; use it in DirectUpdate().

[Approval Request Comment]
Regression caused by (bug #): not sure. regression in TiledTextureImage. window given in comment 0.
User impact if declined: WebGL unusable on many Android phones (broken rendering)
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): doesn't seem risky, but if we are very unlucky, we could have rendering regressions elsewhere, even though reftests should catch exactly that, and they did catch a regression in an earlier version of the patch, see above comment.
String changes made by this patch: none
Attachment #620793 - Flags: approval-mozilla-aurora?
We are leaving all non-beta+ bugs nominated for Aurora approval in the queue until FN14 Beta 1 is signed off on by QA.
https://hg.mozilla.org/mozilla-central/rev/6d551fea3297
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 620793 [details] [diff] [review]
add mFlags field to TextureImage; introduce GetSrcTileRect flipping tile y-coords as needed; use it in DirectUpdate().

[Triage Comment]
QA has now signed off on our first beta of FN. Approving for Aurora 14.
Attachment #620793 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I cannot verify this bug on the Galaxy Nexus because of bug 746794.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.