Closed Bug 1386483 Opened 7 years ago Closed 7 years ago

JSFiddle content suddenly disappear in layers-free mode.

Categories

(Core :: Graphics: WebRender, defect)

Product:
Core
Component:
Graphics: WebRender
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: mtseng, Assigned: kats)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1386483 - Split the wr_dp_push_clip method into two to allow reusing clips.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1386483 - Push, push, push the clips, caching all the way.
59 bytes, text/x-review-board-request
jrmuizel
: review+
mstange
: review+
Details 
STR:

1. Turn on layers-free perference.
2. Open https://jsfiddle.net/

Expected result:
The content display normal.

Actual result:
The content suddenly disappear and appear when the caret is blink.
The code editor of jsfiddle draw custom caret instead of gecko's caret. But when I debug this bug, I saw both custom and gecko caret appear. The way that jsfiddle hide the gecko caret is wrap a textarea with a zero height div and this div is overflow hidden. When I analyzed the display lists, The bound/clip of caret display item is not empty, but the cllpchain contains 3 clips and one of this clips is empty. Maybe we didn't handle clipchain properly in layers-free mode. I'll keep looking into this. Blow is the display item info of caret.

Caret p=0x197a8f958 f=0x1979596f8(Frame(br)(0)) bounds(13740,16410,60,960) layerBounds(0,0,60,960) visible(13740,16410,240,60) componentAlpha(0,0,0,0) clip(13740,16410,240,60) asr(<0x197af54c0>, <0x115074d78>) clipChain(<13740,16410,240,60> [0x197af54c0], <13740,16410,180,0> [0x115074d78], <0,0,57540,54780> [root asr]) ref=0x115073b00 agr=0x197af5d20
It turns out that we didn't set clip for each item. So there are some unwanted items got rendered on the screen causes flicker. I'll attach a patch that set clip properly.
Assignee: nobody → mtseng
Comment on attachment 8893279 [details]
Bug 1386483 - Set clip for each item in layers-free mode.

https://reviewboard.mozilla.org/r/164334/#review169764

Nice, this seems pretty clean, and I can build on this to set the rest of the scrolling layers stuff too.

::: gfx/layers/wr/ScrollingLayersHelper.cpp:122
(Diff revision 1)
> +    LayerRect clip = ViewAs<LayerPixel>(
> +      LayoutDeviceRect::FromAppUnits(itemClip.GetClipRect(), appUnitsPerDevPixel),
> +      PixelCastJustification::MovingDownToChildren);

The MovingDownToChildren justification doesn't make sense here. I would just compute this as a LayoutDeviceRect and skip the conversion to LayerRect. There is an overload of StackingContextHelper::ToRelativeLayoutRect that takes a LayoutDeviceRect so calling that should be fine.
Attachment #8893279 - Flags: review?(bugmail) → review+
Comment on attachment 8893279 [details]
Bug 1386483 - Set clip for each item in layers-free mode.

https://reviewboard.mozilla.org/r/164334/#review169906

It looks like this Pushes and Pops a clip for every item. I'd like us to instead more closely match the Gecko clip tree.

i.e. something like: 
https://public.etherpad-mozilla.org/p/gfx-wr-clip
Attachment #8893279 - Flags: review-
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> i.e. something like: 
> https://public.etherpad-mozilla.org/p/gfx-wr-clip

It seems like the main difference between what's outlined in this etherpad and the patch is that the etherpad generates clip IDs and reuses them rather than constantly pushing and popping the same clip rect. I'll need to do that anyway as I add support for scroll layers, so I don't mind landing this patch as-is for now and then cleaning it up going forward.

From my understanding of the etherpad code, ScrollingLayersHelper (in layers mode) currently does more or less the same thing, but using the layer tree. We "just" need to port it to using the display list instead.
Just as a heads up I'm working on patches to bring this patch more in line with what we want long-term.
Top two patches at https://github.com/staktrace/gecko-dev/commits/apzfree are my WIP. Although I can't reproduce the STR in comment 0. I'll continue testing it on tuesday.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> Just as a heads up I'm working on patches to bring this patch more in line
> with what we want long-term.

Great! Thanks you. I'm going to assign this bug to you.
Assignee: mtseng → bugmail
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> Top two patches at https://github.com/staktrace/gecko-dev/commits/apzfree
> are my WIP. Although I can't reproduce the STR in comment 0. I'll continue
> testing it on tuesday.

Not only the comment 0. There are many issues with clipping. such as:

1. Sometimes reload button is weird. The all animation sprites appear at once.
2. Even though there are multiple tabs opened. But the xul UI only show only one tab.
I spent some time running with my patches locally and they seem to improve some things and not regress anything. It's hard to tell though. I also did some try pushes with [1] and without [2] my patches just see what the results are like with layers-free.

But I think the patches are ok to land for now. I'll see if I can get the rounded corners hooked up as well.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b03a2b8c7fd97ef7a923134dffbf6db771fadb6d
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=642929c67130afdf906bb503cc9981136c2aee08
Comment on attachment 8895038 [details]
Bug 1386483 - Split the wr_dp_push_clip method into two to allow reusing clips.

https://reviewboard.mozilla.org/r/166164/#review171384

Looks great!
Attachment #8895038 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8895039 [details]
Bug 1386483 - Push, push, push the clips, caching all the way.

https://reviewboard.mozilla.org/r/166166/#review171472

This seems like a good start.

It doesn't give WebRender any information about how to scroll these clips, does it?
Attachment #8895039 - Flags: review?(mstange) → review+
Attachment #8893279 - Attachment is obsolete: true
(In reply to Markus Stange [:mstange] from comment #15)
> It doesn't give WebRender any information about how to scroll these clips,
> does it?

Correct, I have not yet implemented the scroll layers bit.
Comment on attachment 8895039 [details]
Bug 1386483 - Push, push, push the clips, caching all the way.

https://reviewboard.mozilla.org/r/166166/#review171834
Attachment #8895039 - Flags: review?(jmuizelaar) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9aeb8ecce1a0
Split the wr_dp_push_clip method into two to allow reusing clips. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/0a754c7cc2c5
Push, push, push the clips, caching all the way. r=jrmuizel,mstange
https://hg.mozilla.org/mozilla-central/rev/9aeb8ecce1a0
https://hg.mozilla.org/mozilla-central/rev/0a754c7cc2c5
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: