Fix incorrect usage of ToNearestPixels in FrameLayerBuilder and elsewhere

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

({regression})

unspecified
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(6 attachments, 2 obsolete attachments)

7.83 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.20 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.36 KB, patch
Details | Diff | Splinter Review
2.16 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
18.22 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.02 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
In FrameLayerBuilder in a couple of places we do
  nsIntRect itemVisibleRect =
    aItem->GetVisibleRect().ToNearestPixels(appUnitsPerDevPixel);

This code is assuming that when the item draws, whatever it draws will be
snapped to pixel boundaries. But this assumption is overoptimistic for a couple
of reasons:
-- Some content, such as an element transformed by rotation, simply doesn't
snap its drawing.
-- If the context has a tricky transform on it when we come to draw the
content, we might not snap.

Using ToOutsidePixels here would be the conservative approach and it would give
correct results. However it would mean that for content that *does* snap, but
is not already aligned at pixel boundaries, the visible rect is bigger than the
area that actually gets drawn, so a layer containing only that content could no
longer be treated as opaque. That could be bad.

In BasicLayers we have a couple of places where we do
gfxUtils::ClipToRegionSnapped that have related issues.
Blocks: 579808
blocking2.0: --- → ?
Created attachment 494632 [details] [diff] [review]
Part 1: Be conservative when converting visible regions and opaque regions from appunits to pixels
Attachment #494632 - Flags: review?(tnikkel)
Created attachment 494633 [details] [diff] [review]
Part 2: Remove some usage of gfxUtils::ClipToRegionSnapped since snapping the visible region may not be correct
Attachment #494633 - Flags: review?(jmuizelaar)
Attachment #494633 - Flags: review?(jmuizelaar) → review+
Created attachment 494909 [details] [diff] [review]
Part 3: Snap bounds of border and background display items to pixels if we're sure they will be snapped

The previous two patches are conservative and correct. But they can lead to regressions because we've lost information about what's snapped. When a display item draws a snapped rectangle, we can give it a smaller rectangle for the visible nsIntRegion, and a bigger rectangle for the opaque nsIntRegion. This patch adjusts GetBounds and GetOpaqueRegion to return the snapped region if we know the display item is not transformed.

The patch also modifies PresShell::RenderDocument so we tell nsDisplayListBuilder we're in a transform if we're being asked to draw into a context which has a transform (other than a straight transform by pixels).
Attachment #494909 - Flags: review?(tnikkel)
Created attachment 495462 [details] [diff] [review]
Part 1 v2

Fixes a regression in this patch. When converting a ThebesLayer to a ColorLayer we need to ensure that not only is the display item a uniform color inside its bounds, but that those bounds contain the entire visible rect of pixels for the item. If the bounds aren't pixel-aligned there might be a row or column of pixels that aren't fully covered.
Attachment #495462 - Flags: review?(tnikkel)
Attachment #494632 - Attachment is obsolete: true
Attachment #494632 - Flags: review?(tnikkel)
Created attachment 495649 [details] [diff] [review]
Part 4: mark tests as passing

Something in my patch queue --- I think the fixes in this bug --- makes the abspos-*.html transform tests pass with D2D.
Created attachment 496318 [details] [diff] [review]
Part 5: Don't abort in RoundedRectIntersectsRect

Part 3 means the display item GetBounds is not always the frame border-box, so occasionally we can get an abort in RoundedRectIntersectsRect. We should just return false there instead.
Attachment #496318 - Flags: review?(dbaron)
Comment on attachment 496318 [details] [diff] [review]
Part 5: Don't abort in RoundedRectIntersectsRect

r=dbaron
Attachment #496318 - Flags: review?(dbaron) → review+
Blocks a blocker, and fixes regressions too.
blocking2.0: ? → final+
Created attachment 497754 [details] [diff] [review]
Part 3 v2

Snap nsDisplayClip too
Attachment #497754 - Flags: review?(tnikkel)
Created attachment 498662 [details] [diff] [review]
Part 1.5: Fix drawing of mForcedBackgroundColor to only fill the bounds of the visible region

It's not correct to completely fill the destination context with the
mForcedBackgroundColor. FindOpaqueBackgroundColorFor only checks that
the bounds of the layer's visible region can be filled with the color.
Drawing outside the bounds of the visible region may not necessarily
be ignored (see comments on Layer::SetVisibleRegion).

(This is an existing bug revealed by part 2.)
Attachment #498662 - Flags: review?(tnikkel)
Attachment #498662 - Flags: review?(tnikkel) → review+
Comment on attachment 494909 [details] [diff] [review]
Part 3: Snap bounds of border and background display items to pixels if we're sure they will be snapped

I assume this is obsolete now?
Comment on attachment 494909 [details] [diff] [review]
Part 3: Snap bounds of border and background display items to pixels if we're sure they will be snapped

Yes
Attachment #494909 - Attachment is obsolete: true
Attachment #494909 - Flags: review?(tnikkel)
Could you push your patch queue to your users repo? I will probably want to look at FrameLayerBuilder in the context of your other pending changes.
Done.
Attachment #495462 - Flags: review?(tnikkel) → review+
Attachment #497754 - Flags: review?(tnikkel) → review+
Whiteboard: [needs landing]
Blocks 579808, which is a regression.
Keywords: regression
A tryserver build with these patches was green (modulo known random orange) on a tryserver build with "-b d -p all -t none -u all":
http://hg.mozilla.org/try/pushloghtml?changeset=b3f754733e39
I think this is safe to land.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/a955dcb369f6
http://hg.mozilla.org/mozilla-central/rev/9fb07b18a3a5
http://hg.mozilla.org/mozilla-central/rev/bf71941d1aee
http://hg.mozilla.org/mozilla-central/rev/1ca912846884
http://hg.mozilla.org/mozilla-central/rev/a61970a01de4
http://hg.mozilla.org/mozilla-central/rev/ca890a13c598
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]

Updated

7 years ago
Depends on: 623606
Blocks: 598575

Comment 18

7 years ago
Something in Parts 1.5 - 5 has caused <https://bugzilla.mozilla.org/show_bug.cgi?id=363861#c145>, while Part 1 alone fixes Bug 598575. Would you like to treat this in a new bug or would you like to investigate the problem here?

Comment 19

7 years ago
(In reply to comment #18)
> Something in Parts 1.5 - 5 has caused
> <https://bugzilla.mozilla.org/show_bug.cgi?id=363861#c145>

While backout of these checkins helped when printing to PDF my own reduced testcase, this didn't restore printing of semi-transparent areas of a real-life case <http://weblogs.mozillazine.org/asa/archives/2010/12/visualizing_plugin_m.html>. Even if these patches contributed to the new problem, they are not alone responsible.

My apologies.
Depends on: 646757
Depends on: 646037
Depends on: 714346
You need to log in before you can comment on or make changes to this bug.