physical backing size and overlay panels

[Bo Liu]

Context: Jinsuk is adding ViewRoot with physical backing size being the first call that's moved to propagation through the view tree. But physical backing size requires "specializing" for overlay panels. I think Matt explained to me once that this is because overlay on tablets may not occupy the whole width of the screen (?)

My new proposal: stop specializing physical backing size

Physical backing size is used only for calculating gpu raster tile size. If anyone see any other use case, please let me know.

It's probably not worth keeping this small optimization for gpu raster only for this edge case. Thoughts?

[Alexandre Elias]

Agreed, it doesn't seem worth keeping.  Physical backing size used to be used for a lot more than an optimization (had effects on correctness), perhaps the code dates from that time.

[Jinsuk Kim]

Sounds like I made a great choice of the first use case of ViewRoot : )

I think it is fine not to use the customized size for overlay panel as long as it affects gpu tile size only, and doesn't impact memory usage. 

--
You received this message because you are subscribed to the Google Groups "Slimming Clank" group.
To unsubscribe from this group and stop receiving emails from it, send an email to slimming-clank+unsubscribe@chromium.org.
To post to this group, send email to slimmin...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/slimming-clank/CADeTeo5uUO1yw9WObgW24K%3Dn_xOsyBHqy508%2B0j%3DnW%3DjkerdKA%40mail.gmail.com.

[Jinsuk Kim]

Tried setting the overlay panel physical backing size the same as that of main content (or just not setting at all) -  it gives a gray patch spanning on the right side of the panel. 

Didn't look into deeper, but it is affecting something other than gpu tile size?

​

[David Trainor]

It was originally supposed to dictate the backing texture size because we didn't want to allocate a new texture every time the toolbar became visible or hidden.  I don't know if that changed as we moved to uber comp though.  If we aren't using it to size a texture maybe we want to remove it or rename it to something more appropriate. 

On Mon, Jan 9, 2017 at 11:32 PM Jinsuk Kim <jins...@google.com> wrote:

Tried setting the overlay panel physical backing size the same as that of main content (or just not setting at all) -  it gives a gray patch spanning on the right side of the panel. 

Didn't look into deeper, but it is affecting something other than gpu tile size?

​

On Tue, Jan 10, 2017 at 10:30 AM, Jinsuk Kim <jins...@google.com> wrote:

Sounds like I made a great choice of the first use case of ViewRoot : )

I think it is fine not to use the customized size for overlay panel as long as it affects gpu tile size only, and doesn't impact memory usage. 

On Tue, Jan 10, 2017 at 7:42 AM, Alexandre Elias <ael...@chromium.org> wrote:

Agreed, it doesn't seem worth keeping.  Physical backing size used to be used for a lot more than an optimization (had effects on correctness), perhaps the code dates from that time.

On Mon, Jan 9, 2017 at 12:08 PM, Bo Liu <bo65...@gmail.com> wrote:

Context: Jinsuk is adding ViewRoot with physical backing size being the first call that's moved to propagation through the view tree. But physical backing size requires "specializing" for overlay panels. I think Matt explained to me once that this is because overlay on tablets may not occupy the whole width of the screen (?)

My new proposal: stop specializing physical backing size

Physical backing size is used only for calculating gpu raster tile size. If anyone see any other use case, please let me know.

It's probably not worth keeping this small optimization for gpu raster only for this edge case. Thoughts?

--
You received this message because you are subscribed to the Google Groups "Slimming Clank" group.

To unsubscribe from this group and stop receiving emails from it, send an email to slimming-clan...@chromium.org.

To post to this group, send email to slimmin...@chromium.org.

[Bo Liu]

re that screenshot, this is already broken the other way from the previous refactor: crbug.com/679820

Haven't looked at code either, but looks like I missed some backing size usage..

On Tue, Jan 10, 2017 at 8:05 AM, David Trainor <dtra...@chromium.org> wrote:

It was originally supposed to dictate the backing texture size because we didn't want to allocate a new texture every time the toolbar became visible or hidden.  I don't know if that changed as we moved to uber comp though.  If we aren't using it to size a texture maybe we want to remove it or rename it to something more appropriate. 

On Mon, Jan 9, 2017 at 11:32 PM Jinsuk Kim <jins...@google.com> wrote:

Tried setting the overlay panel physical backing size the same as that of main content (or just not setting at all) -  it gives a gray patch spanning on the right side of the panel. 

Didn't look into deeper, but it is affecting something other than gpu tile size?

