749425 - Scissor rect incorrect with scaled viewport

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Joe Drew (not getting mail)]

Attachment: incomplete attempt to fix

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.

Comment 1

[Joe Drew (not getting mail)]

Attachment: screenshot

Comment 2

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

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.

Comment 3

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

A dump of the shadow layer tree might help here.

Comment 4

[Joe Drew (not getting mail)]

Attachment: layer dump

Comment 5

[Joe Drew (not getting mail)]

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. :(

Comment 6

[Joe Drew (not getting mail)]

Matt said he was unable to reproduce this, but I just updated and I still can. :(

Comment 7

[Joe Drew (not getting mail)]

We need a fix for this before we release beta.

Comment 8

[Matt Woodrow (:mattwoodrow)]

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.

Comment 9

[Joe Drew (not getting mail)]

Attachment: minimal testcase

Comment 10

[Joe Drew (not getting mail)]

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).

Comment 11

[JP Rosevear [:jpr]]

Joe, can you get the ShadowLayer dump mentioned in #8?

Comment 12

[Joe Drew (not getting mail)]

Attachment: screenshot of testcase

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.

Comment 13

[Damon Sicore (:damons)]

Who owns this?

Comment 14

[Jet Villegas (inactive)]

I spoke with tn about this one yesterday. Assigning...

Comment 15

[Joe Drew (not getting mail)]

Attachment: content side layer tree dump

Comment 16

[Joe Drew (not getting mail)]

Attachment: compositor side layer tree dump

Comment 17

[Joe Drew (not getting mail)]

Attachment: content side layer tree dump

Comment 18

[Joe Drew (not getting mail)]

Attachment: compositor side layer tree dump

Comment 19

[Joe Drew (not getting mail)]

Attachment: content side layer tree dump

This dump mostly matches the compositor side dump.

Comment 20

[Timothy Nikkel (:tnikkel)]

Unfortunately the minimal testcase renders properly on my Galaxy S II.

Comment 21

[Matt Woodrow (:mattwoodrow)]

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.

Comment 22

[Joe Drew (not getting mail)]

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?

Comment 23

[Joe Drew (not getting mail)]

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.

Comment 24

[Timothy Nikkel (:tnikkel)]

(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.

Comment 25

[Timothy Nikkel (:tnikkel)]

(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.

Comment 26

[Timothy Nikkel (:tnikkel)]

I'm debugging exactly why the clipping goes wrong here.

Comment 27

[Timothy Nikkel (:tnikkel)]

Attachment: minimal testcase

Comment 28

[Timothy Nikkel (:tnikkel)]

So what actions does "width=device-width;" actually make us take?

Comment 29

[JP Rosevear [:jpr]]

Adding Brad, Mark who might know answer to #28.

Comment 30

[JP Rosevear [:jpr]]

Looks like jwir3 and dbaron are actually responsible for some device-width related code.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

(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

Comment 32

[Timothy Nikkel (:tnikkel)]

(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?

Comment 33

[Kartikaya Gupta (email:[e-mail])]

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

Comment 34

[Timothy Nikkel (:tnikkel)]

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.

Comment 35

[Timothy Nikkel (:tnikkel)]

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.

Comment 36

[Timothy Nikkel (:tnikkel)]

Setting that scale transform on the parent layer fixes this bug, but you get different bugs while panning and zooming.

Comment 37

[JP Rosevear [:jpr]]

Adding in Benoit and Ali as well for #36

Comment 38

[Ali Juma [:ajuma]]

(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?

Comment 39

[Timothy Nikkel (:tnikkel)]

(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.

Comment 40

[Timothy Nikkel (:tnikkel)]

(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.

Comment 41

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

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.

Comment 42

[Timothy Nikkel (:tnikkel)]

(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.

Comment 43

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

(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.

Comment 44

[Timothy Nikkel (:tnikkel)]

(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.

Comment 45

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

(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>.

Comment 46

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Something like this.

Comment 47

[Timothy Nikkel (:tnikkel)]

Attachment: patch

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.

Comment 48

[Timothy Nikkel (:tnikkel)]

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 49

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

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

Comment 50

[Joe Drew (not getting mail)]

In order to get tests in finite time, with Ehsan's blessing I am reusing Oak since it's not prioritized too low.

Comment 51

[Joe Drew (not getting mail)]

https://tbpl.mozilla.org/?tree=Oak&rev=199f4171030c

Comment 52

[Joe Drew (not getting mail)]

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.)

Comment 53

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/dcecf246f732

Comment 54

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 622140 [details] [diff] [review]
patch

Let's get this landed asap for today's anticipated Beta 1 go to build.

Comment 55

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/23178dc7e035

Comment 56

[Cristian Nicolae (:xti)]

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