Closed
Bug 1342571
Opened 7 years ago
Closed 7 years ago
Shrink dashed DrawTargetSkia::StrokeRect where possible
Categories
(Core :: Graphics, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox51 | --- | unaffected |
firefox52 | --- | affected |
firefox-esr52 | --- | affected |
firefox53 | --- | fixed |
firefox54 | --- | fixed |
People
(Reporter: lsalzman, Assigned: lsalzman)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(2 files, 1 obsolete file)
1.84 KB,
patch
|
mstange
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
5.28 KB,
patch
|
lsalzman
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
As we found during the investigation of bug 1303094, using StrokeRect in Skia on dashed paths that can cause excessive memory usage due to Skia not clipping the rect before allocating memory for all the potential dashes. Markus Stange made a workaround for a similar issue for DrawTargetCG, so let's just reuse that code here since there are still some leftover smaller cases of StrokeRect with dashes (like in SVG) that would benefit from this. Skia luckily at least already culls StrokeLine requests, so this just backports the StrokeRect parts.
Attachment #8841107 -
Flags: review?(mstange)
Comment 1•7 years ago
|
||
There was a regression from that patch: bug 1198307. There's a patch there but it wasn't confirmed whether it would fix the bug. We should probably make sure that we have a fix for that bug before we land this.
Assignee | ||
Comment 2•7 years ago
|
||
Regarding the regression in bug 1198307, it was observed that this only happened with a stroke width of 0.5px. The reason this is problematic is that MaxStrokeExtents returned non-integral values in that case. When computing the final shrunken rect which used MaxStrokeExtents enforce a buffer around the shrinking, it could still end up with a fractional pixel coverage that rounds back up to full pixel coverage during rendering (especially hairlines). So, the fix is to make sure MaxStrokeExtents rounds up its coverage estimate, since, well, you can't write to part of a pixel, as much we wish we could. :)
Attachment #8841878 -
Flags: review?(mstange)
Comment 3•7 years ago
|
||
Comment on attachment 8841878 [details] [diff] [review] fix MaxStrokeExtents to account for partial pixel coverage properly Review of attachment 8841878 [details] [diff] [review]: ----------------------------------------------------------------- Is this the right place to do the rounding? Doesn't it need to be done on the resulting rect in device pixels?
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #3) > Comment on attachment 8841878 [details] [diff] [review] > fix MaxStrokeExtents to account for partial pixel coverage properly > > Review of attachment 8841878 [details] [diff] [review]: > ----------------------------------------------------------------- > > Is this the right place to do the rounding? Doesn't it need to be done on > the resulting rect in device pixels? The problem is that multiple places use MaxStrokeExtents, and they are all vulnerable to the same bug. So the choice was either fix every caller of MaxStrokeExtents to round up, or fix MaxStrokeExtents itself. It makes sense at least for MaxStrokeExtents to return its result in pixel units.
Updated•7 years ago
|
Attachment #8841107 -
Flags: review?(mstange) → review+
Comment 5•7 years ago
|
||
Comment on attachment 8841878 [details] [diff] [review] fix MaxStrokeExtents to account for partial pixel coverage properly Review of attachment 8841878 [details] [diff] [review]: ----------------------------------------------------------------- I agree now that the rounding in this place is probably necessary. I think that in some places we might also need to round out the resulting device-space rectangle, but at least in the UserSpaceStrokeClip case it's not necessary because aDeviceClip is already device-aligned, so it'll still be device-aligned after an integral margin is applied to it, so we don't need to round it again.
Attachment #8841878 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 6•7 years ago
|
||
Made UserSpaceStrokeClip take an IntRect for the deviceClip to make it clear we expect it to be pixel aligned. Also take the SkIRect device clip bounds from the SkCanvas when we can.
Attachment #8841107 -
Attachment is obsolete: true
Attachment #8842160 -
Flags: review+
Pushed by lsalzman@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/129546a71da9 shrink dashed DrawTargetSkia::StrokeRect where possible. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/1a8bcf69e88f fix MaxStrokeExtents to account for partial pixel coverage properly. r=mstange
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/129546a71da9 https://hg.mozilla.org/mozilla-central/rev/1a8bcf69e88f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Updated•7 years ago
|
status-firefox51:
--- → unaffected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox-esr52:
--- → affected
Assignee | ||
Comment 9•7 years ago
|
||
Comment on attachment 8841878 [details] [diff] [review] fix MaxStrokeExtents to account for partial pixel coverage properly Approval Request Comment [Feature/Bug causing the regression]: Switching to Skia content on Android/Linux/Mac (51) and on Windows (52) [User impact if declined]: In bug 1303094, we dealt with the primary cause of OOMs arising from drawing CSS dotted borders in web pages. However, the underlying cause (our implementation of DrawTargetSkia::StrokeRect) still remains which may be exposed by other less prominent use cases, such as SVGs. So it is conceivable that other large OOMs may be caused, though at a lower volume than the ones addressed in bug 1303094, if this is not addressed. [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: [Is the change risky?]: no [Why is the change risky/not risky?]: Originally this was code we deployed already last year for Mac. We are just adapting this to work with Skia on other platforms as well. Only one regression was found, which we have explicitly dealt with in another patch in this same bug. [String changes made/needed]: None
Attachment #8841878 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 10•7 years ago
|
||
Comment on attachment 8842160 [details] [diff] [review] shrink dashed DrawTargetSkia::StrokeRect where possible. r=mstange Approval Request Comment [Feature/Bug causing the regression]: Switching to Skia content on Android/Linux/Mac (51) and on Windows (52) [User impact if declined]: In bug 1303094, we dealt with the primary cause of OOMs arising from drawing CSS dotted borders in web pages. However, the underlying cause (our implementation of DrawTargetSkia::StrokeRect) still remains which may be exposed by other less prominent use cases, such as SVGs. So it is conceivable that other large OOMs may be caused, though at a lower volume than the ones addressed in bug 1303094, if this is not addressed. [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: [Is the change risky?]: no [Why is the change risky/not risky?]: Originally this was code we deployed already last year for Mac. We are just adapting this to work with Skia on other platforms as well. Only one regression was found, which we have explicitly dealt with in another patch in this same bug. [String changes made/needed]: None
Attachment #8842160 -
Flags: approval-mozilla-aurora?
Comment 11•7 years ago
|
||
Comment on attachment 8841878 [details] [diff] [review] fix MaxStrokeExtents to account for partial pixel coverage properly Sounds good to fix more OOMs, let's uplift to aurora.
Attachment #8841878 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•7 years ago
|
Attachment #8842160 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 12•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/2dcb9a997569 https://hg.mozilla.org/releases/mozilla-aurora/rev/dd529ec5fef6
You need to log in
before you can comment on or make changes to this bug.
Description
•