Closed Bug 1654459 Opened 4 years ago Closed 4 years ago

WebGL on Android is doing readback (not using SharedSurface_SurfaceTexture) by default

Categories

(Core :: Graphics: CanvasWebGL, defect, P2)

Product:
Core
Component:
Graphics: CanvasWebGL
Platform:
Unspecified
Android
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
83 Branch
Tracking Flags:
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox78 --- unaffected
firefox79 --- wontfix
firefox80 --- wontfix
firefox81 --- wontfix
firefox82 --- wontfix
firefox83 --- fixed

People

(Reporter: sotaro, Assigned: snorp)

References

(Blocks 2 open bugs, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1654459 - Default webgl.enable-surface-texture to true.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1654459 - Don't use single-buffer SurfaceTexture on emulator
47 bytes, text/x-phabricator-request
Details | Review

WebGL in content process always used SharedSurface_SurfaceTexture until Bug 1632249 fix. Since the fix, SharedSurface_SurfaceTexture is not used if webgl.enable-surface-texture is false.

Regressed by: 1632249
Has Regression Range: --- → yes

The following is old implementation for selecting SurfaceFactory_SurfaceTexture.
https://searchfox.org/mozilla-esr68/source/gfx/gl/GLScreenBuffer.cpp#94

Since Bug 1632249, at first, TexTypeForWebgl() chooses TextureType::AndroidNativeWindow only when StaticPrefs::webgl_enable_surface_texture() is true.

Then SurfaceFactory::Create() chooses a factory when texture type is TextureType::AndroidNativeWindow.

Blocks: 1639280

: jgilbert, can you take this bug?

Flags: needinfo?(jgilbert)

It sounds like the pref is set wrong, but we had two wrongs make a right before, when we ignored the pref!

Flags: needinfo?(jgilbert)
OS: Unspecified → Android

This should match the previous behavior where non-parent processes
ignored the pref and used surface textures.

Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
Severity: -- → S4
Priority: -- → P1

Set release status flags based on info from the regressing bug 1632249

status-firefox78: --- → unaffected
status-firefox79: --- → affected
status-firefox80: --- → affected
status-firefox-esr68: --- → unaffected
status-firefox-esr78: --- → unaffected

Jamie, enabling SurfaceTexture-backed WebGL as this patch attempts to do seems to MOZ_CRASH for me in Fenix[1]. I get some relevant looking driver spew before:

07-22 16:15:06.251 26521 26542 I Adreno  : GetNumBufferSupported: clamped value 2 is not suitable as it exceeded maxBufferCount requested by consumer 1
07-22 16:15:06.251 26521 26542 I Adreno  : InitializeInternal:GetNumBufferSupported failed
07-22 16:15:06.251 26521 26542 I Adreno  : Create: Initialize failed
07-22 16:15:06.251 26521 26542 I Adreno  : GetNumBufferSupported: clamped value 2 is not suitable as it exceeded maxBufferCount requested by consumer 1
07-22 16:15:06.251 26521 26542 I Adreno  : InitializeInternal:GetNumBufferSupported failed
07-22 16:15:06.251 26521 26542 I Adreno  : Create: Initialize failed

Do you have a chance to look into this? Without the ST path, WebGL is doing readpixels under WebRender and it's very sad.

[1] https://crash-stats.mozilla.org/report/index/31b3f3d5-98cf-4181-8c61-227a90200722

Flags: needinfo?(jnicol)

Hey snorp, I've tried to reproduce your crash but it works fine for me on a few different devices. Webgl aquarium runs poorly, then I flip that pref and it runs much faster, without crashing.

Is there a specific website you encountered this on? Is it reproducible? Could you try a different device and see if that makes a difference?

Flags: needinfo?(jnicol) → needinfo?(snorp)

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

Hey snorp, I've tried to reproduce your crash but it works fine for me on a few different devices. Webgl aquarium runs poorly, then I flip that pref and it runs much faster, without crashing.

Is there a specific website you encountered this on? Is it reproducible? Could you try a different device and see if that makes a difference?

It's every website (that uses WebGL), but I've only tried it on Pixel 4 XL. I'm charging some other devices to try there.

Flags: needinfo?(snorp)

P1 bug sitting around for a while without a fix. Jeff can we get something moving here or update the status flags? Thanks!

Flags: needinfo?(jgilbert)

No problem on Pixel 2, so seems to be somewhat specific to my Pixel 4 XL.

Interesting. Thanks for testing!

Sotaro, do you have a Pixel 4 or Pixel 4XL? Kris, this is an important crash to fix. If Sotaro doesn't have a device maybe I should order one for myself?

In the meantime should we enable the pref but add some code to disable it on the Pixel 4 XL? (and maybe the Pixel 4, and maybe all Adreno 640 devices)

Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(ktaeleman)

The problem seems related to using a single-buffer SurfaceTexture. If I use a multi-buffer one it doesn't crash, but perf is awful (about 4 fps). Profile shows fDrawElements() taking forever, which is very curious.

We probably need to adjust the EGLConfig somehow to be compatible with a single-buffer SurfaceTexture.

Maybe Sotaro's AHardwareBuffer work might fix it, too.

I think there are some API levels on which we're forced to use multi-buffer, so we should make sure performance isn't terrible on those!

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

Interesting. Thanks for testing!

Sotaro, do you have a Pixel 4 or Pixel 4XL?

Sorry, I do not have them, I just have Pixel 3a and it did not cause the problem.

Flags: needinfo?(sotaro.ikeda.g)

Since Adreno 640 will be enabled on next release with FF80 and it looks like we don't have a solution for this in mind yet, disabling WR for 640 and uplifting sounds like the right choice.

To move this forward, is there anything we can try without buying the phone?

Flags: needinfo?(ktaeleman) → needinfo?(jnicol)

This pref isn't currently enabled, so I don't think we have to worry about uplifting anything just yet. We should enable it though (with a workaround of some sort for affected devices).

I'm also not sure whether it's webrender specific? Presumably we're currently going through a readback path for layers too, and flipping this pref will make both layers and webrender use SurfaceTextures? It might be a useful clue if it does in fact only crash for webrender.

Snorp, does it only crash with webrender enabled? Or does it crash for either layers or webrender? Also, has canvas always crashed on your Pixel 4XL when using the SurfaceTexture path, or is it a recent regression (likely from the OOP changes)?

Flags: needinfo?(jnicol) → needinfo?(snorp)

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

Snorp, does it only crash with webrender enabled? Or does it crash for either layers or webrender?

It crashes the same way with or without WR.

Also, has canvas always crashed on your Pixel 4XL when using the SurfaceTexture path, or is it a recent regression (likely from the OOP changes)?

I'm not totally sure. I can try to bisect a bit.

Flags: needinfo?(snorp)
Assignee: jgilbert → snorp
Flags: needinfo?(jgilbert)
Summary: WebGL in content process does not use SharedSurface_SurfaceTexture by default → WebGL on Android is doing readback (not using SharedSurface_SurfaceTexture) by default

mozregression isn't working, but I can't seem to repro the crash anymore with latest m-c. It also works in Fenix nightly. I'd say we should go ahead and land this.

Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25f0521ab3a1
Default webgl.enable-surface-texture to true. r=snorp
Priority: P1 → P2

(In reply to Natalia Csoregi [:nataliaCs] from comment #22)

Also crashes: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=313382822&repo=autoland&lineNumber=2306

This looks similar to Bug 1659681.

Depends on: 1659681
Flags: needinfo?(snorp)
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d32596b492b1
Don't use single-buffer SurfaceTexture on emulator r=sotaro

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox82: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1c35c35dd964
Default webgl.enable-surface-texture to true. r=snorp
Status: RESOLVED → REOPENED
status-firefox81: fix-optional → wontfix
status-firefox82: fixed → affected
Resolution: FIXED → ---
Target Milestone: 82 Branch → ---
See Also: → 1595026

(In reply to Sotaro Ikeda [:sotaro] from comment #29)

I wonder if the problem might be related to Bug 1595026.

Yeah, that seems plausible. Also, all of the failures have "readback" in the query string, which seems significant. I would've guessed that would cause it to use the readback path and avoid SurfaceTexture entirely, but maybe that's not right? Jeff?

Flags: needinfo?(snorp) → needinfo?(jgilbert)

I confirmed that WebGL with SurfaceTexture and WebRender worked on Android8 emulator(x86) and Android 10 emulator(x86) . It did not work on Android 7 emulator(x86) . Android 7.0 emulator is current default emulator on m-c. WebGL became stable with Bug 1665300.

When I run SurfaceTexture WebGL on Android 7.1.1 device with Adreno 506, the problem did not happen. WebGL was rendered as expected. Then the problem seemed to happen only on Android 7 emulator.

Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/19541d7e6dab
Default webgl.enable-surface-texture to true. r=snorp

https://hg.mozilla.org/mozilla-central/rev/19541d7e6dab

Status: REOPENED → RESOLVED
Closed: 4 years ago → 4 years ago
status-firefox83: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #30)

(In reply to Sotaro Ikeda [:sotaro] from comment #29)

I wonder if the problem might be related to Bug 1595026.

Yeah, that seems plausible. Also, all of the failures have "readback" in the query string, which seems significant. I would've guessed that would cause it to use the readback path and avoid SurfaceTexture entirely, but maybe that's not right? Jeff?

It might not be working properly. :S Thanks for the heads-up.

Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: