Scissor rect incorrect with scaled viewport

VERIFIED FIXED in Firefox 14

Status

()

defect
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: joe, Assigned: tnikkel)

Tracking

Trunk
mozilla15
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14+ verified, firefox15 verified, blocking-fennec1.0 beta+)

Details

(Whiteboard: [layout][mozilla-central])

Attachments

(8 attachments, 5 obsolete attachments)

I have a fix for bug 728026, but it uncovers this bug in layout.

The scissor rects we compute when we're going to be drawing are wrong. As currently written, we multiply them by the scale, which is wrong when we're downscaling (eg xx = 0.5). Unfortunately, simply removing the scale (this patch) doesn't work, because the clip rects we've got are sometimes too small for the page (eg on the desktop rollingstone.com).

If you apply this patch, then go to rollingstone.com and go to the desktop page on a Galaxy Nexus, you will see something very much like what the screenshot I'm going to attach looks like.

I don't know where to start for this. BasicLayers seemingly don't scale their clip/scissor rects, which is why I tried this patch. I'm not sure why a container rect on the desktop rollingstone.com has the idea that it needs to be smaller than the full size of the viewport.
Posted image screenshot
Layers.h:
  /**
   * CONSTRUCTION PHASE ONLY
   * Set a clip rect which will be applied to this layer as it is
   * composited to the destination. The coordinates are relative to
   * the parent layer (i.e. the contents of this layer
   * are transformed before this clip rect is applied).
   * For the root layer, the coordinates are relative to the widget,
   * in device pixels.
   * If aRect is null no clipping will be performed. 
   */
  void SetClipRect(const nsIntRect* aRect);