​

On Tue, Jan 10, 2017 at 10:30 AM, Jinsuk Kim <jins...@google.com> wrote:

Sounds like I made a great choice of the first use case of ViewRoot : )

I think it is fine not to use the customized size for overlay panel as long as it affects gpu tile size only, and doesn't impact memory usage. 

On Tue, Jan 10, 2017 at 7:42 AM, Alexandre Elias <ael...@chromium.org> wrote:

Agreed, it doesn't seem worth keeping.  Physical backing size used to be used for a lot more than an optimization (had effects on correctness), perhaps the code dates from that time.

On Mon, Jan 9, 2017 at 12:08 PM, Bo Liu <bo65...@gmail.com> wrote:

Context: Jinsuk is adding ViewRoot with physical backing size being the first call that's moved to propagation through the view tree. But physical backing size requires "specializing" for overlay panels. I think Matt explained to me once that this is because overlay on tablets may not occupy the whole width of the screen (?)

My new proposal: stop specializing physical backing size

Physical backing size is used only for calculating gpu raster tile size. If anyone see any other use case, please let me know.

It's probably not worth keeping this small optimization for gpu raster only for this edge case. Thoughts?

--
You received this message because you are subscribed to the Google Groups "Slimming Clank" group.

To unsubscribe from this group and stop receiving emails from it, send an email to slimming-clank+unsubscribe@chromium.org.

To post to this group, send email to slimmin...@chromium.org.

[Bo Liu]

Ahh, I missed this: https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_in_process.cc?rcl=0&l=335

So the physical backing size is used as compositor's viewport, but the blink uses a different size (ie the "regular" one) for its viewport. I'm guessing this is so renderer compositor leave space for top controls? Hmm, if that's the case, then maybe backing size should not be sent through the view tree..

[Alexandre Elias]

The main reason Blink has a different one is because Blink's size is in DIP pixels.  We would get rounding errors if we back-multiplied Blink's viewport by devicescalefactor, so physical_backing_size is the original pixel viewport.

[Jinsuk Kim]

On Wed, Jan 11, 2017 at 4:43 AM, Bo Liu <bo65...@gmail.com> wrote:

re that screenshot, this is already broken the other way from the previous refactor: crbug.com/679820

FYI - merged it to ​https://crbug.com/671967​ which the CL in review is also trying to fix. In case this takes time and can't make it before M57 branch,  I may have to make a quick fix for it or revert https://crrev.com/2502763003/

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/slimming-clank/CAPCJOUo2L8W7bgv8f-iH9rGJu02u5oE7%2BLQJFY_5YdH%3D_xKODQ%40mail.gmail.com.

[Jinsuk Kim]

Let me find an alternative example and substitute PhysicalBackingSIzeChanged to validate the ViewRoot refactoring.

[Jinsuk Kim]

Quoting david's input from code review https://codereview.chromium.org/2595263002/:

> So, I think this is bit complicated because this is the first time we're really

> hitting the case where we want to mix Java event handling and page content event

> handling in the same content area.

> 

> In my head we have a single ViewRoot, but the ViewAndroid children would be

> positioned/ordered so that touch events get propagated to the correct one.  Some

> of these ViewAndroids would delegate events to WebContents, some would delegate

> to UI logic (Java for now).  We're basically building an event dispatching view

> hierarchy right?

> 

> Example overlay view hierarchy.  This might not be the best way to structure it,

> but IIRC this is how the CC layers are constructed today.  Each of these is a

> ViewAndroid, touch handling would check hit testing on the hierarchy until it

> finds a match.

> 

> Root

>  +--- Overlay/Layout Root Thing

>            +--- Overlay Page Content (WebContents backed)

>            +--- Overlay Top Bar (Java backed)

>            +--- Overlay Background (Java backed)

>            +--- Underlying Page Content (WebContents backed)

> 

> When the Overlay builds it's ViewAndroid hierarchy it needs to build some

> ViewAndroids that have Java-backed touch handling delegates.  It can add

> WebContents' ViewAndroid nodes wherever it wants.

> 

> There are a few tricky parts that we need to support for this (I'm sure this

> list isn't comprehensive):

> - Making a Java-backed ViewAndroid touch event delegate.

> - Pinning gestures to a specific ViewAndroid (if a ViewAndroid gets touch down,

> it should get all gestures until touch up).

> - Sizing specific WebContents/ViewAndroid.  This is kind of separate, but we

> should probably answer this sooner rather than later.  Do we drive the

> ViewAndroid size from the rendered frame?  Or do we drive the renderer's size

> from the ViewAndroid?

> 

> What do you think? :)  Does this make sense?

