core(full-page-screenshot): get the MAX_TEXTURE_SIZE from the browser #11847

paulirish · 2020-12-17T08:10:36Z

Connor called this out before and I confirmed and documented it: https://bugs.chromium.org/p/chromium/issues/detail?id=770769#c13

(the 16384 number is the max_texture_size for swiftshader, used by headless (most of the time), but other devices have other ones)

This changes our impl to pull it from the browser rather than use a hardcoded value that might cause problems.

…rowser

connorjclark · 2020-12-17T08:36:17Z

lgtm but my concerns: what's the overhead for making a webgl context? Could this be more noticeable in LR given we are software rendering based there?

connorjclark · 2020-12-17T08:37:47Z

lighthouse-core/lib/page-functions.js

+/* istanbul ignore next */ // todo, c8 ignore?
+function getMaxTextureSize() {
+  const canvas = document.createElement('canvas');
+  const webGL = canvas.getContext('webgl');


Is destroying necessary?

no, but i added it anyway.

patrickhulce · 2020-12-17T15:16:59Z

what's the overhead for making a webgl context? Could this be more noticeable in LR given we are software rendering based there?

Related: are there any environments where webGL might be disabled entirely? How does this fail when it is and should we have a fallback?

AFAIK you'd have to go manually disable WebGL in Chrome flags, so probably not a big deal, but pretty costless to have a fallback too.

brendankenny · 2020-12-21T19:59:24Z

Related: are there any environments where webGL might be disabled entirely? How does this fail when it is and should we have a fallback?

it used to be disabled quite a bit due to driver/OS bugs possibly being security issues (e.g. being able to read rasterized pixels from visible windows other than the browser). Now that swiftshader is cross platform...does Chrome always fall back to it?

Since webgl isn't actually used for taking the screenshot, this is just a query about machine capabilities, maybe it doesn't matter and we should just have a reasonable fallback number no matter what.

lighthouse-core/gather/gatherers/full-page-screenshot.js

brendankenny · 2020-12-21T20:02:45Z

lighthouse-core/lib/page-functions.js

+/* istanbul ignore next */ // todo, c8 ignore?
+function getMaxTextureSize() {
+  const canvas = document.createElement('canvas');
+  const webGL = canvas.getContext('webgl');


tsc would correctly call out webGL as WebGLRenderingContext | null here if the file wasn't // @ts-nocheck, so need to handle that case (e.g. webgl is disabled on that machine, as discussed in the other comments :)

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

paulirish · 2020-12-21T23:30:54Z

what's the overhead for making a webgl context?

~10ms on LR. ~20ms on my oldass MBP.

Could this be more noticeable in LR given we are software rendering based there?

overhead nah, but the M_T_S there is 8192, so our 16k number will give us a problem in LR. either crashes or image repeating. (not sure which)

should we have a fallback?

yes good call. added one.

all comments addressed afaik.

connorjclark · 2021-01-13T00:02:06Z

lighthouse-core/test/gather/gatherers/full-page-screenshot-test.js

-      }
-      if (code.includes('document.documentElement.clientWidth')) {
+    evaluate: async function(fn) {
+      if (fn.name === 'resolveNodes') {


isn't this so much nicer

fn.name is pretty tight. :) good call.

connorjclark · 2021-01-13T00:03:13Z

lighthouse-core/gather/gatherers/full-page-screenshot.js

+   * @see https://bugs.chromium.org/p/chromium/issues/detail?id=770769
+   */
+  async getMaxScreenshotHeight(driver) {
+    return await driver.evaluate(pageFunctions.getMaxTextureSize, {


curious why a page function and not local to this module?

mostly because theres a lot of ways in which PFs are very diff than node code and so i'd like to ideally have all PFs live in separate files.

code coverage exemption (istanbul ignore/c8 ignore) is easier since they're colocated

can apply a eslint-env browser instead of our current handcrafted /* global window document Node ShadowRoot */ list.

maybe use some browserish config for typescript on these files

--

but until all are migrated there i'll just place new ones in pf.js. thats my thinking.

i don't care a lot about this particular case tho.

but until all are migrated there i'll just place new ones in pf.js. thats my thinking.

Funny, I have the exact opposite thinking. Not speaking to the idea at all, but if we are currently doing "all page functions local if possible", we should keep doing that until we change how we structure things, instead of starting a transitionary thing w/o any discussion.

fwiw i like your bullet points

word.

well... looking at the fn's in pf.js, there already are plenty that are one-offs used by gatherrunner & driver. oh and a singleuse for iframe-elements! :)

connorjclark · 2021-01-13T00:07:17Z

lighthouse-core/lib/page-functions.js

+    let canvas = document.createElement('canvas');
+    let gl = canvas.getContext('webgl');
+    const maxTextureSize = gl.getParameter(gl.MAX_TEXTURE_SIZE);
+    canvas = gl = undefined; // Cleanup for GC


I thought there might have been a more explicit "destroy" method for a context, but there isn't. As long as the element is not on the DOM, we're good. Can remove the "cleanup" line.

sorry for confusion

maybe i'm overly cautious but memory mgmt with webgl feels like it has some gotchas.. as such, i'm prone to keep the explicit cleanup and not just wait for auto-GC

@mathiasbynens we're just two developers who don't know how GC works. wdyt? :)

Neither canvas nor the gl context escapes from this block, so it should in theory be fine even without the explicit cleanup. (In case anyone‘s interested, I wrote a high-level explanation of escape analysis when I learned about it.)

That said, I’m not familiar with Chromium’s WebGL context lifecycle, and if perhaps there are weird side effects to creating such contexts that I don’t know about.

…#11847)

core(full-page-screenshot): determine the MAX_TEXTURE_SIZE from the b…

f19d171

…rowser

paulirish requested a review from a team as a code owner December 17, 2020 08:10

paulirish requested review from Beytoven and removed request for a team December 17, 2020 08:10

google-cla bot added the cla: yes label Dec 17, 2020

devtools-bot assigned Beytoven Dec 17, 2020

devtools-bot added the waiting4reviewer label Dec 17, 2020

connorjclark reviewed Dec 17, 2020

View reviewed changes

brendankenny reviewed Dec 21, 2020

View reviewed changes

Update lighthouse-core/gather/gatherers/full-page-screenshot.js

09b4027

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

vercel bot deployed to Preview December 21, 2020 21:02 View deployment

set fallback. destroy for gc

ce7680a

vercel bot deployed to Preview December 22, 2020 00:01 View deployment

paulirish added 4 commits January 12, 2021 15:55

use driver.evaluate()

ab9de8c

drop c8 ignore thang

8c61f9d

use fn name instead

3e9a9a8

Merge remote-tracking branch 'origin/master' into maxtexturesize

93d36fc

vercel bot deployed to Preview January 13, 2021 00:00 View deployment

paulirish assigned connorjclark and unassigned Beytoven Jan 13, 2021

paulirish removed the request for review from Beytoven January 13, 2021 00:01

lint

17f7f04

vercel bot deployed to Preview January 13, 2021 00:01 View deployment

connorjclark reviewed Jan 13, 2021

View reviewed changes

connorjclark approved these changes Jan 13, 2021

View reviewed changes

paulirish added the land-when-ci-is-green label Jan 13, 2021

devtools-bot merged commit 3a820a3 into master Jan 13, 2021

devtools-bot deleted the maxtexturesize branch January 13, 2021 00:48

devtools-bot removed the land-when-ci-is-green label Jan 13, 2021

patrickhulce mentioned this pull request Jan 14, 2021

core: add type checking to page functions #11958

Merged

connorjclark mentioned this pull request Mar 10, 2021

☂️ Element screenshots #12230

Closed

16 tasks

paulirish added a commit that referenced this pull request Mar 23, 2021

core(full-page-screenshot): get the MAX_TEXTURE_SIZE from the browser (…

9d3c53e

…#11847)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(full-page-screenshot): get the MAX_TEXTURE_SIZE from the browser #11847

core(full-page-screenshot): get the MAX_TEXTURE_SIZE from the browser #11847

paulirish commented Dec 17, 2020

connorjclark commented Dec 17, 2020 •

edited

Loading

connorjclark Dec 17, 2020

paulirish Dec 21, 2020

patrickhulce commented Dec 17, 2020

brendankenny commented Dec 21, 2020

brendankenny Dec 21, 2020

paulirish commented Dec 21, 2020

connorjclark Jan 13, 2021

paulirish Jan 13, 2021

connorjclark Jan 13, 2021

paulirish Jan 13, 2021

connorjclark Jan 13, 2021 •

edited

Loading

paulirish Jan 13, 2021

connorjclark Jan 13, 2021

paulirish Jan 13, 2021

connorjclark Jan 13, 2021

mathiasbynens Jan 13, 2021

core(full-page-screenshot): get the MAX_TEXTURE_SIZE from the browser #11847

core(full-page-screenshot): get the MAX_TEXTURE_SIZE from the browser #11847

Conversation

paulirish commented Dec 17, 2020

connorjclark commented Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Dec 17, 2020

brendankenny commented Dec 21, 2020

Choose a reason for hiding this comment

paulirish commented Dec 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Dec 17, 2020 •

edited

Loading

connorjclark Jan 13, 2021 •

edited

Loading