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

RESOLVED FIXED in Firefox 68

Status

()

defect
P5
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: intermittent-bug-filer, Assigned: alexhenrie24)

Tracking

({intermittent-failure, regression})

unspecified
mozilla68
Points:
---

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 unaffected, firefox67 wontfix, firefox68 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Updated

3 months ago
See Also: → 1521425
Assignee

Comment 1

3 months ago

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

Assignee: nobody → alexhenrie24
Attachment #9045859 - Flags: review?(longsonr)
Attachment #9045859 - Flags: review?(longsonr) → review+
Assignee

Comment 2

3 months ago
Keywords: checkin-needed

Comment 3

3 months ago

Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27284783216c
Recalculate context scale when inflating mRect of SVG text. r=longsonr

Keywords: checkin-needed

Comment 4

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Updated

3 months ago

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.

Assignee

Comment 8

3 months ago

(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

Attachment #9045859 - Attachment is obsolete: true
Flags: needinfo?(alexhenrie24)
Attachment #9046097 - Flags: review?(longsonr)
Comment on attachment 9046097 [details] [diff] [review]
[PATCH] Inflate mRect of SVG text by 1.5 device pixels

This is fine.
Attachment #9046097 - Flags: review?(longsonr) → review+
Assignee

Comment 10

3 months ago
Keywords: checkin-needed

Comment 11

3 months ago

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f39455c02333
Inflate mRect of SVG text by 1.5 device pixels. r=longsonr

Keywords: checkin-needed

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

Flags: needinfo?(alexhenrie24)
Assignee

Comment 13

3 months ago

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

Attachment #9046097 - Attachment is obsolete: true
Flags: needinfo?(alexhenrie24)
Attachment #9046229 - Flags: review?(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.

Attachment #9046229 - Flags: review?(longsonr) → review+

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

Assignee

Comment 16

3 months ago

(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

Attachment #9046229 - Attachment is obsolete: true
Attachment #9046240 - Flags: review?(longsonr)
Assignee

Comment 17

3 months ago

(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!

Keywords: leave-open
Comment hidden (Intermittent Failures Robot)
Attachment #9046240 - Flags: review?(longsonr) → review+

Robert, can this be landed?

Flags: needinfo?(longsonr)
Assignee

Comment 20

3 months ago
Keywords: checkin-needed
Flags: needinfo?(longsonr)

Comment 21

3 months ago

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bcc26b5ecf7
Correctly apply scale factor when clamping SVG text size. r=longsonr

Keywords: checkin-needed
Comment hidden (Intermittent Failures Robot)
Assignee

Comment 24

3 months ago

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?

Flags: needinfo?(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.

Flags: needinfo?(longsonr)
Comment hidden (Intermittent Failures Robot)
Assignee

Comment 27

2 months ago

I sent an email to dev-platform@lists.mozilla.org a week ago today but I have not received a reply. Is there anyone else I can talk to?

Flags: needinfo?(longsonr)

can you help Cam?

Flags: needinfo?(longsonr) → needinfo?(cam)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)

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

Flags: needinfo?(cam)
Assignee

Comment 33

2 months ago

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?

Assignee

Comment 34

2 months ago
Keywords: checkin-needed

Comment 35

2 months ago

Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7f603959f6c
Include stroke in SVG <text> bounding box. r=longsonr,heycam

Keywords: checkin-needed
Assignee

Comment 37

2 months ago

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

Status: REOPENED → RESOLVED
Last Resolved: 3 months ago2 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: mozilla67 → mozilla68
Assignee

Updated

2 months ago
See Also: → 1519144
You need to log in before you can comment on or make changes to this bug.