As far as I can tell, the code you're attempting to change is already correct. 'container' isn't drawing to its own intermediate surface, so we'll be drawing to the intermediate surface of some ancestor. That means 'this''s clip rect must be transformed by 'container's transform.
A dump of the shadow layer tree might help here.
Posted file layer dump
We have container layers with clips set on them, and these clips are being sent across correctly. However, the clips themselves seem to be wrong, and I don't know why. I don't understand enough of FrameLayerBuilder to comment meaningfully on this. :(
Whiteboard: [layout]
Matt said he was unable to reproduce this, but I just updated and I still can. :(
We need a fix for this before we release beta.
blocking-fennec1.0: ? → beta+
Do you have any other patches applied? My build was just trunk, plus the patch from  bug 728026 applied, not this one.

Some thoughts:

The layer dump you provided was for the content side, it would be nice to have the matching shadow layer tree. Calling LayerManager::Dump somewhere in Compositor::Composite() should work?

The content layer (the last ThebesLayer) is only 720x1557. Scaled by 0.5,0.5, this doesn't seem like it would cover the screen regardless of clipping. Since it does, I'd suggest this this region is being reported incorrectly or in the wrong coordinate space.

The 4th layer (ContainerLayer) has a clip twice the size of its visible region. This doesn't make sense either, and is probably wrong.
Posted file minimal testcase (obsolete) —
The only patch I have applied is the one from bug 728026, but note it needs unbitrotting (I will upload a new version to that bug).
Joe, can you get the ShadowLayer dump mentioned in #8?
Note: it is equally wrong for something like this screenshot to appear. If you pan or zoom on the page, you'll see that it's actually clipping the page, though the clipped area is not black, but white.
Who owns this?
I spoke with tn about this one yesterday. Assigning...
Assignee: nobody → tnikkel
Status: NEW → ASSIGNED
Posted file content side layer tree dump (obsolete) —
Posted file compositor side layer tree dump (obsolete) —
Posted file content side layer tree dump (obsolete) —
Attachment #620040 - Attachment is obsolete: true
Attachment #620042 - Attachment is obsolete: true
This dump mostly matches the compositor side dump.
Attachment #620077 - Attachment is obsolete: true
Unfortunately the minimal testcase renders properly on my Galaxy S II.
It would be interesting to see the equivalent layer trees from your device Tim.

The unwanted clip is fairly obvious in Joe's layer tree examples, I wonder why you're not being affected by them.
I've been able to reproduce this on a Galaxy Nexus, a Galaxy S 2, and a Nexus S, so I am sort of surprised that others have been having problems.

Tim: You've applied the patch in bug 728026, right?
I'm building a try build at https://tbpl.mozilla.org/?tree=Try&rev=d0fbb635b3c7 to see if it has the same behaviour as my local builds.
(In reply to Joe Drew (:JOEDREW!) from comment #22)
> Tim: You've applied the patch in bug 728026, right?

Oops, no. I'm a little dumb sometimes.
(In reply to Joe Drew (:JOEDREW!) from comment #22)
> Tim: You've applied the patch in bug 728026, right?

Ok, I see the same thing as the screenshot of the minimal testcase with that patch applied.
I'm debugging exactly why the clipping goes wrong here.
Posted file minimal testcase
Attachment #619560 - Attachment is obsolete: true
So what actions does "width=device-width;" actually make us take?
Adding Brad, Mark who might know answer to #28.
Looks like jwir3 and dbaron are actually responsible for some device-width related code.
(In reply to Timothy Nikkel (:tn) from comment #28)
> So what actions does "width=device-width;" actually make us take?

It makes the CSS viewport be the same width as the device. i.e. If you have a device like the Galaxy Nexus with a usable screen area of 720x1038 in portrait, we will call DOMWindowUtils::setCSSViewport with a width of 720. If the user rotates the device to landscape, we call setCSSViewport again with a width of 1038.

The code for this is largely at http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2363
(In reply to Kartikaya Gupta (:kats) from comment #31)
> It makes the CSS viewport be the same width as the device. i.e. If you have
> a device like the Galaxy Nexus with a usable screen area of 720x1038 in
> portrait, we will call DOMWindowUtils::setCSSViewport with a width of 720.
> If the user rotates the device to landscape, we call setCSSViewport again
> with a width of 1038.

On my Galaxy S II (wikipedia tells me it has a 480x720 screen) with the minimal testcase in this bug I see setCSSViewport calls with 320x460. So this is not the screen width but the resolution is set to 1.5, and 320 * 1.5 = 480. Is this what you would expect?
Oh, we also do some scaling of the CSS viewport because of the screen dpi, so that things don't appear tiny on high-density displays. There's a pref that can be used to override it if you want to disable that behaviour.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3675
Here is what happens on my device. setCSSViewport call with 320x460. Document size is now set to that. We create clips (by the scroll frame, and for the subdocument) with this width. When we apply these clips to layers we multiply by the resolution (1.5) to get 480 as a width. The root layer has a 0.6666667 scale transform corresponding to the inverse of the resolution, so when these 480 wide clip gets to the screen it is 320 wide.
So I think the problem is that CompositorParent::TransformShadowTree sets a scale transform on the main scrollable layer that the shadowable layers and layout don't know exist. We correctly scale the contents of the layer and then apply the (non-scaled) clip of that layer as comment 2 points out. Since this transform is added after layout has updated the layer tree and only on the shadow layers, layout has no idea it exists.
Setting that scale transform on the parent layer fixes this bug, but you get different bugs while panning and zooming.
Adding in Benoit and Ali as well for #36
(In reply to Timothy Nikkel (:tn) from comment #36)
> Setting that scale transform on the parent layer fixes this bug, but you get
> different bugs while panning and zooming.

Setting that scale transform where? On the shadowable side? If so, how do you account for that transform changing during asynchronous zooming?

Would it help if TransformShadowTree transformed the ShadowClipRect by the same scale applied to the layer?
(In reply to Ali Juma [:ajuma] from comment #38)
> (In reply to Timothy Nikkel (:tn) from comment #36)
> > Setting that scale transform on the parent layer fixes this bug, but you get
> > different bugs while panning and zooming.
> 
> Setting that scale transform where? On the shadowable side? If so, how do
> you account for that transform changing during asynchronous zooming?

On the shadow side. So where CompositorParent::TransformShadowTree calls SetShadowTransform change that call to happen on the parent of that layer basically.

> Would it help if TransformShadowTree transformed the ShadowClipRect by the
> same scale applied to the layer?

Yeah, probably. Haven't tried that yet.
(In reply to Ali Juma [:ajuma] from comment #38)
> Would it help if TransformShadowTree transformed the ShadowClipRect by the
> same scale applied to the layer?

That fixes the minimal testcase here and panning zooming work fine on it but it severely screws up bugzilla pages, and I've only done minimal testing.
The problem is that the clip rect set on the scrolled layers is the intersection of clipping imposed by containers and the clipping imposed by the scrollframe itself. The latter should be adjusted to account for the shadow transform, and the former should not.

(In reply to Timothy Nikkel (:tn) from comment #34)
> Here is what happens on my device. setCSSViewport call with 320x460.
> Document size is now set to that. We create clips (by the scroll frame, and
> for the subdocument) with this width.

Seems to me we should be setting clipSubdocument to false for the <browser>, then none of this clipping would be happening.

For async scrolling of scrollframes other than the root scroll frame, we need a different solution though.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> The problem is that the clip rect set on the scrolled layers is the
> intersection of clipping imposed by containers and the clipping imposed by
> the scrollframe itself. The latter should be adjusted to account for the
> shadow transform, and the former should not.

Does layout know what the shadow transform is going to be? Or is it set on the compositor thread to support asynchronous panning/zooming?

> Seems to me we should be setting clipSubdocument to false for the <browser>,
> then none of this clipping would be happening.

I tried setting clipSubdocument to false, it caused a lot of other problems. But also, as the code is now, it would not remove that scrollframe clipping.
(In reply to Timothy Nikkel (:tn) from comment #42)
> Does layout know what the shadow transform is going to be?

No.

> Or is it set on the compositor thread to support asynchronous panning/zooming?

Yes.

> > Seems to me we should be setting clipSubdocument to false for the <browser>,
> > then none of this clipping would be happening.
> 
> I tried setting clipSubdocument to false, it caused a lot of other problems.
> But also, as the code is now, it would not remove that scrollframe clipping.

Why not? We would hit the "if (aBuilder->GetIgnoreScrollFrame() == mOuter || IsIgnoringViewportClipping())" path in nsGfxScrollFrameInner::BuildDisplayList.

Another approach would be to rejigger the display item setup. Currently we wrap child items in nsDisplayScrollLayers and then wrap those in nsDisplayClips. Instead we could wrap in the other order. Then I think things just might work; the overflow clip due to scrolling would be applied to the child layers of the layer created by nsDisplayScrollLayer.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> > I tried setting clipSubdocument to false, it caused a lot of other problems.
> > But also, as the code is now, it would not remove that scrollframe clipping.
> 
> Why not? We would hit the "if (aBuilder->GetIgnoreScrollFrame() == mOuter ||
> IsIgnoringViewportClipping())" path in
> nsGfxScrollFrameInner::BuildDisplayList.

Sorry, you are right about that. But it looks like that path also bypasses creating scroll layer items completely, which is probably why I got a completely unusable browser when I tried that.

> Another approach would be to rejigger the display item setup. Currently we
> wrap child items in nsDisplayScrollLayers and then wrap those in
> nsDisplayClips. Instead we could wrap in the other order. Then I think
> things just might work; the overflow clip due to scrolling would be applied
> to the child layers of the layer created by nsDisplayScrollLayer.

Yeah. We'd still have to do something about the subdocument clip if clipSubdocument doesn't currently work for our needs there.
(In reply to Timothy Nikkel (:tn) from comment #44)
> Sorry, you are right about that. But it looks like that path also bypasses
> creating scroll layer items completely, which is probably why I got a
> completely unusable browser when I tried that.

True.

> > Another approach would be to rejigger the display item setup. Currently we
> > wrap child items in nsDisplayScrollLayers and then wrap those in
> > nsDisplayClips. Instead we could wrap in the other order. Then I think
> > things just might work; the overflow clip due to scrolling would be applied
> > to the child layers of the layer created by nsDisplayScrollLayer.
> 
> Yeah. We'd still have to do something about the subdocument clip if
> clipSubdocument doesn't currently work for our needs there.

CompositorParent could just remove the clip rect induced by the <browser>.
Posted patch patch (obsolete) — Splinter Review
Something like this.
Posted patch patchSplinter Review
The subdocument clip wasn't actually a problem as it's outside the subdocument, and it doesn't make it through the container layer for the subdocument.

The reftest becomes unexpected pass with this patch. I haven't investigated why, it might be just an accident, so I don't want to force people to keep it passing on desktop platforms if it was a fluke.

We also had to use the display port rect for clipping otherwise we clip that out and we notice the redrawing when panning or zooming out.

Was there any particular reason that using an nsDisplayListCollection instead of a nsDisplayListSet was important here? Because this patch makes us do the scroll layer wrapping on the final nsDisplayListSet instead of the temp nsDisplayListCollection.
Attachment #622140 - Flags: review?(roc)
Try server build with this patch
https://tbpl.mozilla.org/?tree=Try&rev=4dbf3696bdfa

Try server build with this patch and bug 728026
https://tbpl.mozilla.org/?tree=Try&rev=b7e14cd82e3a
Comment on attachment 622140 [details] [diff] [review]
patch

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

very nice!

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2248,2 @@
>    nsRect clip;
>    clip = mScrollPort + aBuilder->ToReferenceFrame(mOuter);

move this line down into the !usingDisplayport path

@@ +2250,5 @@
>  
> +  nscoord radii[8] = {0, 0, 0, 0, 0, 0, 0, 0};
> +
> +  if (usingDisplayport) {
> +    clip = displayPort + aBuilder->ToReferenceFrame(mOuter);

remove the array initializer and memset radii to zero here
Attachment #622140 - Flags: review?(roc) → review+
In order to get tests in finite time, with Ehsan's blessing I am reusing Oak since it's not prioritized too low.
ftp://ftp.mozilla.org/pub/mobile/tinderbox-builds/oak-android/1336516588/ has the build with both this fix and bug 728026, ready for testing. (Still going through unit tests, though, so I haven't landed it anywhere.)
https://hg.mozilla.org/mozilla-central/rev/dcecf246f732
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Attachment #622140 - Flags: approval-mozilla-aurora?
Whiteboard: [layout] → [layout][mozilla-central]
Comment on attachment 622140 [details] [diff] [review]
patch

Let's get this landed asap for today's anticipated Beta 1 go to build.
Attachment #622140 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #621906 - Attachment is obsolete: true
This issue seems to be fixed on the latest Nightly and Aurora build. Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-30)
Firefox 14.0a2 (2012-05-30)

Device: Galaxy Nexus
OS: Android 4.0.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.