Closed
Bug 1421481
Opened 6 years ago
Closed 6 years ago
The mobile limit of 2 WebGL contexts per principal breaks GL-based images on tier-1 Google Search pages.
Categories
(Core :: Graphics: CanvasWebGL, defect, P3)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: wisniewskit, Assigned: jnicol)
References
(Blocks 1 open bug, )
Details
(Whiteboard: [webcompat][gfx-noted])
Attachments
(2 files)
It seems that Fennec has a hard-coded limit of 2 WebGL contexts per principal (kMaxWebGLContextsPerPrincipal in dom/canvas/WebGLContext.cpp). The comment there states that this is because some devices cannot handle more than 8 GL contexts. However, the Google Search pages are using more contexts than that for their tier-1 (currently served to Chrome) variants, which is causing a webcompat issue where tapping on a image thumbnail in user reviews causes a never-ended loading indicator, due to a page script error caused by the GL context limit. If one builds a custom Fennec nightly with the limit raised (I just went with 16), the error goes away. Chrome for Android seems to have a higher limit, but I'm not sure if it's a hard-coded value or one chosen via some heuristic. Either way, it would be good to head off webcompat issues by adopting their solution, since if Google are using it for their tier-1 content, then it's likely other sites will do so as well (though I'm not sure if many sites currently experience this kind of breakage). In the interest of brevity, I'll re-post the STR from the webcompat.com issue here: 1. Alter Fennec's UA to a Chrome mobile UA (one quick way is to install my addon: https://addons.mozilla.org/en-US/android/addon/google-search-fixer/). 2. Visit https://goo.gl/2SiYVI 3. Scroll down to one of the images in the user reviews and tap on it. 4. Observe the never-ending loading indicator, and console warnings of too many GL contexts (and the obscure GL-related script errors that break the load).
Comment 1•6 years ago
|
||
Milan, who would be the best person to comment on this? Thanks.
Flags: needinfo?(milan)
Chrome's implementation of context limiting is here I think: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp?gsn=CurrentMaxGLContexts&l=120 Looks like we support 8 concurrent contexts on OS_ANDROID, 16 elsewhere. The limit appears to be per-renderer-process, so approximately that equates to per-site.
Updated•6 years ago
|
Flags: needinfo?(milan) → needinfo?(jnicol)
Whiteboard: [webcompat] → [webcompat][gfx-noted]
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 3•6 years ago
|
||
These limits were added a long time ago so I'm unsure how strict they were at the time. I presume nowadays we can afford to relax them somewhat, especially if it's to match Chrome. We could always re-tighten the limits for certain devices if there are issues. I'd say increase the constants to 8 and 16. Snorp, do you agree?
Flags: needinfo?(jnicol) → needinfo?(snorp)
Sounds ok to me.
Flags: needinfo?(snorp)
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8933263 [details] Bug 1421481 - Increase allowed number of webgl contexts on mobile. https://reviewboard.mozilla.org/r/204206/#review209786
Attachment #8933263 -
Flags: review?(snorp) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•6 years ago
|
||
Milan asked that this be made a pref in case there are any issues.
Comment 10•6 years ago
|
||
Pushed by jnicol@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/824c4a13f3bb Increase allowed number of webgl contexts on mobile. r=snorp
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/824c4a13f3bb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 12•6 years ago
|
||
Man, I really need to be watching my own component. This sounds fine.
Assignee: nobody → jnicol
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8933263 [details] Bug 1421481 - Increase allowed number of webgl contexts on mobile. https://reviewboard.mozilla.org/r/204206/#review215342 ::: dom/canvas/WebGLContext.cpp:1157 (Diff revision 3) > } > > void > WebGLContext::LoseOldestWebGLContextIfLimitExceeded() > { > -#ifdef MOZ_GFX_OPTIMIZE_MOBILE > + const size_t maxWebGLContexts = gfxPrefs::WebGLMaxContexts(); These should probably be uint32_t to match the prefs. `auto` would handle this. ::: dom/canvas/WebGLContext.cpp:1162 (Diff revision 3) > -#ifdef MOZ_GFX_OPTIMIZE_MOBILE > - // some mobile devices can't have more than 8 GL contexts overall > - const size_t kMaxWebGLContextsPerPrincipal = 2; > - const size_t kMaxWebGLContexts = 4; > -#else > - const size_t kMaxWebGLContextsPerPrincipal = 16; > + const size_t maxWebGLContexts = gfxPrefs::WebGLMaxContexts(); > + size_t maxWebGLContextsPerPrincipal = gfxPrefs::WebGLMaxContextsPerPrincipal(); > + > + // maxWebGLContextsPerPrincipal must be less than maxWebGLContexts > + MOZ_ASSERT(maxWebGLContextsPerPrincipal < maxWebGLContexts); > + maxWebGLContextsPerPrincipal = std::min(maxWebGLContextsPerPrincipal, maxWebGLContexts - 1); Why is this -1? ::: gfx/thebes/gfxPrefs.h:743 (Diff revision 3) > DECL_GFX_PREF(Live, "webgl.enable-webgl2", WebGL2Enabled, bool, true); > DECL_GFX_PREF(Live, "webgl.force-enabled", WebGLForceEnabled, bool, false); > DECL_GFX_PREF(Once, "webgl.force-layers-readback", WebGLForceLayersReadback, bool, false); > DECL_GFX_PREF(Live, "webgl.force-index-validation", WebGLForceIndexValidation, int32_t, 0); > DECL_GFX_PREF(Live, "webgl.lose-context-on-memory-pressure", WebGLLoseContextOnMemoryPressure, bool, false); > + DECL_GFX_PREF(Once, "webgl.max-contexts", WebGLMaxContexts, uint32_t, 32); Why is this `Once`? Default should be `Live`.
Updated•6 years ago
|
Flags: needinfo?(jnicol)
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8933263 [details] Bug 1421481 - Increase allowed number of webgl contexts on mobile. https://reviewboard.mozilla.org/r/204206/#review215418 I'll post the patch to fix these issues outwith mozreview because I think it'd just get confused
Assignee | ||
Comment 15•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933263 [details] Bug 1421481 - Increase allowed number of webgl contexts on mobile. https://reviewboard.mozilla.org/r/204206/#review215342 > These should probably be uint32_t to match the prefs. `auto` would handle this. ok > Why is this -1? The previous code asserted maxWebGLContextsPerPrincipal < maxWebGLContexts, so I was making sure that still held. I'm guessing that's not necessary so I'll drop the -1 and make the assertion <= > Why is this `Once`? Default should be `Live`. ok
Assignee | ||
Comment 16•6 years ago
|
||
Flags: needinfo?(jnicol)
Attachment #8939556 -
Flags: review?(jgilbert)
Updated•6 years ago
|
Attachment #8939556 -
Flags: review?(jgilbert) → review+
Comment 17•6 years ago
|
||
Pushed by jnicol@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/65571ced9763 Post-review follow-up for webgl context initialization. r=jgilbert
Comment 18•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/65571ced9763
You need to log in
before you can comment on or make changes to this bug.
Description
•