Simplify existing solution for IPC channel communication

[kalyankondapally]

IPC channel communication:

Background:
Gpu Process acts as the wayland client per browser instance. All realized widgets are created on GPU process side. Browser process needs to send any widget state change, e.g. Min, Max, FullScreen information to DisplayChannel. This information would be used to update wayland shell. Also, any output related information (e.g. output size) needs to be passed from GPU process to Browser process.

Current Solution:
DisplayChannelHost: Message filter on Browser process side.
DisplayChannel: Ipc listener on GPU process side.

On Browser Process Side:

We observe for any child process being forked by Browser process. If child process is GPU process, we add DisplayChannelHost as one of the BrowserMessageFilter. DisplayChannelHost listens to any messages specific to Wayland coming from GPU process side and handles them appropriately.

On GPU process side:

DisplayChannel is responsible for listening to any routed messages coming from Browser process and handles them appropriately. GPUChildThread needs to be created before DisplayChannel can be added as listener. InitializeHardware gets called very early (before GPUChildThread is created) and hence not the right place to set up DisplayChannel as listener. Thus, we use RealizeWidget to do this, which is wrong.

Issues:
1)In order to avoid changes in chromium, we have been adding work around's as to how and when listeners are installed. This has complicated the existing solution.
2)OutputSize notification is too late. The value returned by DefaultDisplaySpec is used by widget as the work area of current screen. This information needs to come from GPU side and is needed before any calculations related to widget bounds are done. With the current solutions, it's already too late as to when the listeners are setup and communication happens between them.

Proposal:
On Browser Process side:
Set DisplayChannelHost as message filter as soon as GPUProcesshost is created.
On GPU Process side:
Set DisplayChannel as IPC listener as soon as GPUChildThread is created.

Remove any un-necessary complication from the solution, even if it means having to maintain minor changes in Chromium.

Single and Multi Process should work.

Not part of this CL:
1)Handle GPU crash and re-launch. This is something we might need to handle in future.

[kalyankondapally]

Initial solution can be found here: https://github.com/kalyankondapally/ozone-wayland/commits/IPCHandShake

[kalyankondapally]

This is how it would work after these changes:

Single Process:

1. Browser process is launched. SurfaceFactory calls InitializeHardware in constructor and initializes appropriate resources.
2. Once GPUProcessHost is successfully initialized, DelayedChannelHostInitialization is called. Here, we force a synchronized wayland flush to ensure any pending wayland events are handled. Then, eventfactory is created and starts polling on display fd.

Multi-Process:
Browser Process:

1. Browser process is launched. SurfaceFactory calls InitializeHardware in constructor and initializes appropriate resources.
2. GPUProcessHost is created. This would result in forking GPU process.
3. Once GPUProcessHost is successfully initialized, DelayedChannelHostInitialization is called. This would set DisplayChannelHost as one of the message filters of browser process.

GPUProcess:

1. Once GPU Process is forked, SurfaceFactory calls InitializeHardware in constructor and initializes appropriate resources. Wayland connection is made here and we start polling on an IO thread.
2. DelayedChannelInitialization is called as soon as GPUChildThread is created and set as main thread. This would initialize ipc channel communication with browser process.

OutPutMode Synchronizaton:

DelayedChannelInitialization ensures that any initial output size notification is sent to browser process. In case the event hasn't arrived yet, we force a synchronized wayland flush.

[tiagovignatti]

per discussion on IRC with @kalyankondapally, I'll prepare a branch to be reviewed with the changes for clean up first before going with the discussions on the solution we're intending to do. I'll be posting here it.

[tiagovignatti]

alright. I'd like to push to master the following. Please review it @kalyankondapally: https://github.com/01org/ozone-wayland/tree/ipc-cleanup

[tiagovignatti]

this branch now contains the after-review changes:
https://github.com/01org/ozone-wayland/tree/ipc-cleanup-v2

[tiagovignatti]

@kalyankondapally, I read the description you put in #93 (comment) and I think that's indeed a good approach to follow. Thank you for summarizing. I have two main worries though, so let's make sure we'll be covering them next:

1. Chrome browser initiates its frame creation (BrowserFrame::InitBrowserFrame) quite early and relies on the output properties from DefaultDisplaySpec (therefore, the Wayland connection has to be established already and so on). This happens at PreMainMessageLoopRun time, i.e. prior the main loop starts to run. So our next solution must fix this behavior, because at the moment everything happens in InitializeHardware's time as task of the scheduler, when the main loop is already up & running.
2. I've checked your idea of splitting InitializeHardware in three stages, calling at GPUChildThread and GpuProcessHost for both GPU and browser processes respectively. I think they're fine, but it's not clear to me if it make sure that any order of the execution works for establishing the connection between the two channels. I.e. we cannot guarantee a certain order of scheduling the GPU process before the browser's, or vice-versa. And we're talking about four process here (GPU process, GPU IO dispatching thread, browser and browser IO dispatching thread), which makes it even more tricky to solve. So are we covering that any order of channel establishing, coming at any time, just works?

finally, I've seen you pulled in master my "ipc-cleanup-v2" branch. Thanks. Maybe now you want to follow-up with your clean-ups before we go to code the solution for the biggest problem?

[kalyankondapally]

@tiagovignatti
1)Reg Chromium:

I am bit concerned with this. This happens way before GPUProcessHost is created(i.e GPU process is forked). I would prefer one of the following solutions for this:
1)Have a env variable like Ozone does.
2)Read from weston.ini file or from same place wayland gets this information?
3)Create a temporary wayland client connection on browser process side, get screen output and close it ? As this is an issue only at start up.

Reg GPU process & Browser Process:
GPU process is always forked by Browser process. In a sense, we are sure that displaychannelhost is set as message filter by the time GPU process starts dispatching messages.

Probably you meant 4 different threads:
1)GPU main thread.
2)GPU IO thread.
3)Browser main thread.
4)Browser IO thread.

Yes, these changes ensure that all these threads are initialized before appropriate functions are called.

It looks for me that there are two different things to solve:
1)IPC Communication simplification and robustness.
2)A unique solution to get preferred Screen size at start up.

As dust has settled down, I am in no hurry to get my changes in as yet. We can take time and carefully go through this. I would like to have a concrete solution before starting to make any changes for the code. We would need to live with this for some time and thus would like to ensure both of us are happy with it. As of now the branch https://github.com/kalyankondapally/ozone-wayland/commits/IPCHandShake implements the proposal.

[kalyankondapally]

Lets please use this as a discussion bug and create appropriate tasks as needed.
Tasks:
1)Chromium/general solution to get screen bounds at startup. #95
2)Add synchronized flush support in dispatcher. #96
3)Fix ownership of cursor data. #97

[tiagovignatti]

alright, thanks for the explanation on those two points I was worried.

I've liked that you separated the issues in different tasks. Let's follow now there instead.

[tiagovignatti]

@kalyankondapally, can you please review this one:
https://github.com/tiagovignatti/ozone-wayland/tree/output-lookahead

I think it pretty much summarizes a bunch of what we've been discussing lately. It doesn't deal directly with IPC channel handshake though -- I'd like to solve this anyway next so we won't have those ugly hacks in OzoneDisplay::RealizeAcceleratedWidget() and so on. Another thing is that Chrome browser now starts alright but gets a pretty small root window. We need to solve it and I think there's some changes that happened upstream that it went unnoticed.

If you want/prefer, feel free to take over this series until I'll be online again hacking, next Monday (07.10).

[kalyankondapally]

@tiagovignatti As we seem to be happy with the above mentioned proposal, and it is already implemented in the above mentioned branch. I will break that up for review.

I will not hijack your OutMode fixes as my interest was only to get the IPC communication simplified. I am quite happy with it now.

[tiagovignatti]

yes. I see that the order of patches go like this:

1. "ipc-cleanup-v2" (already applied)
2. "output-lookahead" (needs some adjustments still, after your comments)
3. "IPCHandShake" (needs to wait these two work before land first)

[kalyankondapally]

To be honest 2 and 3 are not related. I see what you mean though (merge conflicts :( ).

As agreed in IRC:
Polished step 2 and prepared it for review: https://github.com/kalyankondapally/ozone-wayland/commits/Output-Review

I will breakdown IPCHandShake branch now.

[kalyankondapally]

k, Now changes related to IPC Simplification are here:
https://github.com/kalyankondapally/ozone-wayland/tree/IPCSimplification/

For now, I have adopted your earlier approach of registering Listener in BrowserProcess side during GetAcceleratedWidget call. In GPU process side it will initiate the connection once GPU Child thread is created.
I haven't added the api to SFO yet but at OzoneWayland level. I will make necessary changes once api is agreed upstream(I am setting up the review soon).

I have created a separate task for initializing IPC communication on Browser Process side as this should also handle cases when GPU process is re-spawned. #102
Here, I will check if we need a separate entry point in Ozone, or can we do without it.
Discussed with Rob about adding an entry point on Browser process side. We should avoid this and use BrowserChildProcessObserver as we currently do. I am going to test this once and open IPCSimplification branch for review.

[kalyankondapally]

I would actually like to propose to delay this task till #29 is handled, as this will greatly influence the solution. I will start with 29 and update any IPC related comments here. This would give us a complete picture and a better understanding of what we need.

@tiagovignatti In the mean time, I think we can integrate your proposal with out the Establish Channel changes. I will try to separate your proposal and put it up for review.

[tiagovignatti]

makes perfect sense for me. I'll be waiting the remaining patches for review then. Thanks.

[kalyankondapally]

@tiagovignatti pls review here: kalyankondapally@aa5f1ec

[kalyankondapally]

I think we are able to close this now.