Closed Bug 1422966 Opened 6 years ago Closed 6 years ago

[Wayland] - Implement WindowSurfaceWayland

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla59

Tracking Flags:

Tracking

Status

firefox59

---

fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1422966 - Comment Wayland rendering scheme, 6 years ago Martin Stránský [:stransky] (ni? me) 59 bytes, text/x-review-board-request	jhorak : review+	Details
Bug 1422966 - Update mDisplay attribute initialization at nsWaylandDisplay::nsWaylandDisplay(), 6 years ago Martin Stránský [:stransky] (ni? me) 59 bytes, text/x-review-board-request	jhorak : review+	Details
Bug 1422966 - implement WaylandShmPool to manage shared memory for wl_buffer, 6 years ago Martin Stránský [:stransky] (ni? me) 59 bytes, text/x-review-board-request	jhorak : review+	Details
Bug 1422966 - implemented WindowBackBuffer to encapsulate wl_buffer, 6 years ago Martin Stránský [:stransky] (ni? me) 59 bytes, text/x-review-board-request	jhorak : review+	Details
Bug 1422966 - Implement SurfaceWayland for Wayland, 6 years ago Martin Stránský [:stransky] (ni? me) 59 bytes, text/x-review-board-request	jhorak : review+	Details

Martin Stránský [:stransky] (ni? me)

Assignee

Description

•

6 years ago

We need to implement WindowSurface for Wayland.

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/205196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205196/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/205198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205198/

Comment hidden (mozreview-request)

We allocate shared memory (shm) by mmap(..., MAP_SHARED,...) as an interface between wayland based application
and wayland compositor. We draw our graphics data to the shm and handle to wayland compositor by wl_buffer/wl_surface.

WaylandShmPool acts as a manager of the allocated memory. Allocates it, holds reference to it and releases it.

Review commit: https://reviewboard.mozilla.org/r/205200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205200/

Comment hidden (mozreview-request)

wl_buffer is a main Wayland object with graphics data. wl_buffer basically represent one complete window screen.
When double buffering is involved every window (GdkWindow in our case) utilises two wl_buffers which are cycled.
One is filed with data by application and one is rendered by compositor.

WindowBackBuffer class manages one wl_buffer. It owns wl_buffer objects, owns WaylandShmPool (which provides shared memory)
and ties them together.

Review commit: https://reviewboard.mozilla.org/r/205202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205202/

Comment hidden (mozreview-request)

WindowSurfaceWayland is Wayland implementation of WindowSurface class. One WindowSurfaceWayland object manages drawing of one nsWindow so those are tied 1:1. It implements base Lock() and Commit() interfaces from WindowSurface. At Wayland side it represents one wl_surface object.

To perform visualiation of nsWindow, WindowSurfaceWayland contains one wl_surface and two wl_buffer (by WindowBackBuffer) objects (as we use double buffering). When nsWindow drawing is finished to wl_buffer, the wl_buffer is attached to wl_surface and it's sent to Wayland compositor.

Review commit: https://reviewboard.mozilla.org/r/205394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205394/

Mats Palmgren (inactive)

Comment 6

•

6 years ago

mozreview-review

Comment on attachment 8934459 [details]
Bug 1422966 - Implement SurfaceWayland for Wayland,

https://reviewboard.mozilla.org/r/205394/#review211054

::: widget/gtk/WindowSurfaceWayland.h:121
(Diff revision 1)
> +  bool                      mDelayedCommit;
> +  bool                      mFullScreenDamage;
> +  MessageLoop*              mWaylandMessageLoop;
> +  bool                      mIsMainThread;

minor nit: you're introducing alignment padding here - I suggest you move all bool members to the end to avoid that

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 7

•

6 years ago

mozreview-review

Comment on attachment 8934459 [details]
Bug 1422966 - Implement SurfaceWayland for Wayland,

https://reviewboard.mozilla.org/r/205394/#review211058

::: widget/gtk/WindowSurfaceWayland.h:121
(Diff revision 1)
> +  bool                      mDelayedCommit;
> +  bool                      mFullScreenDamage;
> +  MessageLoop*              mWaylandMessageLoop;
> +  bool                      mIsMainThread;

