633344 - Revert to exact resolution for static transformed ThebesLayers

Status: RESOLVED DUPLICATE of bug 637852

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In bug 586683 we clamp the scale factors we use to set resolution to a power of 2, to avoid rerendering too much as the scale factor changes. But if the transform is static we shouldn't clamp, we should use the optimal resolution.

This should be a softblocker, it'll be a quality regression over 3.6.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the best way to fix this might be to add a new flag on layers "transform is active", only clamp if that flag is set, and set the flag from nsDisplayTransform when the transform is known to be active there.

I'm not sure how this should interact with shadowlayers and their transforms though. Should we have them set the flag too, and possibly back out the patch in bug 633040 because it should be fixed by this bug?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

The text is all fuzzy. It shouldn't be.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I think the patch in bug 633040 is still correct.  BasicManager resolution is for all intents and purposes always an "exact resolution", so I don't think it would affect the "active flag".  However we twiddle scales on active/inactive transformed *ShadowableLayers (et al.) doesn't affect *ShadowLayers either because the shadows just draw buffers that have a final computed resolution attached to them, they don't care how the resolution was computed.  But maybe I misunderstand something.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Matt, can you work on this? I expect it should be quite easy.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add IsActiveTransform interface to container layers

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: BasicLayers can draw at exact resolutions for inactive transforms

WIP, going to bed.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: BasicLayers can draw at exact resolutions for inactive transforms v2

Clipping needs to happen before we scale the context. This is really ugly, any ideas how we can do this cleanly?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-    float paintXRes = BasicManager()->XResolution() * gfxUtils::ClampToScaleFactor(scale.width);
-    float paintYRes = BasicManager()->YResolution() * gfxUtils::ClampToScaleFactor(scale.height);
+    float paintXRes = BasicManager()->XResolution() * scale.width;
+    float paintYRes = BasicManager()->YResolution() * scale.height;
+    if (GetParent() && GetParent()->IsTransformActive()) {
+      //Parent (container) has an active transform, clamp to scale factors, otherwise paint at exact resolution.
+      paintXRes = gfxUtils::ClampToScaleFactor(paintXRes);
+      paintYRes = gfxUtils::ClampToScaleFactor(paintYRes);

Isn't this going to regress 633040 again, because we're going to go back to clamping the resolution * scale instead of just the scale?

Comment 9

[Matt Woodrow (:mattwoodrow)]

Only while the transform component is actively changing! Probably worth fixing though :)

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: BasicLayers can draw at exact resolutions for inactive transforms v3

Only clamp the transform component.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Add drawing at exact resolution for OGL layers

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't understand the clipping changes in the OGL code here. Why are you adding a new clipping call? Why aren't we doing the "transform, round out, clip, inverse transform, round out, clip" dance?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems we won't be taking this for desktop FF4.

Comment 14

[Matt Woodrow (:mattwoodrow)]

We already do this to pass a scaled region into BeginUpdate. Though the second clip should be moved outside of the gfxASurface::CONTENT_COLOR_ALPHA if condition.

Comment 15

[Benjamin Stover (:stechz)]

This should hard block Fennec because it causes corruption for Fennec, which tends to always render at non-identity resolutions.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #13)
> It seems we won't be taking this for desktop FF4.

Robert, we'd like to take this for Fennec. Any technical issues or risk with landing it (regarding FF4)?

Comment 17

[Benjamin Stover (:stechz)]

The smallest fix would be to stop clamping and stop using ExtendToScale with some sort of mobile-only ifdef. That's all we care about.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Fennec needs this so we stop thrashing ashm memory

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 514752 [details] [diff] [review]
Part 1: Add IsActiveTransform interface to container layers

for fennec

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 515401 [details] [diff] [review]
Part 2: BasicLayers can draw at exact resolutions for inactive transforms v3

for fennec

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 514752 [details] [diff] [review]
Part 1: Add IsActiveTransform interface to container layers

drivers thought an sr is all that is needed

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 515401 [details] [diff] [review]
Part 2: BasicLayers can draw at exact resolutions for inactive transforms v3

drivers thought an sr is all that is needed

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 515401 [details] [diff] [review]
Part 2: BasicLayers can draw at exact resolutions for inactive transforms v3

No problem taking this for FF4 if Fennec needs it.

Comment 24

[Benjamin Stover (:stechz)]

This patch breaks some things in Fennec. STR:
1. Go to news.google.com
2. Scroll down to Google Fast Flip
3. Hit the arrows

We are busting the assertion that the region to draw bounds be a subset of quadrantRect because we are rounding out the region in the GetContext call. The new region to draw gets passed to BasicLayers.cpp, and when it calls GetContext with the rounded out version which it seems messes up the calculation for what quadrant we are in.

Also, moving clipping into GetContext isn't exactly equivalent. We don't apply the clip in BasicLayers.cpp in the content process for non-identity resolutions for better or worse. I've seen that this causes problems with your patch rebased for patches in 635373 (now landed) and 635035. It seems that the regions we are passing back and forth for the buffers aren't working quite properly.

Comment 25

[Benjamin Stover (:stechz)]

So on, mozilla-central, if I force clipping on in BasicLayers.cpp I see this rendering issue. Hm.

Comment 26

[Stuart Parmenter]

Trying to understand if we still need/want this bug.  roc/mattwoodrow can you comment?

Comment 27

[Matt Woodrow (:mattwoodrow)]

On basic layers (where the layer manager has a specified resolution), we don't clamp this value (to avoid the fuzzy text) and end up with non integer/reciprocal values. ExtendForScaling can't actually handle these, and we get assertions and unknown results.

So yes, I believe we need this for fennec.

These patches were ready to land, but we need to investigate the broken rendering the Ben is seeing.

Comment 28

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: BasicLayers can draw at exact resolutions for inactive transforms v4

Removed the now unnecessary call to ExtendForScaling

Carrying forward r/sr=roc.

I see the "Messed up quadrants" assertion even without this patch, I don't think this is related (the assertion checks the region before we modify it).

I can't reproduce any rendering glitches either, but I can't see a "Google Fast Flip" option on news.google.com to test using the STR.

Comment 29

[Matt Woodrow (:mattwoodrow)]

Managed to find Fast flip on the US version of google news, still can't reproduce any corruption different to what I see with tip.

Comment 30

[Benjamin Stover (:stechz)]

Matt: not sure about tip anymore, but I just now rebased your patches on top of 635035 and there are corruption issues. We could land this now and deal with it in 635035?

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If this isn't fixing any fennec bugs I don't see why we should land it.  Bug 635035 shouldn't cause corruption (can you be more specific?) but there will be seaming.  Are these patches or bug 635035 on their own the cause of the corruption?  Bug 637852, which bug 635035 is blocked on, will fix the seaming, and its approach avoids the "ASSERTION: Multiplication factors must be integers or 1/integer" problem for fennec's common case.

Comment 32

[Benjamin Stover (:stechz)]

Moving discussion to 635035 because I'm seeing the problem without these patches.

Comment 33

[Benjamin Stover (:stechz)]

This bug seems to fix some rendering issues in about:config.

Comment 34

[Brad Lassey [:blassey] (use needinfo?)]

what is the plan/status here?

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is fixed in my patches in bug 637852.