Closed
Bug 637852
Opened 14 years ago
Closed 13 years ago
Move resolution handling out of layer system into FrameLayerBuilder
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: cjones, Assigned: roc)
References
Details
(Keywords: mobile, perf, Whiteboard: QA?)
Attachments
(33 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
stechz
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/html
|
Details | |
(deleted),
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.
Reporter | ||
Comment 1•14 years ago
|
||
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
Reporter | ||
Comment 2•14 years ago
|
||
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.
Reporter | ||
Comment 3•14 years ago
|
||
(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.
Reporter | ||
Comment 4•14 years ago
|
||
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.)
Reporter | ||
Comment 5•14 years ago
|
||
Forgot to mark this yesterday. I've got a patch and some ideas.
Assignee: nobody → jones.chris.g
Reporter | ||
Comment 6•14 years ago
|
||
This is the second part of bug 630743, and should block for the same reason.
tracking-fennec: --- → ?
Updated•14 years ago
|
tracking-fennec: ? → 2.0+
Reporter | ||
Updated•14 years ago
|
Summary: Allow specifying a scroll offset in fractional CSS pixels → Snap scroll offsets and displayport to device pixels
Updated•14 years ago
|
Whiteboard: [needs patch (ETA 3/7)]
Reporter | ||
Comment 7•14 years ago
|
||
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.
Reporter | ||
Comment 8•14 years ago
|
||
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+ → ?
Reporter | ||
Comment 9•14 years ago
|
||
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.
Reporter | ||
Comment 10•14 years ago
|
||
(This work also fixes the jitter in buffer sizes noted in bug 635035.)
Updated•14 years ago
|
tracking-fennec: ? → 2.0next+
Reporter | ||
Comment 11•14 years ago
|
||
Reporter | ||
Comment 12•14 years ago
|
||
Partially broken.
Reporter | ||
Comment 13•14 years ago
|
||
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.
Assignee | ||
Comment 14•14 years ago
|
||
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.
Reporter | ||
Comment 15•14 years ago
|
||
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.
Assignee | ||
Comment 16•14 years ago
|
||
It can probably be contained within FrameLayerBuilder.
Reporter | ||
Comment 17•14 years ago
|
||
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?
Reporter | ||
Comment 18•14 years ago
|
||
(Oops, ignoring all the logging gunk in the patches.)
Assignee | ||
Comment 19•14 years ago
|
||
I can't think of a better way right now!
Reporter | ||
Comment 22•14 years ago
|
||
IIUC, Rob is proposing to move the scroll-offset-frobbing into FrameLayerBuilder, which would obsolete the first patch above.
Assignee | ||
Comment 23•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → roc
Updated•14 years ago
|
tracking-fennec: 2.0next+ → 6+
Updated•14 years ago
|
Assignee | ||
Comment 26•14 years ago
|
||
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.
Assignee | ||
Comment 27•14 years ago
|
||
Attachment #532123 -
Flags: review?(tnikkel)
Comment 28•14 years ago
|
||
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+
Assignee | ||
Comment 29•13 years ago
|
||
Attachment #533241 -
Flags: review?(joe)
Assignee | ||
Comment 30•13 years ago
|
||
Attachment #533242 -
Flags: review?(joe)
Assignee | ||
Comment 31•13 years ago
|
||
Attachment #533243 -
Flags: review?(joe)
Assignee | ||
Comment 32•13 years ago
|
||
Attachment #533244 -
Flags: review?(tnikkel)
Assignee | ||
Comment 33•13 years ago
|
||
Attachment #533245 -
Flags: review?(tnikkel)
Assignee | ||
Comment 34•13 years ago
|
||
Attachment #533246 -
Flags: review?(matt.woodrow+bugzilla)
Assignee | ||
Comment 35•13 years ago
|
||
Attachment #533248 -
Flags: review?(matt.woodrow+bugzilla)
Assignee | ||
Comment 36•13 years ago
|
||
Attachment #533250 -
Flags: review?(matt.woodrow+bugzilla)
Assignee | ||
Comment 37•13 years ago
|
||
Attachment #533251 -
Flags: review?(joe)
Assignee | ||
Comment 38•13 years ago
|
||
Attachment #533252 -
Flags: review?(joe)
Assignee | ||
Comment 39•13 years ago
|
||
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)
Assignee | ||
Comment 40•13 years ago
|
||
Attachment #533254 -
Flags: review?(tnikkel)
Assignee | ||
Comment 41•13 years ago
|
||
Attachment #533256 -
Flags: review?(joe)
Assignee | ||
Comment 42•13 years ago
|
||
Attachment #533257 -
Flags: review?(joe)
Assignee | ||
Comment 43•13 years ago
|
||
Assignee | ||
Comment 44•13 years ago
|
||
Attachment #533259 -
Flags: review?(tnikkel)
Assignee | ||
Comment 45•13 years ago
|
||
Attachment #533263 -
Flags: review?(tnikkel)
Assignee | ||
Comment 46•13 years ago
|
||
Assignee | ||
Comment 47•13 years ago
|
||
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)
Assignee | ||
Comment 48•13 years ago
|
||
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)
Assignee | ||
Comment 49•13 years ago
|
||
Attachment #533268 -
Flags: review?(tnikkel)
Assignee | ||
Comment 50•13 years ago
|
||
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.
Updated•13 years ago
|
Attachment #533246 -
Flags: review?(matt.woodrow+bugzilla) → review+
Updated•13 years ago
|
Attachment #533248 -
Flags: review?(matt.woodrow+bugzilla) → review+
Updated•13 years ago
|
Attachment #533253 -
Flags: review?(tnikkel) → review+
Comment 52•13 years ago
|
||
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.
Assignee | ||
Comment 53•13 years ago
|
||
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).
Comment 54•13 years ago
|
||
Besides CSS transforms, where is this resolution support all used?
Assignee | ||
Comment 55•13 years ago
|
||
The presshell resolution API also triggers it.
Updated•13 years ago
|
Attachment #533250 -
Flags: review?(matt.woodrow+bugzilla) → review+
Comment 56•13 years ago
|
||
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?
Assignee | ||
Comment 57•13 years ago
|
||
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.
Comment 58•13 years ago
|
||
Ah, okay. If there was a ScaleInverseToAppUnits that did this in one step it would have been clearer.
Comment 59•13 years ago
|
||
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?
Assignee | ||
Comment 60•13 years ago
|
||
It should be ScaleRoundIn, good catch.
Updated•13 years ago
|
Attachment #533254 -
Flags: review?(tnikkel) → review+
Comment 61•13 years ago
|
||
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.
Assignee | ||
Comment 62•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Attachment #533245 -
Attachment is obsolete: true
Attachment #533245 -
Flags: review?(tnikkel)
Comment 63•13 years ago
|
||
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 64•13 years ago
|
||
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 65•13 years ago
|
||
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 66•13 years ago
|
||
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 67•13 years ago
|
||
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 68•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #533257 -
Flags: review?(joe) → review+
Comment 69•13 years ago
|
||
(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 70•13 years ago
|
||
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?
Updated•13 years ago
|
Attachment #533244 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 71•13 years ago
|
||
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.
Assignee | ||
Comment 72•13 years ago
|
||
(but I'll do it for those two places if you want)
Comment 73•13 years ago
|
||
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 74•13 years ago
|
||
Comment on attachment 536528 [details] [diff] [review]
Part 6 v2
Ok, fair enough.
Attachment #536528 -
Flags: review?(tnikkel) → review+
Updated•13 years ago
|
Attachment #533259 -
Flags: review?(tnikkel) → review+
Comment 75•13 years ago
|
||
Comment on attachment 533266 [details] [diff] [review]
Part 20: Allow fast scrolling within transformed content
Part 22 is what allows us to do this?
Updated•13 years ago
|
Attachment #533267 -
Flags: review?(tnikkel) → review+
Updated•13 years ago
|
Attachment #533268 -
Flags: review?(tnikkel) → review+
Comment 76•13 years ago
|
||
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?
Assignee | ||
Comment 77•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #533263 -
Flags: review?(tnikkel) → review+
Updated•13 years ago
|
Attachment #533266 -
Flags: review?(tnikkel) → review+
Updated•13 years ago
|
tracking-fennec: 6+ → 7+
Assignee | ||
Comment 78•13 years ago
|
||
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.
Assignee | ||
Comment 79•13 years ago
|
||
I think I'll make FrameMetrics coordinates always be in layer coordinates, that makes sense given that they are a property of the ContainerLayer.
Assignee | ||
Comment 80•13 years ago
|
||
This fixes an obvious bug. The test fails without the patch, passes with it.
Assignee | ||
Updated•13 years ago
|
Attachment #540437 -
Flags: review?(tnikkel)
Assignee | ||
Comment 81•13 years ago
|
||
Attachment #540442 -
Flags: review?(tnikkel)
Updated•13 years ago
|
Attachment #540437 -
Flags: review?(tnikkel) → review+
Comment 82•13 years ago
|
||
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+
Assignee | ||
Comment 83•13 years ago
|
||
Attachment #540649 -
Flags: review?(tnikkel)
Assignee | ||
Comment 84•13 years ago
|
||
Attachment #540650 -
Flags: review?(tnikkel)
Assignee | ||
Comment 85•13 years ago
|
||
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)
Assignee | ||
Comment 86•13 years ago
|
||
I used this testcase to help debug the Fennec problems. Do we have automated tests for the nsContentView APIs?
Assignee | ||
Comment 87•13 years ago
|
||
Here's a slightly modified testcase that tests non-root-viewport scrolling. It exposes some more bugs :-(
Assignee | ||
Comment 88•13 years ago
|
||
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)
Assignee | ||
Comment 89•13 years ago
|
||
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.
Assignee | ||
Comment 90•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #540649 -
Flags: review?(tnikkel) → review+
Comment 91•13 years ago
|
||
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.
Updated•13 years ago
|
Attachment #540650 -
Flags: review?(tnikkel) → review+
Assignee | ||
Updated•13 years ago
|
Summary: Snap scroll offsets and displayport to device pixels → Move resolution handling out of layer system into FrameLayerBuilder
Comment 92•13 years ago
|
||
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.
Assignee | ||
Comment 93•13 years ago
|
||
(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 94•13 years ago
|
||
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+
Comment 95•13 years ago
|
||
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
Assignee | ||
Comment 96•13 years ago
|
||
The Mac Tp4 regression was caused by part 22.
Assignee | ||
Comment 97•13 years ago
|
||
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.
Updated•13 years ago
|
status-firefox7:
--- → fixed
Comment 98•13 years ago
|
||
So it seems that before this bug can be fixed, first the blockers must be removed.
so it remains resolved fixed?
Thanks.
Assignee | ||
Comment 99•13 years ago
|
||
the bugs blocked by this bug need to be retested. Most or all of them should be fixed.
Updated•13 years ago
|
Whiteboard: QA?
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•