1529182 - Intermittent svg/text-stroke-scaling-02a.html == svg/text-stroke-scaling-02-ref.html | image comparison, max difference: 59, number of differing pixels: 1

Status: RESOLVED FIXED

(Core :: SVG, defect, P5)

Description

[Treeherder Bug Filer]

#[markdown(off)]
Filed by: rmaries [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=229383744&repo=mozilla-central

https://queue.taskcluster.net/v1/task/OMmWozN_TJSeO0hyFhmBsQ/runs/0/artifacts/public/logs/live_backing.log

https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/OMmWozN_TJSeO0hyFhmBsQ/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 1

[Alex Henrie]

Attachment: [PATCH] Recalculate context scale when inflating mRect of SVG text

Well, we know now that rounding wasn't the problem. The next thing I'd like to try is making sure that the most current scale factor is applied. Unfortunately, I can't reproduce this test failure on either a local Windows 10 machine or on the Try server, so I can't confirm beforehand that the attached patch will fix the problem. We'll just have to commit the change and see if the failure disappears from the automated builds.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a883fc9efa7fb33b3c49a1579cd28e358dc633b

Comment 2

[Alex Henrie]

Requesting checkin of attachment 9045859 [details] [diff] [review]

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27284783216c
Recalculate context scale when inflating mRect of SVG text. r=longsonr

Comment 4

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/27284783216c

Comment 5

[Razvan Maries]

This issue keeps occurring after fix:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=229962659&searchStr=windows%2C10%2Cx64%2Cccov%2Cdebug%2Creftests%2Cwith%2Ce10s%2Ctest-windows10-64-ccov%2Fdebug-reftest-e10s-4%2Cr-e10s%28r4%29

Comment 6

[Robert Longson [:longsonr]]

I suggest backing this out and then multiplying the Inflate value by some value > 1. We could start with 1.5 say, the ceil would not be required either then.

Comment 7

[Razvan Maries]

Backout: https://hg.mozilla.org/mozilla-central/rev/ca0ea512f874390477cb4595d9c36c8106296820

Comment 8

[Alex Henrie]

Attachment: [PATCH] Inflate mRect of SVG text by 1.5 device pixels

(In reply to Robert Longson [:longsonr] from comment #6)

I suggest backing this out and then multiplying the Inflate value by some value > 1. We could start with 1.5 say, the ceil would not be required either then.

Sure, let's do that.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2878bb51a33b2ddf1b12a57a426f9d052df1afc

Comment 9

[Robert Longson [:longsonr]]

Comment on attachment 9046097 [details] [diff] [review]
[PATCH] Inflate mRect of SVG text by 1.5 device pixels

This is fine.

Comment 10

[Alex Henrie]

Requesting checkin of attachment 9046097 [details] [diff] [review]

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f39455c02333
Inflate mRect of SVG text by 1.5 device pixels. r=longsonr

Comment 12

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&revision=f39455c023332f4aebd8542a147f0c67228861cb&selectedJob=230094918

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230094918&repo=mozilla-inbound&lineNumber=3103
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230094918&repo=mozilla-inbound&lineNumber=3103

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8f6c0ffb8a88cbb9ef8490dfea3020a5fddce8

19:33:48 INFO - TEST-PASS | dom/svg/test/test_bounds.html | svg3.getBoundingClientRect().height
19:33:48 INFO - Buffered messages finished
19:33:48 INFO - TEST-UNEXPECTED-FAIL | dom/svg/test/test_bounds.html | text1.getBoundingClientRect().left - got 22.5, expected 24 ± 1
19:33:48 INFO - SimpleTest.ok@SimpleTest/SimpleTest.js:275:18
19:33:48 INFO - isWithAbsTolerance@dom/svg/test/test_bounds.html:51:3
19:33:48 INFO - runTest@dom/svg/test/test_bounds.html:102:3
19:33:48 INFO - EventListener.handleEvent*@dom/svg/test/test_bounds.html:289:8
19:33:48 INFO - TEST-PASS | dom/svg/test/test_bounds.html | text2.getBoundingClientRect().left

Comment 13

[Alex Henrie]

Attachment: [PATCH] Correctly apply scale factor when clamping SVG text size

While reading through SVGTextFrame.cpp again, I noticed a problem in SVGTextFrame::UpdateFontSizeScaleFactor:

double minTextRunSize = minSize * contextScale;
double maxTextRunSize = maxSize * contextScale;

if (minTextRunSize >= CLAMP_MIN_SIZE && maxTextRunSize <= CLAMP_MAX_SIZE) {
// We are already in the ideal font size range for all text frames,
// so we only have to take into account the contextScale.
mFontSizeScaleFactor = contextScale;
} else if (maxSize / minSize > CLAMP_MAX_SIZE / CLAMP_MIN_SIZE) {
// We can't scale the font sizes so that all of the text frames lie
// within our ideal font size range, so we treat the minimum as more
// important and just scale so that minSize = CLAMP_MIN_SIZE.
mFontSizeScaleFactor = CLAMP_MIN_SIZE / minTextRunSize;
} else if (minTextRunSize < CLAMP_MIN_SIZE) {
mFontSizeScaleFactor = CLAMP_MIN_SIZE / minTextRunSize;
} else {
mFontSizeScaleFactor = CLAMP_MAX_SIZE / maxTextRunSize;
}

See how in the first case, contextScale is in the numerator of mFontSizeScaleFactor, but in the other three cases it's in the denominator? That can't be right. I don't know if fixing that will help with this regression, but it needs attention.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3452c617af082cd4c2c1eb42a1e2223cf4faeea

Comment 14

[Robert Longson [:longsonr]]

When it was originally written it looked like this: https://dxr.mozilla.org/mozilla2.0/source/layout/svg/base/src/nsSVGGlyphFrame.cpp#1588

I think it was correct back then.

Comment 15

[Robert Longson [:longsonr]]

In case you didn't know, if you give this bug a keyword of leave-open it won't be marked fixed automatically.

Comment 16

[Alex Henrie]

Attachment: [PATCH v2] Correctly apply scale factor when clamping SVG text size

(In reply to Robert Longson [:longsonr] from comment #14)

When it was originally written it looked like this: https://dxr.mozilla.org/mozilla2.0/source/layout/svg/base/src/nsSVGGlyphFrame.cpp#1588

I think it was correct back then.

You're right, that does make more sense. The clamping is supposed to happen after multiplying by the context scale. Thanks for pointing that out!

For this patch, I considered writing contextScale * CLAMP_MIN_SIZE / minTextRunSize, but since that could introduce more rounding errors and the comment already says "scale so that minSize = CLAMP_MIN_SIZE", I decided to use the simplified formula CLAMP_MIN_SIZE / minSize.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4192ed095e1aee6a05d4869919c658fa712c646

Comment 17

[Alex Henrie]

(In reply to Robert Longson [:longsonr] from comment #15)

In case you didn't know, if you give this bug a keyword of leave-open it won't be marked fixed automatically.

I didn't know that. Thanks for the tip!

Comment 18

[Intermittent Failures Robot]

6 failures in 2602 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 6

Platform breakdown:

• windows10-64-ccov: 6

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1529182&startday=2019-02-18&endday=2019-02-24&tree=all

Comment 19

[Andreea Pavel [:apavel]]

Robert, can this be landed?

Comment 20

[Alex Henrie]

Requesting checkin of attachment 9046240 [details] [diff] [review]

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bcc26b5ecf7
Correctly apply scale factor when clamping SVG text size. r=longsonr

Comment 22

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2bcc26b5ecf7

Comment 23

[Intermittent Failures Robot]

3 failures in 2981 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 3

Platform breakdown:

• windows10-64-ccov: 3

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1529182&startday=2019-02-25&endday=2019-03-03&tree=all

Comment 24

[Alex Henrie]

Well, we're making progress: The test has only failed once since the last patch was applied.

Debugging the SVG <text> code is particularly difficult for me because it uses at least six coordinate spaces, and I can't figure out how they relate to each other:

• Run space
• Run user space
• Frame user space
• (Regular?) user space
• GFX space
• App space

Robert, is this something you could explain to me?

Comment 25

[Robert Longson [:longsonr]]

https://groups.google.com/forum/#!forum/mozilla.dev.platform is probably the best place to ask. Someone there may point you in the right direction.

Comment 26

[Intermittent Failures Robot]

6 failures in 2818 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 6

Platform breakdown:

• windows10-64-ccov: 6

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1529182&startday=2019-03-04&endday=2019-03-10&tree=all

Comment 27

[Alex Henrie]

I sent an email to [e-mail] a week ago today but I have not received a reply. Is there anyone else I can talk to?

Comment 28

[Robert Longson [:longsonr]]

can you help Cam?

Comment 29

[Intermittent Failures Robot]

5 failures in 2771 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 5

Platform breakdown:

• windows10-64-ccov: 5

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1529182&startday=2019-03-11&endday=2019-03-17&tree=all

Comment 30

[Intermittent Failures Robot]

5 failures in 2561 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 5

Platform breakdown:

• windows10-64-ccov: 5

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1529182&startday=2019-03-18&endday=2019-03-24&tree=all

Comment 31

[Intermittent Failures Robot]

10 failures in 3910 pushes (0.003 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 10

Platform breakdown:

• windows10-64-ccov: 10

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1529182&startday=2019-03-25&endday=2019-03-31&tree=all

Comment 32

[Cameron McCormack (:heycam)]

Sorry for the delay; I just saw the email get through today. I replied on the list.

Comment 33

[Alex Henrie]

Attachment: Bug 1529182 - Include stroke in SVG <text> bounding box.

I think I found another mistake in the SVG text code that could explain the regression: If the text is drawn with a stroke, shouldn't the stroke be included in the bounding box?

Comment 34

[Alex Henrie]

Requesting checkin of https://phabricator.services.mozilla.com/D26074

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7f603959f6c
Include stroke in SVG <text> bounding box. r=longsonr,heycam

Comment 36

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/f7f603959f6c

Comment 37

[Alex Henrie]

Looks like that fixed it. Robert and Cameron, thanks for the help!