Closed Bug 637852 Opened 13 years ago Closed 13 years ago

Move resolution handling out of layer system into FrameLayerBuilder

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla7
Tracking Status
firefox7 --- fixed
fennec 7+ ---

People

(Reporter: cjones, Assigned: roc)

References

Details

(Keywords: mobile, perf, Whiteboard: QA?)

Attachments

(33 files, 2 obsolete files)

43.74 KB, text/html
Details
8.77 KB, patch
Details | Diff | Splinter Review
22.81 KB, patch
Details | Diff | Splinter Review
17.25 KB, patch
Details | Diff | Splinter Review
1.26 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.26 KB, patch
joe
: review+
Details | Diff | Splinter Review
7.69 KB, patch
joe
: review+
Details | Diff | Splinter Review
3.44 KB, patch
joe
: review+
Details | Diff | Splinter Review
23.70 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
10.91 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
16.44 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
70.04 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.00 KB, patch
joe
: review+
Details | Diff | Splinter Review
4.96 KB, patch
joe
: review+
Details | Diff | Splinter Review
3.42 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.04 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
7.07 KB, patch
joe
: review+
Details | Diff | Splinter Review
2.41 KB, patch
joe
: review+
Details | Diff | Splinter Review
5.58 KB, patch
Details | Diff | Splinter Review
11.57 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
15.68 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.92 KB, patch
Details | Diff | Splinter Review
983 bytes, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.78 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
7.10 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
36.46 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.26 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.79 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.76 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.65 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
6.45 KB, patch
stechz
: review+
Details | Diff | Splinter Review
1.63 KB, text/html
Details
1.67 KB, text/html
Details
To avoid seaming in ThebesLayerBuffer when we use scrolling optimizations, the displayport and scroll offsets need to fall on whole device-pixel boundaries, taking resolution into account.  With a non-identify resolution, we need to specify scroll offsets with fractional components to get device-pixel snapping.  Currently nsGlobalWindow::ScrollTo() only accepts integer scroll offsets.  We probably can't change that, so might need to add an nsIDOMWindowUtils backdoor.

One issue that remains was raised in bug 635035 comment 34: we can end up using the ThebesLayerBuffer self-copy code for non-identity resolution with transformed ThebesLayers.  AFAIK nothing ensures, or can ensure, that those scrolls are snapped to device pixels.  If we get the rounding wrong, then we can get seaming there.  Worth pondering, but that's already a problem on m-c tip so could be fixed in another bug.
This work doesn't really depend on bug 630743 code-wise, but there's no motivation to land this until after bug 630743.
Depends on: 630743
It strikes me that potential fallout from this is interfaces that return the scroll offset in int CSS pixels.  Not sure how big of a deal that will be.
Attached file Test
(To reiterate, this test is only relevant after bug 630743 is fixed.)

