Closed Bug 916125 Opened 6 years ago Closed 6 years ago

When displayports are set on elements, their layers are not always clipped correctly

Categories

(Core :: Graphics: Layers, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- verified

People

(Reporter: kats, Assigned: tnikkel)

References

Details

(Keywords: regression)

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #898444 +++

Based on snorp's investigation in bug 898444 and some more digging around I did, I believe that the code in AsyncCompositionManager.cpp is missing some code to set a clip rect on the shadow layers. In a nutshell, if a subframe is async-scrollable and has a displayport, then the visible region for the layer will be the entire displayport (which is the painted area). Prior to multi-apzc this never happened so we never hit this case. But now that we do this, we also need to clip that layer so that instead of drawing the entire displayport it will only draw the part of it that's inside the scroll port (which should be the mCompositionBounds, possibly transformed into the right coordinate space).
Just from a quick code examination, I think what needs to be done here is to modify this code: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#162

Instead of just setting the clip rect (possibly with translation) on the shadow layer, it should also intersect the (pre-translation) clip rect with the composition bounds of the layer, if the layer is a scrollable container layer. I suspect that the composition bounds you want to use there is mCompositionBounds / mFrameMetrics.GetParentResolution() but I'm not even 60% sure of that.
This might be related to the clipping problem in bug 886969.
Matt, what do you think of comment 1?
Flags: needinfo?(matt.woodrow)
I'm not sure I understand the problem entirely, but if we're creating a scrollable layer that's bigger than its viewport, then the clipping to restrict compositing to that rect probably should be set up by content.

I'd expect nsGfxScrollFrame (where we decide on the display port) to also setup clipping to the viewport.
Flags: needinfo?(matt.woodrow)
If nsGfxScrollFrame sets up the clip, will the clip be transformed appropriately when the layer is async-scrolled? I don't really understand how the clip is affected by transformations.
Why does the clip need to change? Isn't the viewport (scrollport?) staying in the same spot?
The scrollport of a layer stays in the same spot if you scroll that layer, yes. If you scroll a parent layer then the scrollport might move. So it is possible for the clip will move relative to the top-left corner of the layer as well as relative to the physical screen. I don't know if the clip is defined in such a way that both of these are handled automatically or not.
blocking-b2g: koi? → ---
Assignee: nobody → bugmail.mozilla
blocking-b2g: --- → koi+
I don't think I'm the right person to be assigned this bug. I don't know enough about the clipping code to really work on this.

Note that there are existing clipping bugs in Fennec - bug 866585, bug 854873 (+dupes, dependencies) that may or may not be related, so maybe somebody who knows the code should tackle all of them together.
Flags: needinfo?(milan)
Nick, is this something you can handle?  1.2 blocker.
Assignee: bugmail.mozilla → ncameron
Flags: needinfo?(milan) → needinfo?(ncameron)
I hate to shirk on a bug, but I don't think I can fix this quickly (I'm not familiar with either apzc or layout's clipping) and I only have two days until I go on PTO. This doesn't look like it is related with the clipping bugs I fixed last week.
Flags: needinfo?(ncameron)
Jet, who can look at this layout clipping bug?
Flags: needinfo?(bugs)
Timothy, is this in the part of the code you know?
Flags: needinfo?(bugs) → needinfo?(tnikkel)
Hmm, depends on which code needs changing. AsyncCompositionManager.cpp not so much. But while looking into this now I noticed that in nsGfxScrollFrameInner::BuildDisplayList the clip that we use it the display port, which doesn't seem right. If we don't need the clip to be updated async-ly then we should be able to use the right clip in nsGfxScrollFrameInner::BuildDisplayList. The clip shouldn't change if we are just aynsc scrolling the sub-element, but if we are async scrolling the parent that might not work.

kats, comment 0 implies we should be applying this clip in AsyncCompositionManager, but if the clip doesn't need to be updated async then it seems like it would make sense to do it in nsGfxScrollFrameInner::BuildDisplayList. What do you think?
Flags: needinfo?(tnikkel) → needinfo?(bugmail.mozilla)
I think ScrollLayerWrapper should set a cliprect on the nsDisplayScrollLayers it creates, clipping them to the scrollport. That should take care of everything.
Assignee: ncameron → tnikkel
(In reply to Timothy Nikkel (:tn) from comment #13)
> But while looking into this now I noticed that in
> nsGfxScrollFrameInner::BuildDisplayList the clip that we use it the display
> port, which doesn't seem right.

Yeah that doesn't seem right.

> If we don't need the clip to be updated
> async-ly then we should be able to use the right clip in
> nsGfxScrollFrameInner::BuildDisplayList. The clip shouldn't change if we are
> just aynsc scrolling the sub-element, but if we are async scrolling the
> parent that might not work.
>
> kats, comment 0 implies we should be applying this clip in
> AsyncCompositionManager, but if the clip doesn't need to be updated async
> then it seems like it would make sense to do it in
> nsGfxScrollFrameInner::BuildDisplayList. What do you think?

I still don't know what the clip is interpreted relative to so it makes it hard to answer this. Say you have a container layer A with a child layer B, and there is a clip set on layer B. Because of the clip you can currently only see the centermost pixel of layer B's contents. Now, if we set a transform on layer B, do you still see the centermost pixel of layer B? What if you set a transform on layer A? If the answers are "no" and "yes" respectively then I think yes, the clip should be settable in layout code and everything will work.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> I still don't know what the clip is interpreted relative to so it makes it
> hard to answer this. Say you have a container layer A with a child layer B,
> and there is a clip set on layer B. Because of the clip you can currently
> only see the centermost pixel of layer B's contents. Now, if we set a
> transform on layer B, do you still see the centermost pixel of layer B?

No. Layer::SetClipRect sets a clip that is applied after B's transform. See the comment in Layers.h:
The coordinates are relative to
   * the parent layer (i.e. the contents of this layer
   * are transformed before this clip rect is applied).

> What
> if you set a transform on layer A? If the answers are "no" and "yes"
> respectively then I think yes, the clip should be settable in layout code
> and everything will work.

Those are the answers. Yes, comment #14 should work :-).
Attached patch patchSplinter Review
This seems to fix the problem.
Attachment #818695 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/2668b492d852
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
\o/ Thanks! Verified this fixes the issue on my test page. Tested on 1.2 on a hamachi device.
Depends on: 928441
You need to log in before you can comment on or make changes to this bug.