Thanks! Feel free to help review those patches.

Jan Horak [:jhorak]

Comment 8

•

6 years ago

mozreview-review

Comment on attachment 8934455 [details]
Bug 1422966 - Comment Wayland rendering scheme,

https://reviewboard.mozilla.org/r/205196/#review213392

Attachment #8934455 - Flags: review?(jhorak) → review+

Jan Horak [:jhorak]

Comment 9

•

6 years ago

mozreview-review

Comment on attachment 8934456 [details]
Bug 1422966 - Update mDisplay attribute initialization at nsWaylandDisplay::nsWaylandDisplay(),

https://reviewboard.mozilla.org/r/205198/#review213396

Attachment #8934456 - Flags: review?(jhorak) → review+

Jan Horak [:jhorak]

Comment 10

•

6 years ago

mozreview-review

Comment on attachment 8934457 [details]
Bug 1422966 - implement WaylandShmPool to manage shared memory for wl_buffer,

https://reviewboard.mozilla.org/r/205200/#review213398

Please address commends, otherwise looks good.

::: widget/gtk/WindowSurfaceWayland.cpp:374
(Diff revision 1)
> +  return fd;
> +}
> +
> +WaylandShmPool::WaylandShmPool(nsWaylandDisplay* aWaylandDisplay, int aSize)
> +{
> +  mAllocatedSize = aSize;

please initialize by : mAllocatedSize(aSize)

::: widget/gtk/WindowSurfaceWayland.cpp:384
(Diff revision 1)
> +  MOZ_RELEASE_ASSERT(mImageData != MAP_FAILED,
> +                     "Unable to map drawing surface!");
> +
> +  mShmPool = wl_shm_create_pool(aWaylandDisplay->GetShm(),
> +                                mShmPoolFd, mAllocatedSize);
> +  wl_proxy_set_queue((struct wl_proxy *)mShmPool, aWaylandDisplay->GetEventQueue());

This call could be briefly commented.

Attachment #8934457 - Flags: review?(jhorak) → review+

Jan Horak [:jhorak]

Comment 11

•

6 years ago

mozreview-review

Comment on attachment 8934458 [details]
Bug 1422966 - implemented WindowBackBuffer to encapsulate wl_buffer,

https://reviewboard.mozilla.org/r/205202/#review213404

::: commit-message-7bfef:7
(Diff revision 1)
> +
> +wl_buffer is a main Wayland object with graphics data. wl_buffer basically represent one complete window screen.
> +When double buffering is involved every window (GdkWindow in our case) utilises two wl_buffers which are cycled.
> +One is filed with data by application and one is rendered by compositor.
> +
> +WindowBackBuffer class manages one wl_buffer. It owns wl_buffer objects, owns WaylandShmPool (which provides shared memory)

"It owns wl_buffer object" (only one)

::: widget/gtk/WindowSurfaceWayland.cpp:506
(Diff revision 1)
> +WindowBackBuffer::Detach()
> +{
> +  mAttached = false;
> +}
> +
> +bool WindowBackBuffer::Sync(class WindowBackBuffer* aSourceBuffer)

Maybe more appropriate name should be there: like SetImageDataFromBackBuffer.

::: widget/gtk/WindowSurfaceWayland.cpp:509
(Diff revision 1)
> +}
> +
> +bool WindowBackBuffer::Sync(class WindowBackBuffer* aSourceBuffer)
> +{
> +  bool bufferSizeMatches = MatchSize(aSourceBuffer);
> +  if (!bufferSizeMatches) {

if (!MatchSize(aSourceBuffer)) should be enough.

::: widget/gtk/WindowSurfaceWayland.cpp:513
(Diff revision 1)
> +  bool bufferSizeMatches = MatchSize(aSourceBuffer);
> +  if (!bufferSizeMatches) {
> +    Resize(aSourceBuffer->mWidth, aSourceBuffer->mHeight);
> +  }
> +
> +  memcpy(mShmPool.GetImageData(), aSourceBuffer->mShmPool.GetImageData(),

Since the mShmPool object is modified, could you add WaylandShmPool::(Copy/Set)ImageData(WaylandShmPool* shmPool) which encapsulate the memcpy call?

Attachment #8934458 - Flags: review?(jhorak) → review-

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 12

•

6 years ago

mozreview-review

Comment on attachment 8934459 [details]
Bug 1422966 - Implement SurfaceWayland for Wayland,

https://reviewboard.mozilla.org/r/205394/#review213418

::: widget/gtk/WindowSurfaceWayland.h:123
(Diff revision 1)
> +  WindowBackBuffer*         mBackBuffer;
> +  wl_callback*              mFrameCallback;
> +  wl_surface*               mFrameCallbackSurface;
> +  bool                      mDelayedCommit;
> +  bool                      mFullScreenDamage;
> +  MessageLoop*              mWaylandMessageLoop;

We should call MessageLoop::current() directly.

Jan Horak [:jhorak]

Comment 13

•

6 years ago

mozreview-review

Comment on attachment 8934459 [details]
Bug 1422966 - Implement SurfaceWayland for Wayland,

https://reviewboard.mozilla.org/r/205394/#review213416

::: commit-message-5c6df:3
(Diff revision 1)
> +Bug 1422966 - Implement SurfaceWayland for Wayland, r?jhorak
> +
> +WindowSurfaceWayland is Wayland implementation of WindowSurface class. One WindowSurfaceWayland object manages drawing of one nsWindow so those are tied 1:1. It implements base Lock() and Commit() interfaces from WindowSurface. At Wayland side it represents one wl_surface object.

nit please wrap at 80 characters

::: widget/gtk/WindowSurfaceWayland.h:115
(Diff revision 1)
> +  void                      FrameCallbackHandler();
> +
> +private:
> +  WindowBackBuffer*         GetBufferToDraw(int aWidth, int aHeight);
> +
> +  nsWindow*                 mWidget;

Maybe stick to mWindow when not using nsIWidget, mWidget can mean a lot of other stuff.

::: widget/gtk/WindowSurfaceWayland.h:120
(Diff revision 1)
> +  nsWindow*                 mWidget;
> +  nsWaylandDisplay*         mWaylandDisplay;
> +  WindowBackBuffer*         mFrontBuffer;
> +  WindowBackBuffer*         mBackBuffer;
> +  wl_callback*              mFrameCallback;
> +  wl_surface*               mFrameCallbackSurface;

Hm, does this has something to do with the callback as var name suggests?

::: widget/gtk/WindowSurfaceWayland.cpp:664
(Diff revision 1)
> +      wl_surface_damage(waylandSurface, r.x, r.y, r.width, r.height);
> +    }
> +  }
> +
> +  // Frame callback is always connected to actual wl_surface. When the surface
> +  // is unmapped/deleted the frame callback is newer called. Unfortunatelly

typo: newer - never

Attachment #8934459 - Flags: review?(jhorak) → review-

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 14

•

6 years ago

mozreview-review

Comment on attachment 8934459 [details]
Bug 1422966 - Implement SurfaceWayland for Wayland,

https://reviewboard.mozilla.org/r/205394/#review213602

::: widget/gtk/WindowSurfaceWayland.h:121
(Diff revision 1)
> +  bool                      mDelayedCommit;
> +  bool                      mFullScreenDamage;
> +  MessageLoop*              mWaylandMessageLoop;
> +  bool                      mIsMainThread;

fixed

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 15

•

6 years ago

mozreview-review

Comment on attachment 8934459 [details]
Bug 1422966 - Implement SurfaceWayland for Wayland,

https://reviewboard.mozilla.org/r/205394/#review213604

::: widget/gtk/WindowSurfaceWayland.h:120
(Diff revision 1)
> +  nsWindow*                 mWidget;
> +  nsWaylandDisplay*         mWaylandDisplay;
> +  WindowBackBuffer*         mFrontBuffer;
> +  WindowBackBuffer*         mBackBuffer;
> +  wl_callback*              mFrameCallback;
> +  wl_surface*               mFrameCallbackSurface;

Yes, this surface is used at mFrameCallback call only.

Comment hidden (mozreview-request)

Comment on attachment 8934456 [details]
Bug 1422966 - Update mDisplay attribute initialization at nsWaylandDisplay::nsWaylandDisplay(),

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205198/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8934457 [details]
Bug 1422966 - implement WaylandShmPool to manage shared memory for wl_buffer,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205200/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8934458 [details]
Bug 1422966 - implemented WindowBackBuffer to encapsulate wl_buffer,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205202/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8934459 [details]
Bug 1422966 - Implement SurfaceWayland for Wayland,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205394/diff/1-2/

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 20

•

6 years ago

mozreview-review

Comment on attachment 8934459 [details]
Bug 1422966 - Implement SurfaceWayland for Wayland,

https://reviewboard.mozilla.org/r/205394/#review213606

::: widget/gtk/WindowSurfaceWayland.h:115
(Diff revision 1)
> +  void                      FrameCallbackHandler();
> +
> +private:
> +  WindowBackBuffer*         GetBufferToDraw(int aWidth, int aHeight);
> +
> +  nsWindow*                 mWidget;

Fixed. I also add TODO for further investigation if we need to hold a reference to nsWindow object.

Comment hidden (mozreview-request)

Comment on attachment 8934459 [details]
Bug 1422966 - Implement SurfaceWayland for Wayland,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205394/diff/2-3/

Jan Horak [:jhorak]

Comment 22

•

6 years ago

mozreview-review

Comment on attachment 8934458 [details]
Bug 1422966 - implemented WindowBackBuffer to encapsulate wl_buffer,

https://reviewboard.mozilla.org/r/205202/#review213614

r+ with nits fixed.

::: widget/gtk/WindowSurfaceWayland.h:77
(Diff revision 2)
> +  bool IsAttached() { return mAttached; }
> +
> +  bool Resize(int aWidth, int aHeight);
> +  bool SetImageDataFromBackBuffer(class WindowBackBuffer* aSourceBuffer);
> +
> +  bool MatchSize(int aWidth, int aHeight)

Sorry I've overlooked it. This method name is confusing for me, as long as it is testing if sizes are equal, better us IsMatchingSize, IsSameSize or something like this.

::: widget/gtk/WindowSurfaceWayland.h:81
(Diff revision 2)
> +
> +  bool MatchSize(int aWidth, int aHeight)
> +  {
> +    return aWidth == mWidth && aHeight == mHeight;
> +  }
> +  bool MatchSize(class WindowBackBuffer *aBuffer)

see above

Attachment #8934458 - Flags: review?(jhorak) → review+

Jan Horak [:jhorak]

Comment 23

•

6 years ago

mozreview-review

Comment on attachment 8934459 [details]
Bug 1422966 - Implement SurfaceWayland for Wayland,

https://reviewboard.mozilla.org/r/205394/#review213616

Attachment #8934459 - Flags: review?(jhorak) → review+

Comment hidden (mozreview-request)

Comment on attachment 8934459 [details]
Bug 1422966 - Implement SurfaceWayland for Wayland,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205394/diff/3-4/

Pulsebot

Comment 25

•

6 years ago

Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/3cf5876e6d1c
Comment Wayland rendering scheme, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/8870736e152c
Update mDisplay attribute initialization at nsWaylandDisplay::nsWaylandDisplay(), r=jhorak
https://hg.mozilla.org/integration/autoland/rev/9e7204579027
implement WaylandShmPool to manage shared memory for wl_buffer, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/4a82861a1399
implemented WindowBackBuffer to encapsulate wl_buffer, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/68803e96b4be
Implement SurfaceWayland for Wayland, r=jhorak

Natalia Csoregi [:nataliaCs]

Comment 26

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/3cf5876e6d1c
https://hg.mozilla.org/mozilla-central/rev/8870736e152c
https://hg.mozilla.org/mozilla-central/rev/9e7204579027
https://hg.mozilla.org/mozilla-central/rev/4a82861a1399
https://hg.mozilla.org/mozilla-central/rev/68803e96b4be

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox59: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla59

Martin Stránský [:stransky] (ni? me)

Assignee

Updated

•

6 years ago

Blocks: 1425820

Jan Beich

Updated

•

3 years ago

Regressions: 1618914

You need to log in before you can comment on or make changes to this bug.