Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Jan 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions lighthouse-core/gather/gatherers/full-page-screenshot.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@
const Gatherer = require('./gatherer.js');
const pageFunctions = require('../../lib/page-functions.js');

/** @typedef {import('../driver.js')} Driver */

// JPEG quality setting
// Exploration and examples of reports using different quality settings: https://docs.google.com/document/d/1ZSffucIca9XDW2eEwfoevrk-OTl7WQFeMf0CgeJAA8M/edit#
const FULL_PAGE_SCREENSHOT_QUALITY = 30;
// Maximum screenshot height in Chrome https://bugs.chromium.org/p/chromium/issues/detail?id=770769
const MAX_SCREENSHOT_HEIGHT = 16384;

/**
* @param {string} str
Expand All @@ -24,12 +24,26 @@ function snakeCaseToCamelCase(str) {
}

class FullPageScreenshot extends Gatherer {
/**
* @param {Driver} driver
* @return {Promise<number>}
* @see https://bugs.chromium.org/p/chromium/issues/detail?id=770769
*/
async getMaxScreenshotHeight(driver) {
return await driver.evaluate(pageFunctions.getMaxTextureSize, {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@connorjclark connorjclark Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! :)

args: [],
useIsolation: true,
deps: [],
});
}

/**
* @param {LH.Gatherer.PassContext} passContext
* @return {Promise<LH.Artifacts.FullPageScreenshot['screenshot']>}
*/
async _takeScreenshot(passContext) {
const driver = passContext.driver;
const maxScreenshotHeight = await this.getMaxScreenshotHeight(driver);
const metrics = await driver.sendCommand('Page.getLayoutMetrics');

// Width should match emulated width, without considering content overhang.
Expand All @@ -38,8 +52,8 @@ class FullPageScreenshot extends Gatherer {
// Note: If the page is zoomed, many assumptions fail.
//
// Height should be as tall as the content. So we use contentSize.height
const width = Math.min(metrics.layoutViewport.clientWidth, MAX_SCREENSHOT_HEIGHT);
const height = Math.min(metrics.contentSize.height, MAX_SCREENSHOT_HEIGHT);
const width = Math.min(metrics.layoutViewport.clientWidth, maxScreenshotHeight);
const height = Math.min(metrics.contentSize.height, maxScreenshotHeight);

await driver.sendCommand('Emulation.setDeviceMetricsOverride', {
// If we're gathering with mobile screenEmulation on (overlay scrollbars, etc), continue to use that for this screenshot.
Expand Down Expand Up @@ -173,4 +187,3 @@ class FullPageScreenshot extends Gatherer {
}

module.exports = FullPageScreenshot;
module.exports.MAX_SCREENSHOT_HEIGHT = MAX_SCREENSHOT_HEIGHT;
21 changes: 21 additions & 0 deletions lighthouse-core/lib/page-functions.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,26 @@ function getOuterHTMLSnippet(element, ignoreAttrs = [], snippetCharacterLimit =
}
}

/**
* Get the maximum size of a texture the GPU can handle
* @see https://bugs.chromium.org/p/chromium/issues/detail?id=770769#c13
*/
/* istanbul ignore next */
function getMaxTextureSize() {
try {
let canvas = document.createElement('canvas');
let gl = canvas.getContext('webgl');
const maxTextureSize = gl.getParameter(gl.MAX_TEXTURE_SIZE);
canvas = gl = undefined; // Cleanup for GC
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return maxTextureSize;
} catch (e) {
// If the above fails for any reason we need a fallback number;
// 4096 is the max texture size on a Pixel 2 XL, so to be conservative we'll use a low value like it.
// But we'll subtract 1 just to identify this case later on.
const MAX_TEXTURE_SIZE_FALLBACK = 4095;
return MAX_TEXTURE_SIZE_FALLBACK;
}
}

/**
* Computes a memory/CPU performance benchmark index to determine rough device class.
Expand Down Expand Up @@ -521,6 +541,7 @@ module.exports = {
getOuterHTMLSnippet: getOuterHTMLSnippet,
computeBenchmarkIndex: computeBenchmarkIndex,
computeBenchmarkIndexString: computeBenchmarkIndex.toString(),
getMaxTextureSize,
getNodeDetailsString,
getNodeDetails,
getNodePathString: getNodePath.toString(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@

const FullPageScreenshotGatherer = require('../../../gather/gatherers/full-page-screenshot.js');

// Headless's default value is (1024 * 16), but this varies by device
const maxTextureSizeMock = 1024 * 8;

/**
* @param {{contentSize: {width: number, height: number}, screenSize: {width?: number, height?: number, dpr: number}, screenshotData: string[]}}
*/
Expand All @@ -17,6 +20,8 @@ function createMockDriver({contentSize, screenSize, screenshotData}) {
evaluate: async function(fn) {
if (fn.name === 'resolveNodes') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this so much nicer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return {};
} if (fn.name === 'getMaxTextureSize') {
return maxTextureSizeMock;
} else if (fn.name === 'getObservedDeviceMetrics') {
return {
width: screenSize.width,
Expand Down Expand Up @@ -192,7 +197,7 @@ describe('FullPageScreenshot gatherer', () => {
'Emulation.setDeviceMetricsOverride',
expect.objectContaining({
deviceScaleFactor: 1,
height: FullPageScreenshotGatherer.MAX_SCREENSHOT_HEIGHT,
height: maxTextureSizeMock,
})
);
});
Expand Down