1821260 - Figure out what value we want to use for bounds checking active SVG

Status: RESOLVED INVALID

(Core :: Graphics: WebRender, defect)

Description

[Robert Longson [:longsonr]]

If we have a 100px x 100px rect

aBounds.width=6000, aBounds.height=6000 aSc.GetInheritedScale().xScale = 1, aSc.GetInheritedScale().xScale = 1, width and height = 6000 i.e. > largeish.

So aBounds is in app units but it seems that largeish may be in pixels?

So the line

const float largeish = 512;

Should probably have been

const float largeish = 512 * AppUnitsPerCSSPixel();

As we currently make all SVG > 8.5333px active.

Do we

a) change largeish to const float largeish = 512 * AppUnitsPerCSSPixel();

b) change largeish to const float largeish = 8.5 * AppUnitsPerCSSPixel();

And should we at the same time change the calculation so that it checks width x height i.e. change largeish to largeishArea. That way something narrow and tall or vice versa would be active.

Comment 1

[Robert Longson [:longsonr]]

Attachment: Bug 1821260 - correct use of largeish r=tnikkel

Comment 2

[Robert Longson [:longsonr]]

Oh and how do we deal with the performance fallout if we choose a)? It will regress google-slides in browsertime and likely others.

Comment 3

[Timothy Nikkel (:tnikkel)]

The thin case is partially handled by bug 1780191 (I need to write a test for that so it can land).

So the 512 figure originally comes from bug 1761770, which moved us from making all svg geom/image items active that wanted to be active, to giving us a lower bound to those we make active. In that context an 8 px lower bound does not seem unreasonable. There isn't much discussion in the bug about the rationale. I'd like to hear from nical on how he choose this figure and his thoughts on tweaking it. (He's on parental leave right now, but this doesn't seem urgent since this just came from looking at the code, not from an example where we perform badly.)

Comment 4

[Nicolas Silva [:nical]]

Should probably have been
const float largeish = 512 * AppUnitsPerCSSPixel();

Absolutely, that was an oversight.

I'd like to hear from nical on how he choose this figure and his thoughts on tweaking it.

Tweaking is definitely encouraged. 512px felt small enough that causing extra layerization would likely do more harm than good, but there wasn't much methodology in picking the threshold, other than testing on a few websites (google slides in particular).