STR
 (1) Load test
 (2) Press "Dance".  (URL can be out of view or not, doesn't matter after 630743.)

The test scrolls right/down alternately by 30 CSS pixels = 22.5 device pixels with the scale applied.  If the offsets aren't snapped, the text in the page should dance.  This isn't testing *fennec's* setting of the scroll offset during user-initiated panning, but rather content-initiated scrolling.  If both go through the same code path, this test should be OK.  If not, a good test will be hard.
With a build including bug 630743, in the updated test at http://people.mozilla.com/~cjones/test-637852.html I see jumping text, on the galaxy tab in portrait.  (Which makes sense.)
Forgot to mark this yesterday.  I've got a patch and some ideas.
Assignee: nobody → jones.chris.g
This is the second part of bug 630743, and should block for the same reason.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
Summary: Allow specifying a scroll offset in fractional CSS pixels → Snap scroll offsets and displayport to device pixels
Whiteboard: [needs patch (ETA 3/7)]
Blocks: 634397
Update
 - good news: fixed text-positioning problems
 - bad news: patches are invasive, break some assumptions, need more work

I'm out of steam for the night, but haven't given up.  Will resume tomorrow.  This bug is looking riskier.
This has become too much of a tar baby for me to target it for 4.0, with a clear conscience.  HOWEVER
 - I think we can use this work to improve the platform APIs fennec is using
 - I think this and blockees should land ASAP for a 4.x release
 - I propose we fork this work off into a prototype hybrid m-c/m-b project branch and continue on

We've got enough goodies for a killer 4.0; let's focus on less risky stuff.

(To relevant parties:) Let's chat about this future work in-person soon, perhaps after the fennec meeting tomorrow.
tracking-fennec: 2.0+ → ?
The fundamental problem here was threefold
 (1) displayport not on device-pixel boundaries accounting for resolution.  Seemed fixed in my testing by bug 630743, wasn't extensive though.
 (2) scroll offsets not on device-pixel boundaries.  This would lead to seaming if all other issues were fixed with bug 635035.
 (3) cairo's fallback text drawing code snaps glyph positions to integral offsets from surfaces.  FrameLayerBuilder rounds visible regions out to pixel boundaries *before* resolution is applied, which stomps on fixes from (1) and (2).  This still leads to dancing text with both those fixed.

I tried a hacky fix for (3) that translates the gfxContext by the fractional device pixel the bad snapping stomps on, but didn't get it to work.  I gave up pretty quickly in favor of a "real fix."  I'm not sure this could work, but maybe; it also has issues in possibly leaving the bottom/right part of the surface unpainted.  Would need care.

I've got a pretty good start on fixing all three issues "for real".  The patches I have now do
 - nudge scroll offsets to device-pixel boundaries taking resolution into account (i.e., scroll offset might fall on fractional CSS pixel)
 - switch FrameLayerBuilder computations to be in app units, only rounding out to device pixels when needed
 - make FrameLayerBuilder aware of resolution, and snap to pixels while taking resolution into account (relying on (1) and (2) above).  This makes resolution-scaled thebeslayer coordinates be in actual device pixels, which is what we should have done all along.
 - have FrameLayerBuilder::DrawThebesLayer responsible for mapping real-device-pixel ThebesLayer coordinates for painting/invalidating back to possibly-fractional-CSS-pixel app unit values, taking resolution into account.

It's the change in semantics of resolution-scaled thebeslayers that introduces all the risk.  This requires
 - update ThebesLayerBuffer and ThebesLayer.  Mostly done, but breaks resolution-scaling for CSS transforms.  That needs more thought.
 - updates as necessary for ContainerLayers and other layers.  Not sure what's needed here yet.
 - fix viewconfig APIs for the frontend.  The scaleX/Y stuff is still OK, but the scroll offsets reported to the frontend in layer FrameMetrics are just bogus now; they're the original snapped-out values that caused the problems.  These offsets aren't too terribly far off, so it might be possible to hack in some fixes.
 - fix sub-frame scrolling.  This was relying on inheriting resolution from the root scroll frame, AFAICT.  Not too hard to hack in.
 - guarantee device-pixel displayport/scroll offset for sub-frames.  Otherwise these will have the same dancing+seaming we're fixing for root frames.

I may have forgotten something, writing quickly.  All this adds risk to desktop FF too, not just mobile.

Moving forward, I think we have these options
 (A) Find a hacky fix for dancing text for 4.0.  Might be possible, not sure.  This wouldn't necessarily unblock 634397 and 635035.
 (B) Hacky guarantee that ThebesLayerBuffer rotations always fall on device pixels.  This would unblock 634397 and 635035.
 (C) We could get A and B by continuing to hack away at these patches.
 (D) Finish these patches for 4.x, and change the APIs used by the frontend to always deal in real device pixels.  This will make everyone's life easier.
 (E) Ship 4.0 with current resolution API, deprecate it in favor of something else post-4.0.
(This work also fixes the jitter in buffer sizes noted in bug 635035.)
tracking-fennec: ? → 2.0next+
Keeps ThebesLayer coordinates in actual device pixels, moves map->CSS pixels into FrameLayerBuilder.

Very broken, but fixes test-positioning problems.  This patch and the last two show where the original plan here was headed.
Moving all resolution handling out of ThebesLayers into FrameLayerBuilder makes sense. You're right, it should have been done that way from the start; I thought we could hide the resolution abstraction behind the ThebesLayer API, but clearly we can't.
The downside to that is it promotes resolution to a full-class layout citizen alongside the zooms etc., which means it will leak out into a lot more code.  If there's a better way to accomplish the same goal (maybe the goal should be re-evaluated too) while re-using more existing code, I suspect everyone would be happier.
It can probably be contained within FrameLayerBuilder.
We need at least to take the resolution into account when scrolling, so that the scroll offset maps to a device pixel and we can rotate/self-copy properly.  See the first WIP above.  Is there a better way to do that?
(Oops, ignoring all the logging gunk in the patches.)
I can't think of a better way right now!
IIUC, Rob is proposing to move the scroll-offset-frobbing into FrameLayerBuilder, which would obsolete the first patch above.
My basic plan here for FrameLayerBuilder is to pass down an extra scale factor to be applied to child layers as we build the layer tree from the top down. So if a container has a large scaling-up transform, we'll pass that scale-up down to the children, keeping the transforms set on containers to some small scale-down.

That means that when we construct a layer we already know what its resolution is going to be, so we can snap things appropriately when we convert from appunits and we can keep ThebesLayer construction working with nsIntRegions etc. The resolution scale is finally applied either by adjusting the transform on non-ThebesLayer leaf layers, or in DrawThebesLayer.

Scrolling is still going to be a problem. I think the best idea is to support arbitrary appunit scroll offsets, and simply observe when the scroll offset changes by an amount that is not a multiple of layer pixels and rerender the layer in that case. Then we try to avoid that with scrollframes making a best-effort attempt to move their scroll positions by a multiple of layer pixels. This could include peeking at the current layer tree.
I'm not working on this anymore.
Assignee: jones.chris.g → nobody
Assignee: nobody → roc
tracking-fennec: 2.0next+ → 6+
roc, what's the plan/status here?
Whiteboard: [needs patch (ETA 3/7)]
Keywords: mobile, perf
I've been working on this for a couple of weeks. I'm nearly done with the move of resolution handling into FrameLayerBuilder. I'm not sure what else needs to be done here once I've finished that.
Comment on attachment 532123 [details] [diff] [review]
Part 1: Don't snap BasicThebesLayer effective transforms when we're not retaining layers

Seems reasonable enough.
Attachment #532123 - Flags: review?(tnikkel) → review+
Not really needed in this bug, but I got annoyed with the output and I have to put the patch somewhere. Anyway it's trivial.
Attachment #533253 - Flags: review?(tnikkel)
Now that resolution-handling has moved into layout, we can start to tackle the issues this bug is actually about. This patch lets us try to use accelerated scrolling within transformed content, so we can test scrolling with non-identity resolution.
Attachment #533266 - Flags: review?(tnikkel)
This fixes a bug that crept in earlier and adds some warnings to the ScaleRoundOut methods.

I considered making ScaleRoundOut return an empty rect for an empty input rect, but that relies on interpreting rectangles as areas, not sets of points, and we sometimes use the latter interpretation. We might need a different method, ScaleRoundOutEdges or something like that, but I don't want to deal with that here.
Attachment #533267 - Flags: review?(tnikkel)
Phew!

This patch queue passes on tryserver.

Part 22 forces us to repaint entire ThebesLayers if a scroll operation ends up not being ThebesLayer-pixel-aligned. That's better than the current trunk situation where we always repaint everything if there's a non-identity scale. But we can do even better by having our various scroll operations try to choose scroll amounts that are ThebesLayer-pixel-aligned. This is possible both for user-initiated scrolling (where we have some flexibility in how much to scroll) and for script-initiated scrolling (where setting scrollLeft/scrollTop to some value N only requires that the underlying scroll offset return N after rounding to the nearest CSS pixel).

However, let's do that in a separate bug, if and when we do it.
Attachment #533246 - Flags: review?(matt.woodrow+bugzilla) → review+
Attachment #533248 - Flags: review?(matt.woodrow+bugzilla) → review+
Attachment #533253 - Flags: review?(tnikkel) → review+
I little outline of what are are trying to accomplish by adding resolution to layers and how we do it might be helpful here as I didn't follow the original introduction of resolution into layers very closely.
When a transform scales content up, if we just render the content at its normal resolution and scale that up, it will look pixellated. We can avoid this by rendering the content at a higher resolution, preferably one matching the resolution at which the content will eventually be drawn. The idea is that one "device pixel" as seen by layout will map to more than one pixel in the ThebesLayer into which we render the content. When we draw into that ThebesLayer, we draw with a scale in the CTM of the gfxContext used for drawing. Make sense so far?

When a transform scales content down, instead of worrying about quality we worry about buffer size. By using less than one pixel in the ThebesLayer per "device pixel" as seen by layout, we can save memory (and possibly quality too if we can avoid having to resample the ThebesLayer when we composite it).

When a transform's scale is constantly changing, we don't want to keep rerendering the ThebesLayer due to resolution changes. So in some cases we round up the resolution to the nearest power of two in each direction. This guarantees that during compositing the content is being scaled down (less lossy than scaling up), but it's being scaled down by less than a factor of 2 (scaling down by more than that means we may be wasting VRAM on oversized buffers; also, many scaling algorithms produce poor quality results for scale-down by more than a factor of 2).
Besides CSS transforms, where is this resolution support all used?
The presshell resolution API also triggers it.
Attachment #533250 - Flags: review?(matt.woodrow+bugzilla) → review+
Comment on attachment 533245 [details] [diff] [review]
Part 6: Implement resolution scaling in FrameLayerBuilder

>@@ -905,23 +926,24 @@ ContainerState::FindOpaqueBackgroundColo
>+    rect.ScaleInverseRoundOut(mParameters.mXScale, mParameters.mYScale);

Why do we need to scale this rect? Aren't we comparing it only with things in the same container layer and so same scale?
Appunits are always before scaling, integer (layer) coordinates are always after scaling. Here we're going from layer coordinates to appunits so we have to inverse-scale.
Ah, okay. If there was a ScaleInverseToAppUnits that did this in one step it would have been clearer.
Blocks: 660740
Comment on attachment 533245 [details] [diff] [review]
Part 6: Implement resolution scaling in FrameLayerBuilder

@@ -1676,30 +1778,31 @@ FrameLayerBuilder::BuildContainerLayerFo
+  if (aChildren.IsOpaque() && !aChildren.NeedsTransparentSurface()) {
+    bounds.ScaleRoundOut(scaleParameters.mXScale, scaleParameters.mYScale);
+    if (bounds.Contains(pixBounds.ToAppUnits(appUnitsPerDevPixel))) {
+      // Clear CONTENT_COMPONENT_ALPHA
+      flags = Layer::CONTENT_OPAQUE;
+    }

Shouldn't this be ScaleRoundIn? Or should we convert pixBounds to appunits and then scale it and then compare it to bounds?
Blocks: 656797
It should be ScaleRoundIn, good catch.
Attachment #533254 - Flags: review?(tnikkel) → review+
Comment on attachment 533245 [details] [diff] [review]
Part 6: Implement resolution scaling in FrameLayerBuilder

@@ -562,24 +562,30 @@ void nsDisplayList::PaintForFrame(nsDisp
+  // Root is being scaled up by the X/Y resolution. Scale it back down.
+  gfx3DMatrix rootTransform = root->GetTransform()*
+    gfx3DMatrix::Scale(1.0f/containerParameters.mXScale,
+                       1.0f/containerParameters.mYScale, 1.0f);

And then we don't do anything with rootTransform.
Attached patch Part 6 v2Splinter Review
Good catch. Here's the updated patch with that fixed. We just need to set that transform on the root layer.
Attachment #536528 - Flags: review?(tnikkel)
Attachment #533245 - Attachment is obsolete: true
Attachment #533245 - Flags: review?(tnikkel)
Comment on attachment 533241 [details] [diff] [review]
Part 2: Add BaseRect::ScaleInverseRoundOut API

NS_ceil and NS_floor are both gone now; just use the standard C functions.
Attachment #533241 - Flags: review?(joe) → review+
Comment on attachment 533242 [details] [diff] [review]
Part 3: Add nsPoint::ScaleToNearestPixels, nsRect::ScaleToNearestPixels, nsRect::ScaleToInsidePixels and nsRect::ScaleToOutsidePixels APIs

Review of attachment 533242 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/nsPoint.h
@@ -71,3 @@
>    return nsIntPoint(
> -      NSToIntRoundUp(NSAppUnitsToDoublePixels(x, aAppUnitsPerPixel)),
> -      NSToIntRoundUp(NSAppUnitsToDoublePixels(y, aAppUnitsPerPixel)));

It is weird to me that this is "Nearest" pixels but we just explicitly round up. Obviously this is something that has existed for a while though :)
Attachment #533242 - Flags: review?(joe) → review+
Comment on attachment 533243 [details] [diff] [review]
Part 4: Add nsRegion::ScaleInverseRoundOut and nsRegion::ScaleToOutsidePixels APIs

Review of attachment 533243 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #533243 - Flags: review?(joe) → review+
Comment on attachment 533251 [details] [diff] [review]
Part 10: Remove mX/YResolution from ThebesLayer

Review of attachment 533251 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #533251 - Flags: review?(joe) → review+
Comment on attachment 533252 [details] [diff] [review]
Part 11: Remove ExtendForScaling from nsRect and nsRegion

Review of attachment 533252 [details] [diff] [review]:
-----------------------------------------------------------------

one r+ for every line removed
Attachment #533252 - Flags: review?(joe) → review+
Comment on attachment 533256 [details] [diff] [review]
Part 14: Try to use snappable rects to draw solid borders instead of using stroke, when a scaling transform is present

Review of attachment 533256 [details] [diff] [review]:
-----------------------------------------------------------------

This is a weak r+; I don't know enough about nsCSSRenderingBorders to make it stronger.
Attachment #533256 - Flags: review?(joe) → review+
Attachment #533257 - Flags: review?(joe) → review+
(In reply to comment #64)
> It is weird to me that this is "Nearest" pixels but we just explicitly round
> up. Obviously this is something that has existed for a while though :)

The "Up" refers to the handling of rounding 0.5, and we have to pick one way to round it.
Comment on attachment 536528 [details] [diff] [review]
Part 6 v2

There are a few places where we convert from nsInt* to ns* by the two step process ToAppUnits then ScaleInverse. Why not make a function that does this?
Attachment #533244 - Flags: review?(tnikkel) → review+
I see only two places where we could use that.

In DrawThebesLayer it would be hard to use that convenience function, because we need to apply the offset as well. Adding the offset to aRegionToDraw would require the creation of an extra temporary region.
(but I'll do it for those two places if you want)
I've tested these patches with mobile browser, and that looks mostly broken.
1) Initial scale is wrong
2) Offsets are wrong when you scroll it down.
3) some black areas...

