[Wayland] - Implement CompositorWidget - display connection

RESOLVED FIXED in Firefox 59

Status

()

Core
Widget: Gtk
P2
enhancement
RESOLVED FIXED
2 years ago
18 days ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla59
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected, firefox59 fixed)

Details

(Whiteboard: tpi:+)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
We need to fix Wayland port to render pages with OMTC. Right now needs:

layers.offmainthreadcomposition.enabled: false
or
layers.offmainthreadcomposition.force-disabled: true
Depends on: 1283299
Priority: -- → P5
Whiteboard: tpi:+
(Assignee)

Comment 1

a year ago
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

Updated

a year ago
Blocks: 1305341

Comment 2

a year ago
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.

Comment 3

a year ago
> 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).

Comment 4

a year ago
> 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/

Comment 5

a year ago
> 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

For PC:

* https://github.com/mozilla-japan/gecko-dev/tree/esr45-wayland-with-egl
(Assignee)

Comment 6

a year ago
Created attachment 8800696 [details] [diff] [review]
WIP

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.
(Assignee)

Comment 7

a year ago
Created attachment 8807340 [details] [diff] [review]
WIP v.2

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
(Assignee)

Comment 8

a year ago
(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.
Comment hidden (spam)

Comment 10

4 months ago
What's the status here?
Flags: needinfo?(stransky)
(Assignee)

Comment 11

3 months ago
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)
(Assignee)

Updated

2 months ago
No longer depends on: 1283299
(Assignee)

Updated

2 months ago
Depends on: 1337369
Priority: P5 → P2
(Assignee)

Updated

2 months ago
Summary: [Wayland] - Implement CompositorWidget → [Wayland] - Implement CompositorWidget - Wayland display connection
(Assignee)

Updated

2 months ago
Summary: [Wayland] - Implement CompositorWidget - Wayland display connection → [Wayland] - Implement CompositorWidget - display connection
(Assignee)

Updated

2 months ago
Assignee: nobody → stransky
Comment hidden (mozreview-request)

Comment 13

2 months ago
mozreview-review
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-
(Assignee)

Comment 14

2 months ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

2 months ago
mozreview-review
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-
(Assignee)

Comment 18

a month ago
mozreview-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 19

20 days ago
mozreview-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/#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-
(Assignee)

Comment 20

19 days ago
mozreview-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.
(Assignee)

Comment 21

19 days ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

18 days ago
mozreview-review
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 25

18 days ago
mozreview-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+

Comment 26

18 days ago
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
Last Resolved: 18 days 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.