> 

Thanks David for your input. We have ViewAndroid in native and it is only for WebContents-backed page contents. I'm focusing at this moment on how to get the right content (i.e. in which tab, whether for overlay or for main page) that will receive touch events. Right now it is kind of handled by the platform since it chooses the ContentView that gets touch events, and the view simply propagates them down to its CVC and the associated WebContents. But with ViewRoot shared by all the contents, we need extra logic to figure out where they should go. I think ViewRoot should be informed through an API of which content goes on top, ready to get the user input events. And should put the child ViewAndroids in the right order accordingly.

One possible way would be for embedder (say Chrome) to update ViewRoot whenever a certain tab (and its content) becomes active (goes to the top). Overlay panel needs to do the same when it is created or destroyed. I wonder if there was any discussion on this.

[David Trainor]

Yeah right now we ask the LayoutManager for which ContentView should get touch events right?  That goes through this order (1) Overlays (2) Layouts (3) Current Tab to figure out which ContentView should get the events.  It's a pull model but we kind of want to push the right ViewAndroid to the ViewRoot and just always send events to the root, so things are a bit flipped.  I think pushing the right ViewAndroid into the ViewRoot makes sense but we just have to make sure we don't miss a case during the changeover from pull to push.

What other implications does attaching a ViewAndroid to the ViewRoot have currently (specifically around CC layers)?

 

On Wed, Jan 11, 2017 at 10:21 AM, Jinsuk Kim <jins...@google.com> wrote:

Let me find an alternative example and substitute PhysicalBackingSIzeChanged to validate the ViewRoot refactoring.

On Wed, Jan 11, 2017 at 7:01 AM, Jinsuk Kim <jins...@google.com> wrote:

On Wed, Jan 11, 2017 at 4:43 AM, Bo Liu <bo65...@gmail.com> wrote:

re that screenshot, this is already broken the other way from the previous refactor: crbug.com/679820

FYI - merged it to ​https://crbug.com/671967​ which the CL in review is also trying to fix. In case this takes time and can't make it before M57 branch,  I may have to make a quick fix for it or revert https://crrev.com/2502763003/

Haven't looked at code either, but looks like I missed some backing size usage..

On Tue, Jan 10, 2017 at 8:05 AM, David Trainor <dtra...@chromium.org> wrote:

It was originally supposed to dictate the backing texture size because we didn't want to allocate a new texture every time the toolbar became visible or hidden.  I don't know if that changed as we moved to uber comp though.  If we aren't using it to size a texture maybe we want to remove it or rename it to something more appropriate. 

On Mon, Jan 9, 2017 at 11:32 PM Jinsuk Kim <jins...@google.com> wrote:

Tried setting the overlay panel physical backing size the same as that of main content (or just not setting at all) -  it gives a gray patch spanning on the right side of the panel. 

Didn't look into deeper, but it is affecting something other than gpu tile size?

​

On Tue, Jan 10, 2017 at 10:30 AM, Jinsuk Kim <jins...@google.com> wrote:

Sounds like I made a great choice of the first use case of ViewRoot : )

I think it is fine not to use the customized size for overlay panel as long as it affects gpu tile size only, and doesn't impact memory usage. 

On Tue, Jan 10, 2017 at 7:42 AM, Alexandre Elias <ael...@chromium.org> wrote:

Agreed, it doesn't seem worth keeping.  Physical backing size used to be used for a lot more than an optimization (had effects on correctness), perhaps the code dates from that time.

On Mon, Jan 9, 2017 at 12:08 PM, Bo Liu <bo65...@gmail.com> wrote:

Context: Jinsuk is adding ViewRoot with physical backing size being the first call that's moved to propagation through the view tree. But physical backing size requires "specializing" for overlay panels. I think Matt explained to me once that this is because overlay on tablets may not occupy the whole width of the screen (?)

My new proposal: stop specializing physical backing size

Physical backing size is used only for calculating gpu raster tile size. If anyone see any other use case, please let me know.

It's probably not worth keeping this small optimization for gpu raster only for this edge case. Thoughts?

--
You received this message because you are subscribed to the Google Groups "Slimming Clank" group.

To unsubscribe from this group and stop receiving emails from it, send an email to slimming-clan...@chromium.org.