I was testing on fennec with accelerated layers enabled.
without these patches it works fine (but slow updates)
Comment on attachment 536528 [details] [diff] [review]
Part 6 v2

Ok, fair enough.
Attachment #536528 - Flags: review?(tnikkel) → review+
Blocks: 662521
Attachment #533259 - Flags: review?(tnikkel) → review+
Blocks: 651341
Comment on attachment 533266 [details] [diff] [review]
Part 20: Allow fast scrolling within transformed content

Part 22 is what allows us to do this?
Attachment #533267 - Flags: review?(tnikkel) → review+
Attachment #533268 - Flags: review?(tnikkel) → review+
Comment on attachment 533263 [details] [diff] [review]
Part 18: Support computing the "residual transform" for a ThebesLayer --- the difference between its snapped transform and the ideal transform --- and use it to align ThebesLayer drawing for transform

+  /**
+   * True when
+   */
+  bool mAllowResidualTranslation;

You trailed off there.

In bug 630835 we fixed some problems where we reported our layer as opaque but it wasn't actually opaque because we didn't quite paint everything. Is adding this residual translation going to cause problems like that?

Why do we want to snap the layer compositing transform and then perform a "fixup" before drawing into the thebes layer, as opposed to just not snapping the layer compositing transform?
(In reply to comment #75)
> Comment on attachment 533266 [details] [diff] [review] [review]
> Part 20: Allow fast scrolling within transformed content
> 
> Part 22 is what allows us to do this?

Yes, part 22 fixes a regression that part 20 causes.

(In reply to comment #76)
> +  /**
> +   * True when
> +   */
> +  bool mAllowResidualTranslation;
> 
> You trailed off there.

I'll change it to "See SetAllowResidualTranslation."

> In bug 630835 we fixed some problems where we reported our layer as opaque
> but it wasn't actually opaque because we didn't quite paint everything. Is
> adding this residual translation going to cause problems like that?

No. See the comment above FrameLayerBuilder::DrawThebesLayer. As long as the opaque stuff is snapped we should be fine.

> Why do we want to snap the layer compositing transform and then perform a
> "fixup" before drawing into the thebes layer, as opposed to just not
> snapping the layer compositing transform?

Because unsnapped transforms, e.g. a translation by half a pixel, cause horrible blur due to resampling (and are slow if you don't have a GPU). And you still have the problem that pixels are being affected that you didn't expect; e.g. nearest-neighbour sampling doesn't blur, but it behaves just like snapping the transform except you don't know you're doing it.
Attachment #533263 - Flags: review?(tnikkel) → review+
Attachment #533266 - Flags: review?(tnikkel) → review+
tracking-fennec: 6+ → 7+
The reason this breaks Fennec is confusion about whether FrameMetrics::mViewport/mContentSize/mViewportScrollOffset/mDisplayPort are CSS pixels or layer coordinates. Currently they're the same in Fennec, but the work in this bug makes layer coordinates not always be CSS pixels.
I think I'll make FrameMetrics coordinates always be in layer coordinates, that makes sense given that they are a property of the ContainerLayer.
This fixes an obvious bug. The test fails without the patch, passes with it.
Attachment #540437 - Flags: review?(tnikkel) → review+
Comment on attachment 540442 [details] [diff] [review]
Part 24: fix scale/translate order

I remember thinking about this during the review, but it got lost in my head somewhere.
Attachment #540442 - Flags: review?(tnikkel) → review+
Even though I reviewed this RenderFrameParent code, now that I've worked on it, I don't like it very much :-). It could probably be rewritten to be much nicer. It also sort of assumes that transforms are just scales + translations, which isn't good. But I don't want to rewrite it now, especially not in this bug, and I think its problems are fixable within RenderFrameParent (and possibly nsContentView and the Fennec chrome code); the FrameMetrics interface shouldn't need to be changed.

I thought BuildListForLayer would need to be changed, but event handling seems to work fine. As far as I can tell, Fennec works great at this point.
Attachment #540652 - Flags: review?(ben)
Attached file manual testcase
I used this testcase to help debug the Fennec problems. Do we have automated tests for the nsContentView APIs?
Attached file modified testcase
Here's a slightly modified testcase that tests non-root-viewport scrolling. It exposes some more bugs :-(
Didn't this get changed recently? Anyway, it's broken. BorderBackground is the bottom-most list so this should be fine.
Attachment #540664 - Flags: review?(ben)
Hmm, all the bugs I can reproduce using the testcase in comment #87 exist in trunk Fennec already, so I'm not going to try to fix them here.
Comment on attachment 540664 [details] [diff] [review]
Part 28: Fix nsDisplayScrollInfoLayer abort by ensuring nsDisplayScrollInfoLayer is before all other nsDisplayScrollInfos in z-order

I thought this looked familiar, you fixed it in bug 656041.
Attachment #540664 - Attachment is obsolete: true
Attachment #540664 - Flags: review?(ben)
Attachment #540649 - Flags: review?(tnikkel) → review+
Comment on attachment 533257 [details] [diff] [review]
Part 15: Don't round mOuterRect/mInnerRect if there's a scale factor in the current transform

>From: Robert O'Callahan <robert@ocallahan.org>
>
>Bug 673852. Part 15: Don't round mOuterRect/mInnerRect if there's a scale factor in the current transform. r=joe

Bug number is wrong.
Attachment #540650 - Flags: review?(tnikkel) → review+
Summary: Snap scroll offsets and displayport to device pixels → Move resolution handling out of layer system into FrameLayerBuilder
Comment on attachment 540652 [details] [diff] [review]
27: Fix RenderFrameParent rendering to handle transforms on the root layer, and fix bugs with parameters being modified

Review of attachment 540652 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, looks good except I have some confusion about your new use of ComputeShadowTreeTransform.

::: layout/ipc/RenderFrameParent.cpp
@@ +230,1 @@
>                    nsDisplayListBuilder* aBuilder,

Yes, this is more readable if we aren't changing aTransform. As I grok it, the only change you made in the function is this refactoring. Am I right?

@@ +303,5 @@
>  
>      ViewTransform viewTransform = ComputeShadowTreeTransform(
>        aFrame, aFrameLoader, metrics, view->GetViewConfig(),
> +      1 / (GetXScale(currentTransform)*layerTransform.mXScale),
> +      1 / (GetYScale(currentTransform)*layerTransform.mYScale)

I'm confused by this. BuildListForLayer only includes the inverse scale for the transforms up to but not including the current layer, but this includes the current layer. Why? I should say I'm having a really hard time following this code now that I've been out of it for a while...

@@ +314,5 @@
> +      layerTransform.mTranslation = viewTransform.mTranslation;
> +      // Apply the root frame translation *before* we do the rest of the transforms.
> +      nsIntPoint rootFrameOffset = GetRootFrameOffset(aFrame, aBuilder);
> +      shadowTransform = shadowTransform *
> +          gfx3DMatrix::Translation(float(rootFrameOffset.x), float(rootFrameOffset.y), 0.0);

OK, this makes sense. Root frame translation precedes normal layer transformation precedes shadow transformation. Good catch.
(In reply to comment #92)
> ::: layout/ipc/RenderFrameParent.cpp
> @@ +230,1 @@
> >                    nsDisplayListBuilder* aBuilder,
> 
> Yes, this is more readable if we aren't changing aTransform. As I grok it,
> the only change you made in the function is this refactoring. Am I right?

Yes.

> @@ +303,5 @@
> >  
> >      ViewTransform viewTransform = ComputeShadowTreeTransform(
> >        aFrame, aFrameLoader, metrics, view->GetViewConfig(),
> > +      1 / (GetXScale(currentTransform)*layerTransform.mXScale),
> > +      1 / (GetYScale(currentTransform)*layerTransform.mYScale)
> 
> I'm confused by this. BuildListForLayer only includes the inverse scale for
> the transforms up to but not including the current layer, but this includes
> the current layer. Why? I should say I'm having a really hard time following
> this code now that I've been out of it for a while...

In TransformShadowTree, the layer's transform is applied on top of the viewTransform, so the inverse transform should include the layer's transform. Does that make sense?

I don't know why BuildListForLayer doesn't need that. I think it should for the same reasons, but I didn't find any bugs with that so I didn't change it.
Comment on attachment 540652 [details] [diff] [review]
27: Fix RenderFrameParent rendering to handle transforms on the root layer, and fix bugs with parameters being modified

> In TransformShadowTree, the layer's transform is applied on top of the
> viewTransform, so the inverse transform should include the layer's
> transform. Does that make sense?

Ah. Yes.

> 
> I don't know why BuildListForLayer doesn't need that. I think it should for
> the same reasons, but I didn't find any bugs with that so I didn't change it.

This worries me. Let's put an XXX comment in BuildListForLayer, as I would expect it to be symmetric with your logic here.

r+ with XXX comment and space-separated * operator mentioned above. Let's get this landed! :D
Attachment #540652 - Flags: review?(ben) → review+
http://hg.mozilla.org/mozilla-central/rev/7853e5cf72f7
http://hg.mozilla.org/mozilla-central/rev/c9dd59391c7d
http://hg.mozilla.org/mozilla-central/rev/b53df216be61
http://hg.mozilla.org/mozilla-central/rev/1da2c78a1a72
http://hg.mozilla.org/mozilla-central/rev/31c47102a6fc
http://hg.mozilla.org/mozilla-central/rev/45b7622bc948
http://hg.mozilla.org/mozilla-central/rev/198d6364abb9
http://hg.mozilla.org/mozilla-central/rev/602d13dcab53
http://hg.mozilla.org/mozilla-central/rev/123d2c2f6260
http://hg.mozilla.org/mozilla-central/rev/5b2a58c9c279
http://hg.mozilla.org/mozilla-central/rev/a63a96b9571e
http://hg.mozilla.org/mozilla-central/rev/e552be420a02
http://hg.mozilla.org/mozilla-central/rev/2c7a42271f31
http://hg.mozilla.org/mozilla-central/rev/9c05bcab628f
http://hg.mozilla.org/mozilla-central/rev/9ae31ef1ae05
http://hg.mozilla.org/mozilla-central/rev/c6c5f217fedf
http://hg.mozilla.org/mozilla-central/rev/500265c61f37
http://hg.mozilla.org/mozilla-central/rev/4c323a5e40c2
http://hg.mozilla.org/mozilla-central/rev/e96e2e5829cd
http://hg.mozilla.org/mozilla-central/rev/c9f644aa2fa5
http://hg.mozilla.org/mozilla-central/rev/3d7fda340878
http://hg.mozilla.org/mozilla-central/rev/6256bcafbdf2
http://hg.mozilla.org/mozilla-central/rev/dcfa8de9746a
http://hg.mozilla.org/mozilla-central/rev/c18bdf8b0b76
http://hg.mozilla.org/mozilla-central/rev/8b1e793fbf45
http://hg.mozilla.org/mozilla-central/rev/21a67ac790f1
http://hg.mozilla.org/mozilla-central/rev/e302cef502f6
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
No longer blocks: 660740
Depends on: 668698
Depends on: 669522
The Mac Tp4 regression was caused by part 22.
I did some experiments on try.

It's definitely this block:
886   if (activeScrolledRootTopLeft != data->mActiveScrolledRootPosition) {
887     data->mActiveScrolledRootPosition = activeScrolledRootTopLeft;
888     nsIntRect invalidate = layer->GetValidRegion().GetBounds();
889     layer->InvalidateRegion(invalidate);
890   }
Setting the condition to false makes the regression go away.

I added some instrumentation to see when that's getting hit with nonempty valid region. I get results like this:
http://tinderbox.mozilla.org/showlog.cgi?log=Try/1310106496.1310107273.15185.gz&fulltext=1

NOISE: Cycle 8: loaded http://localhost/page_load_test/tp4/www.ku6.com/www.ku6.com/index.html (next: http://localhost/page_load_test/tp4/www.interia.pl/www.interia.pl/index.html)
NOISE: NOTE! Invalidating area 0,0,1009,654, offset was 0.000000,0.000000, is 0.000000,0.433333
NOISE: NOTE! Invalidating area 0,0,1009,656, offset was 0.000000,0.433333, is 0.000000,-0.500000
NOISE: NOTE! Invalidating area 0,0,1009,667, offset was 0.000000,-0.500000, is 0.000000,0.066667
NOISE: NOTE! Invalidating area 0,0,1009,673, offset was 0.000000,0.066667, is 0.000000,0.433333
NOISE: NOTE! Invalidating area 0,0,1009,676, offset was 0.000000,0.433333, is 0.000000,0.466667
NOISE: NOTE! Invalidating area 0,0,1009,679, offset was 0.000000,0.466667, is 0.000000,0.300000
NOISE: NOTE! Invalidating area 0,0,1009,680, offset was 0.000000,0.300000, is 0.000000,0.033333
NOISE: NOTE! Invalidating area 0,0,1009,681, offset was 0.000000,0.033333, is 0.000000,0.000000

But I downloaded that same build and ran it with Tp4 on my 10.5 machine locally (standalone Talos), and got no output like that whatsoever. Conditional breakpoints show the same thing on Mac and Windows, I can never get it to  invalidate anything there.

So I don't seem to be able to reproduce the regression.
So it seems that before this bug can be fixed, first the blockers must be removed.
so it remains resolved fixed?
Thanks.
the bugs blocked by this bug need to be retested. Most or all of them should be fixed.
Depends on: 681033
Depends on: 692713
Whiteboard: QA?
Depends on: 766564
Depends on: 778698
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: