D26171: Implement wp_viewporter
davidedmundson accepted this revision. This revision is now accepted and ready to land. REPOSITORY R127 KWayland BRANCH viewporter REVISION DETAIL https://phabricator.kde.org/D26171 To: romangg, #kwin, davidedmundson Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26171: Implement wp_viewporter
romangg updated this revision to Diff 72576. romangg added a comment. - Cleanup REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26171?vs=72575=72576 BRANCH viewporter REVISION DETAIL https://phabricator.kde.org/D26171 AFFECTED FILES autotests/client/CMakeLists.txt autotests/client/test_viewporter.cpp src/client/CMakeLists.txt src/client/registry.cpp src/client/registry.h src/client/viewporter.cpp src/client/viewporter.h src/server/CMakeLists.txt src/server/display.cpp src/server/display.h src/server/surface_interface.cpp src/server/surface_interface.h src/server/surface_interface_p.h src/server/viewporter_interface.cpp src/server/viewporter_interface.h To: romangg, #kwin Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26171: Implement wp_viewporter
romangg updated this revision to Diff 72575. romangg marked 6 inline comments as done. romangg added a comment. - Check source rectangle on related changes, buffer rename. REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26171?vs=72029=72575 BRANCH viewporter REVISION DETAIL https://phabricator.kde.org/D26171 AFFECTED FILES autotests/client/CMakeLists.txt autotests/client/test_viewporter.cpp src/client/CMakeLists.txt src/client/registry.cpp src/client/registry.h src/client/viewporter.cpp src/client/viewporter.h src/server/CMakeLists.txt src/server/display.cpp src/server/display.h src/server/surface_interface.cpp src/server/surface_interface.h src/server/surface_interface_p.h src/server/viewporter_interface.cpp src/server/viewporter_interface.h To: romangg, #kwin Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26171: Implement wp_viewporter
davidedmundson added inline comments. INLINE COMMENTS > romangg wrote in surface_interface.cpp:882 > How I read the spec: the source rectangle is always specified in coordinates > after scale and transform. [1] > > `SurfaceInterface::size()` returns the scaled and transformed (not yet) > buffer size so since viewporter coordinates are to this base we can just > directly return them. > > [1] > https://gitlab.freedesktop.org/wayland/wayland-protocols/blob/master/stable/viewporter/viewporter.xml#L103 Yeah, on re-reading I think your intepretation is right. Leave your stuff as-is. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D26171 To: romangg, #kwin Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26171: Implement wp_viewporter
romangg added inline comments. INLINE COMMENTS > davidedmundson wrote in surface_interface.cpp:485 > We're not testing this error in the case of: > > buffer is set > viewporter.sourceRect is set > > then later > buffer changes > viewporter.sourceRect remains the same > > this should be an error too. Good point. Same should be done for when the transform or scale changes. > davidedmundson wrote in surface_interface.cpp:882 > Should this be > > d->current.sourceRectangle.size()/ scale() > > It would be a somewhat weird case to use both viewporter and wl_buffer.scale, > but it's what the spec implies and you seem to be assuming that in the kwin > render path. > > destinationSize would be as-is. How I read the spec: the source rectangle is always specified in coordinates after scale and transform. [1] `SurfaceInterface::size()` returns the scaled and transformed (not yet) buffer size so since viewporter coordinates are to this base we can just directly return them. [1] https://gitlab.freedesktop.org/wayland/wayland-protocols/blob/master/stable/viewporter/viewporter.xml#L103 > davidedmundson wrote in surface_interface.h:130 > Why not just > > BufferInterface *buffer() const > > There's no reason why a caller would need to explicitly pick a const vs > non-const version. Yea, I added the new function for API/ABI-compatibility. But if you say making the function `buffer()` const is not a problem in this regard I will gladly do that. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D26171 To: romangg, #kwin Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26171: Implement wp_viewporter
davidedmundson added a comment.
Cool, I like viewporter.
INLINE COMMENTS
> surface_interface.cpp:464
> +
> +sizeChanged |= (bool)buffer;
> +}
C-style casts are frowned upon.
> surface_interface.cpp:485
> +if (!QRectF(QPointF(),
> bufferSize).contains(source->sourceRectangle)) {
> +wl_resource_post_error(viewport->parentResource(),
> + WP_VIEWPORT_ERROR_OUT_OF_BUFFER,
We're not testing this error in the case of:
buffer is set
viewporter.sourceRect is set
then later
buffer changes
viewporter.sourceRect remains the same
this should be an error too.
> surface_interface.cpp:882
> +if (d->current.sourceRectangle.isValid()) {
> +return d->current.sourceRectangle.size().toSize();
> +}
Should this be
d->current.sourceRectangle.size()/ scale()
It would be a somewhat weird case to use both viewporter and wl_buffer.scale,
but it's what the spec implies and you seem to be assuming that in the kwin
render path.
destinationSize would be as-is.
> romangg wrote in surface_interface.h:130
> Needed for a call in a const function in KWin. Do we want to do it like that?
> Other suggestions?
Why not just
BufferInterface *buffer() const
There's no reason why a caller would need to explicitly pick a const vs
non-const version.
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D26171
To: romangg, #kwin
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham,
bruns
D26171: Implement wp_viewporter
romangg added a task: T4456: Implement viewporter protocol. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D26171 To: romangg, #kwin Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26171: Implement wp_viewporter
romangg added inline comments. INLINE COMMENTS > surface_interface.h:130 > + **/ > +BufferInterface *constBuffer() const; > QPoint offset() const; Needed for a call in a const function in KWin. Do we want to do it like that? Other suggestions? REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D26171 To: romangg, #kwin Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26171: Implement wp_viewporter
romangg added a dependent revision: D26172: Add wp_viewporter support. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D26171 To: romangg, #kwin Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26171: Implement wp_viewporter
romangg created this revision. romangg added a reviewer: KWin. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. romangg requested review of this revision. REVISION SUMMARY This patch adds interfaces for specifying viewports via the wp_viewporter protocol extension. This allows to make surface size and buffer independent from each other. For example a video player can send 1080p video data while the window of the player is of different size. The server interface ViewportInterface is directly integrated with SurfaceInterface. Viewport changes are double-buffered by that. TEST PLAN Added auto tests and with weston-scaler. REPOSITORY R127 KWayland BRANCH viewporter REVISION DETAIL https://phabricator.kde.org/D26171 AFFECTED FILES autotests/client/CMakeLists.txt autotests/client/test_viewporter.cpp src/client/CMakeLists.txt src/client/registry.cpp src/client/registry.h src/client/viewporter.cpp src/client/viewporter.h src/server/CMakeLists.txt src/server/display.cpp src/server/display.h src/server/surface_interface.cpp src/server/surface_interface.h src/server/surface_interface_p.h src/server/viewporter_interface.cpp src/server/viewporter_interface.h To: romangg, #kwin Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
