Closed Bug 1342571 Opened 3 years ago Closed 3 years ago

Shrink dashed DrawTargetSkia::StrokeRect where possible

Categories

(Core :: Graphics, defect, P3)

52 Branch
defect

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)

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)
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.
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 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?
(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.
Attachment #8841107 - Flags: review?(mstange) → review+
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+
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
https://hg.mozilla.org/mozilla-central/rev/129546a71da9
https://hg.mozilla.org/mozilla-central/rev/1a8bcf69e88f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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?
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 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+
Attachment #8842160 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.