Closed Bug 1421481 Opened 5 years ago Closed 5 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)

Product:
Core
Component:
Graphics: CanvasWebGL
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: wisniewskit, Assigned: jnicol)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [webcompat][gfx-noted])

Attachments

(2 files)

Bug 1421481 - Increase allowed number of webgl contexts on mobile.
59 bytes, text/x-review-board-request
snorp
: review+
Details
0001-Bug-1421481-Post-review-follow-up-for-webgl-context-.patch
2.70 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review 
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).
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.
Flags: needinfo?(milan) → needinfo?(jnicol)
Whiteboard: [webcompat] → [webcompat][gfx-noted]
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 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+
Milan asked that this be made a pref in case there are any issues.
Pushed by jnicol@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/824c4a13f3bb
Increase allowed number of webgl contexts on mobile. r=snorp
https://hg.mozilla.org/mozilla-central/rev/824c4a13f3bb
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Man, I really need to be watching my own component.
This sounds fine.
Assignee: nobody → jnicol
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`.
Flags: needinfo?(jnicol)
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
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
Flags: needinfo?(jnicol)

        Attachment #8939556 -
        Flags: review?(jgilbert)

        Attachment #8939556 -
        Flags: review?(jgilbert) → review+

    
    

    
  
Pushed by jnicol@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65571ced9763
Post-review follow-up for webgl context initialization. r=jgilbert

          You need to log in
          before you can comment on or make changes to this bug.