To post to this group, send email to slimmin...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/slimming-clank/CADeTeo5uUO1yw9WObgW24K%3Dn_xOsyBHqy508%2B0j%3DnW%3DjkerdKA%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "Slimming Clank" group.

To unsubscribe from this group and stop receiving emails from it, send an email to slimming-clan...@chromium.org.

To post to this group, send email to slimmin...@chromium.org.

[Jinsuk Kim]

Read layout manager code and found that my initial idea was very naive.. 

Just to clarify some points: ViewAndroid does not represent Java-backed components David mentioned above but only represents WebContents. Nor do we have plans to extend the design in that way, at least to my understanding. That would certainly require a design doc to get many rounds of your input.

At this point my plan on the refactoring around ViewRoot goes as far as letting the layout manager + event filter keep handling the routing to Java-backed components, and having ViewRoot take over the routing that was going to content view (overlay or main contents) only. May be disappointing to you if you're thinking of more grand refactoring - but I hope this is a starting point if we do intend to head that way in the long run.

This raises another question as to how this should be done for other embedders like content shell / webview. I think a similar but much simpler logic intercepting events needs to be in place in the holder of their content view so that the events can be routed to ViewRoot in the same way just like we do it in CompositorViewHolder for Chrome.

I'm now adding a command line flag and putting my work behind it so that any change in the meantime won't affect how it works.

Feel free to let me know if you have suggestions/inputs. 

Thanks,

To unsubscribe from this group and stop receiving emails from it, send an email to slimming-clank+unsubscribe@chromium.org.

To post to this group, send email to slimmin...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/slimming-clank/CADeTeo5uUO1yw9WObgW24K%3Dn_xOsyBHqy508%2B0j%3DnW%3DjkerdKA%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "Slimming Clank" group.

To unsubscribe from this group and stop receiving emails from it, send an email to slimming-clank+unsubscribe@chromium.org.

To post to this group, send email to slimmin...@chromium.org.

[David Trainor]

Thanks Jinsuk!  Responses inline.

On Thu, Jan 19, 2017 at 7:32 PM Jinsuk Kim <jins...@google.com> wrote:

Read layout manager code and found that my initial idea was very naive.. 

Just to clarify some points: ViewAndroid does not represent Java-backed components David mentioned above but only represents WebContents. Nor do we have plans to extend the design in that way, at least to my understanding. That would certainly require a design doc to get many rounds of your input.

Yeah ViewAndroid only backs WebContents and RenderWidgetHostViewAndroid at the moment.

To me, the final goal would be a world where we don't have to maintain screen space positions of all touchable elements in the Java Layout system.  ViewAndroid should be a component that can visually represent compositor-backed content (buttons, WebContents, etc.) *and* handle hit testing for those elements.

At this point my plan on the refactoring around ViewRoot goes as far as letting the layout manager + event filter keep handling the routing to Java-backed components, and having ViewRoot take over the routing that was going to content view (overlay or main contents) only. May be disappointing to you if you're thinking of more grand refactoring - but I hope this is a starting point if we do intend to head that way in the long run.

I think this is great :).  It's a perfect starting point for any eventual refactors we want to do on the Layout side.  Your refactoring goals are to remove ContentView(Core).  That shouldn't include fixing the entire Clank UI framework!  But you're getting us closer and setting us up for success in the future.

I'd be happy to spin up a doc on the usage of ViewAndroid in the LayoutManager to expand on what you're doing here :).

This raises another question as to how this should be done for other embedders like content shell / webview. I think a similar but much simpler logic intercepting events needs to be in place in the holder of their content view so that the events can be routed to ViewRoot in the same way just like we do it in CompositorViewHolder for Chrome.

 

Yeah something will need to propagate the events if we don't just have a ContentView to add to the hierarchy.  If there isn't a CompositorView, we'll have to find another View that currently does the same job (or add one) and route the events to the ViewRoot like you suggest.

I'm now adding a command line flag and putting my work behind it so that any change in the meantime won't affect how it works.

 

Sounds good! 

 

To unsubscribe from this group and stop receiving emails from it, send an email to slimming-clan...@chromium.org.

To post to this group, send email to slimmin...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/slimming-clank/CADeTeo5uUO1yw9WObgW24K%3Dn_xOsyBHqy508%2B0j%3DnW%3DjkerdKA%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "Slimming Clank" group.

To unsubscribe from this group and stop receiving emails from it, send an email to slimming-clan...@chromium.org.

To post to this group, send email to slimmin...@chromium.org.

[David Trainor]

+Khushal Sagar