Closed Bug 1281425 Opened 8 years ago Closed 7 years ago

[Wayland] - Implement CompositorWidget - display connection

Categories

(Core :: Widget: Gtk, enhancement, P2)

Product:
Core
Component:
Widget: Gtk
Platform:
All
Linux
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 2 open bugs)

Details

(Whiteboard: tpi:+)

Attachments

(3 files, 1 obsolete file)

WIP v.2
76.19 KB, patch
Details | Diff | Splinter Review
Bug 1281425 - Added nsWaylandDisplay class to support access to Wayland display,
59 bytes, text/x-review-board-request
jhorak
: review+
Details
Bug 1281425 - Implement wayland loop to process wl_display events for threads and bind wl_registry for wl_shm,
59 bytes, text/x-review-board-request
jhorak
: review+
Details 
We need to fix Wayland port to render pages with OMTC. Right now needs:

layers.offmainthreadcomposition.enabled: false
or
layers.offmainthreadcomposition.force-disabled: true
Priority: -- → P5
Whiteboard: tpi:+
It's completely broken now, looking at it. The implementation seems to be fairy straightforward, see some shm_* examples at https://jan.newmarch.name/Wayland/WhenCanIDraw/ for instance.
Summary: [Wayland] - Web content is not shown when OMTC is enabled → [Wayland] - Implement CompositorWidget
Blocks: gem
I'm now trying to enable CompositorOGL on ESR45 on some SoC such as Renesas's RZ/G1E. I ported your Wayland patches to ESR45 and added some experimental patches to enable GLContextProviderEGL.

Here is the repository:

* Yocto recipe: https://github.com/mozilla-japan/meta-browser
* patches in it: https://github.com/mozilla-japan/meta-browser/tree/firefox-45.0esr/recipes-mozilla/firefox/firefox/wayland-patches

It simply creates wl_egl_window for the GDKWindow then passes it to GLContextProviderEGL.

I confirmed that it can enable CompositorOGL, so drawing CSS transition/transform/animation is efficiently improved.

But it's still heavily broken. When the GTK+ draws the window frame, the content will be hidden, it causes flicker. In addition, drawing position isn't match with actual position.
I think that the main cause of it is that GDKWindow & wl_surface isn't 1:1 relation, it's n:1 relation. The wl_surface for a child GDKWindow is same with it's parent GDKWindow.

Do you have any idea to resolve it?
I'm now investigating how GtkGLArea resolve it.
> But it's still heavily broken. When the GTK+ draws the window frame, the content will be hidden, it causes flicker. In addition, drawing position isn't match with actual position.

BTW the above patches avoid it by a dirty hack (making the window fullscreen).
> I confirmed that it can enable CompositorOGL, so drawing CSS transition/transform/animation is efficiently improved.

For example I use the following site to confirm:

http://ondras.zarovi.cz/demos/nojs/
Attached patch WIP (obsolete) — — Splinter Review 
A very first proof of concept. It uses wl_shm to create shared buffer to draw to wl_subsurface attached to MozContainer window. 

Should build & run with latest trunk but crashes (a bug with a cairo surface attached to GdkWindow) and also causes rendering artifacts caused by wl_buffer/wl_shm_pool resize code.
Attached patch WIP v.2 — — Splinter Review 
Fixed cairo resize crashes, pop-up window rendering, crash at browser end, seems to be roughly working. It needs to improve wl_buffer management, implement wayland event handler for Compositor thread (when OMTC is enabled).
Attachment #8800696 - Attachment is obsolete: true
(In reply to Takuro Ashie from comment #2)
> But it's still heavily broken. When the GTK+ draws the window frame, the
> content will be hidden, it causes flicker. In addition, drawing position
> isn't match with actual position.
> I think that the main cause of it is that GDKWindow & wl_surface isn't 1:1
> relation, it's n:1 relation. The wl_surface for a child GDKWindow is same
> with it's parent GDKWindow.

I solved that by adding wl_subsurface as overlay of wl_surface owned by GdkWindow. Such approach uses clutter-gtk for instance. But the gl rendering should be supported directly by GdkWindow AFAIK, you may not need to fiddle with wl_surfaces.
What's the status here?
Flags: needinfo?(stransky)
It's implemented here:
https://github.com/stransky/gecko-dev/tree/master/widget/gtk

I just need to find time to file patches here. The main problem is to find reviewer at Mozilla.
Flags: needinfo?(stransky)
No longer depends on: gtktitlebar
Depends on: 1337369
Priority: P5 → P2
Summary: [Wayland] - Implement CompositorWidget → [Wayland] - Implement CompositorWidget - Wayland display connection
Summary: [Wayland] - Implement CompositorWidget - Wayland display connection → [Wayland] - Implement CompositorWidget - display connection
Assignee: nobody → stransky
Comment on attachment 8922443 [details]
Bug 1281425 - Added nsWaylandDisplay class to support access to Wayland display,

https://reviewboard.mozilla.org/r/193504/#review200488

::: commit-message-40fd3:1
(Diff revision 1)
> +Bug 1281425 - Add Wayland CompositorWidgetand with base display connection class, r?jhorak

nit: typo in comment

::: widget/gtk/WindowSurfaceWayland.h:23
(Diff revision 1)
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +
> +public:
> +  nsWaylandDisplay(wl_display *aDisplay);
> +
> +  wl_display*         GetDisplay()           { return mDisplay; };

If you don't intend to change return value content, consider using wl_display * const GetDisplay()

::: widget/gtk/WindowSurfaceWayland.cpp:49
(Diff revision 1)
> +}
> +
> +nsWaylandDisplay::~nsWaylandDisplay()
> +{
> +  MOZ_ASSERT(mThreadId == PR_GetCurrentThread());
> +  mDisplay = nullptr;

Please add a brief comment why this does not leak.
Attachment #8922443 - Flags: review?(jhorak) → review-
Comment on attachment 8922443 [details]
Bug 1281425 - Added nsWaylandDisplay class to support access to Wayland display,

https://reviewboard.mozilla.org/r/193504/#review201012

::: widget/gtk/WindowSurfaceWayland.h:23
(Diff revision 1)
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +
> +public:
> +  nsWaylandDisplay(wl_display *aDisplay);
> +
> +  wl_display*         GetDisplay()           { return mDisplay; };

I think it's a bit impractical to return const here as all wayland function prototypes does not use the const modifier. That would mean we need to type cast for all wl_* function cals. Also Gtk+ does not use const for GdkDisplay.
Comment on attachment 8922443 [details]
Bug 1281425 - Added nsWaylandDisplay class to support access to Wayland display,

https://reviewboard.mozilla.org/r/193504/#review201396

::: commit-message-40fd3:1
(Diff revision 2)
> +Bug 1281425 - Add Wayland CompositorWidgetand with base display connection class, r?jhorak

Looks like you've overlooked fixing the commit message:
"Added nsWaylandDisplay to support access to Wayland display"
Attachment #8922443 - Flags: review?(jhorak) → review-
Comment on attachment 8924607 [details]
Bug 1281425 - Implement wayland loop to process wl_display events for threads and bind wl_registry for wl_shm,

https://reviewboard.mozilla.org/r/195832/#review205896

::: widget/gtk/WindowSurfaceWayland.cpp:28
(Diff revision 1)
>  #include <errno.h>
>  
>  namespace mozilla {
>  namespace widget {
>  
> +static nsCOMArray<nsWaylandDisplay> gWaylandDisplays;

The question is how many rendering threads gecko has. If only one (compositor) we can get rid of the array here.

::: widget/gtk/WindowSurfaceWayland.cpp:41
(Diff revision 1)
> +
> +static nsWaylandDisplay* WaylandDisplayGet(wl_display *aDisplay);
> +static void WaylandDisplayRelease(wl_display *aDisplay);
> +static void WaylandDisplayLoop(wl_display *aDisplay);
> +
> +#define EVENT_LOOP_DELAY (1000/60)

The loop fps may be adjusted - I just used 60fps as a guess, there's no heuristic behind.
Comment on attachment 8924607 [details]
Bug 1281425 - Implement wayland loop to process wl_display events for threads and bind wl_registry for wl_shm,

https://reviewboard.mozilla.org/r/195832/#review208912

::: widget/gtk/WindowSurfaceWayland.cpp:27
(Diff revision 1)
>  #include <fcntl.h>
>  #include <errno.h>
>  
>  namespace mozilla {
>  namespace widget {
>  

In the beginning of the file it would be suitable to have some comment about relationship between nsWindow, WindowSurfaceWayland, nsWaylandDisplay and main/compositor threads and how big the gWaylandDisplays is expected to be (possibly adding some logging facility when entry is added or removed).

::: widget/gtk/WindowSurfaceWayland.cpp:62
(Diff revision 1)
> +  if (!waylandDisplay) {
> +    waylandDisplay = new nsWaylandDisplay(aDisplay);
> +    gWaylandDisplays.AppendObject(waylandDisplay);
> +  }
> +
> +  NS_ADDREF(waylandDisplay);

Honestly I'd like to see there something what is more common in sources, ie returning nsWaylandDisplay pointer as parameter, something like: 
https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/widget/gtk/nsPrintOptionsGTK.cpp#98 But I'll leave it up to you to decide.

::: widget/gtk/WindowSurfaceWayland.cpp:190
(Diff revision 1)
>  NS_IMPL_ISUPPORTS(nsWaylandDisplay, nsISupports);
>  
>  nsWaylandDisplay::nsWaylandDisplay(wl_display *aDisplay)
>  {
>    mThreadId = PR_GetCurrentThread();
>    mDisplay = aDisplay;

Please move before the beginning of the constructor implementation

::: widget/gtk/WindowSurfaceWayland.cpp:195
(Diff revision 1)
>    mDisplay = aDisplay;
>  
>    // gfx::SurfaceFormat::B8G8R8A8 is a basic Wayland format
>    // and is always present.
>    mFormat = gfx::SurfaceFormat::B8G8R8A8;
> +  mShm = nullptr;

dtto
Attachment #8924607 - Flags: review?(jhorak) → review-
Comment on attachment 8924607 [details]
Bug 1281425 - Implement wayland loop to process wl_display events for threads and bind wl_registry for wl_shm,

https://reviewboard.mozilla.org/r/195832/#review209320

::: widget/gtk/WindowSurfaceWayland.cpp:27
(Diff revision 1)
>  #include <fcntl.h>
>  #include <errno.h>
>  
>  namespace mozilla {
>  namespace widget {
>  

I'll add that info with WindowSurfaceWayland class commits. It would be awkward to have commented missing code.
Comment on attachment 8924607 [details]
Bug 1281425 - Implement wayland loop to process wl_display events for threads and bind wl_registry for wl_shm,

https://reviewboard.mozilla.org/r/195832/#review205896

> The question is how many rendering threads gecko has. If only one (compositor) we can get rid of the array here.

Added note to the sources for furter investigation.

> The loop fps may be adjusted - I just used 60fps as a guess, there's no heuristic behind.

Added note to the sources for furter investigation.
Comment on attachment 8922443 [details]
Bug 1281425 - Added nsWaylandDisplay class to support access to Wayland display,

https://reviewboard.mozilla.org/r/193504/#review209722
Attachment #8922443 - Flags: review?(jhorak) → review+
Comment on attachment 8924607 [details]
Bug 1281425 - Implement wayland loop to process wl_display events for threads and bind wl_registry for wl_shm,

https://reviewboard.mozilla.org/r/195832/#review209750
Attachment #8924607 - Flags: review?(jhorak) → review+
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/d33c72578b62
Added nsWaylandDisplay class to support access to Wayland display, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/d0cbae7ed8d1
Implement wayland loop to process wl_display events for threads and bind wl_registry for wl_shm, r=jhorak
https://hg.mozilla.org/mozilla-central/rev/d33c72578b62
https://hg.mozilla.org/mozilla-central/rev/d0cbae7ed8d1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: