Closed Bug 1422966 Opened 2 years ago Closed 2 years ago

[Wayland] - Implement WindowSurfaceWayland

Categories

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

All
Linux
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

We need to implement WindowSurface for Wayland.
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
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.
Comment on attachment 8934455 [details]
Bug 1422966 - Comment Wayland rendering scheme,

https://reviewboard.mozilla.org/r/205196/#review213392
Attachment #8934455 - Flags: review?(jhorak) → 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+
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+
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-
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.
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-
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
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 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 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+
Comment on attachment 8934459 [details]
Bug 1422966 - Implement SurfaceWayland for Wayland,

https://reviewboard.mozilla.org/r/205394/#review213616
Attachment #8934459 - Flags: review?(jhorak) → review+
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
Blocks: 1425820
You need to log in before you can comment on or make changes to this bug.