QtMir

Merge lp:~unity-team/qtmir/surfaceItemFillMode into lp:qtmir

  1. surfaceItemFillMode
  2. Merge into trunk
Proposed by Michał Sawicz on 2015-12-07
Status: Merged
Approved by: Michael Zanetti on 2015-12-09
Approved revision: 425
Merged at revision: 425
Proposed branch: lp:~unity-team/qtmir/surfaceItemFillMode
Merge into: lp:qtmir
Prerequisite: lp:~nick-dedekind/qtmir/polite-close
Diff against target: 137 lines (+42/-7)
5 files modified
CMakeLists.txt (+1/-1)
debian/control (+2/-2)
src/modules/Unity/Application/mirsurface.cpp (+1/-1)
src/modules/Unity/Application/mirsurfaceitem.cpp (+33/-3)
src/modules/Unity/Application/mirsurfaceitem.h (+5/-0)
To merge this branch: bzr merge lp:~unity-team/qtmir/surfaceItemFillMode
Reviewer Review Type Date Requested Status
Michael Zanetti (community) 2015-12-07 Approve on 2015-12-09
PS Jenkins bot continuous-integration 2015-12-07 Needs Fixing on 2015-12-07
Review via email: mp+279757@code.launchpad.net

This proposal supersedes a proposal from 2015-12-04.

Commit Message

Add MirSurfaceItem.fillMode and ensure items and buffer are in sync

Ensure that by the time we enter the phase of updating the scene graph, all qml items are up to date regarding the size of the buffer about to be rendered.

NB: This rendering scheme needs triple buffering to work.

Description of the Change

* Are there any related MPs required for this MP to build/function as expected? Please list.

https://code.launchpad.net/~dandrader/unity-api/surfaceItemFillMode2/+merge/278955
https://code.launchpad.net/~dandrader/unity8/surfaceItemFillMode/+merge/279010

* Did you perform an exploratory manual test run of your code change and any related functionality?

Yes

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

Not applicable

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Glad to have helped. This isn't the first time someone has told me I'm wrong and then they went ahead and copied my code :)

Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

What's the use-case of this fillMode? The crop ability might have use in the spread? But it's not a proper solution for the stretched frames issue on rotation.

Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

On 18/09/15 08:02, Gerry Boland wrote:
> What's the use-case of this fillMode? The crop ability might have use in the spread? But it's not a proper solution for the stretched frames issue on rotation.

Resize untiy8-dash in desktop mode on a Nexus device and you'll see.

Michael Zanetti (mzanetti) wrote : Posted in a previous version of this proposal

Code looks ok, but as noticed on the related unity8 branch, I'm not sure if this is the proper place to calculate the innerRect stuff. It has the effect that it makes window content and window decoration/shadow go out of sync.

review: Needs Information
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

On 23/09/2015 07:32, Michael Zanetti wrote:
> Review: Needs Information
>
> Code looks ok, but as noticed on the related unity8 branch, I'm not sure if this is the proper place to calculate the innerRect stuff. It has the effect that it makes window content and window decoration/shadow go out of sync.

They're two separate things. fillMode is there to ensure that you never
see a stretched surface even if the MirSurfaceItem size doesn't match
it. See it as yet another MirSurface feature that shells (unity8) can
take and use it on their UI/interaction designs.

What unity8 *does with it* is another thing. That shadow "going out of
sync" is a crude implementation or that unity7 window resize mode where
the window is not resize live, but you drag a translucent orang bounding
rect of it and only on release does the window get committed to that
bounding rect size (or actually a hint for the possibility doing the same).

PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Michael Zanetti (mzanetti) wrote : Posted in a previous version of this proposal

I've tested this and it works fine. Would prefer Gerry to have a look at the code as there is quite some buffer handling involved which I'm not too experienced with.

review: Approve (functional testing)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

Remind me why we want to support Stretch?

Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

> Remind me why we want to support Stretch?

Stretch does sound like a bad name, but scaling is stretching while respecting the aspect ratio of the surface.

And we want scaling working to display window thumbnails in the alt-tab spread, for instance.

Loïc Molinari (loic.molinari) wrote : Posted in a previous version of this proposal

Using polish() sounds like the right thing to do in order to correctly sync surface size and item size, no more temporary frame drawn with surface size different than item size. Emitting the item sizeChanged signal in the GUI thread right before the updatePaintNode() callback in the render thread doesn't imply 2 frames anymore when Unity requires a surface resize. It should also fix that [1].

Regarding the fill mode, I think we should expose the same enum names that the standard QtQuick Image uses with, for now, just the needed ones : Stretch and Pad. That would make things cleaner IMO, easier to understand for people used to that and would allow to add the PreserveAspectFit and PreserveAspectCrop (and maybe the alignment props) later if ever needed.

(Take my comments with a grain of salt because I'm not completely aware of the whole picture, but I'm taking the liberty of commenting on that because I've been working a bit on qtmir lately for the mutlibuffer stream thing and for the AA optimization which kinda touches the same code path ;) )

[1] https://code.launchpad.net/~dandrader/qtmir/revertRev415/+merge/278604

PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Michael Zanetti (mzanetti) :
review: Approve
lp:~unity-team/qtmir/surfaceItemFillMode updated on 2015-12-10
426. By Michał Sawicz on 2015-12-10

Revert changes related to surface/texture handling as they caused black apps from time to time

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2015-11-25 15:38:50 +0000
3+++ CMakeLists.txt 2015-12-10 12:54:20 +0000
4@@ -75,7 +75,7 @@
5 pkg_check_modules(GSETTINGS_QT REQUIRED gsettings-qt)
6 pkg_check_modules(QTDBUSTEST libqtdbustest-1 REQUIRED)
7 pkg_check_modules(QTDBUSMOCK libqtdbusmock-1 REQUIRED)
8-pkg_check_modules(APPLICATION_API REQUIRED unity-shell-application=11)
9+pkg_check_modules(APPLICATION_API REQUIRED unity-shell-application=12)
10
11 include_directories(${APPLICATION_API_INCLUDE_DIRS})
12
13
14=== modified file 'debian/control'
15--- debian/control 2015-11-25 15:38:50 +0000
16+++ debian/control 2015-12-10 12:54:20 +0000
17@@ -22,7 +22,7 @@
18 libubuntu-app-launch2-dev,
19 libubuntu-application-api-dev (>= 2.1.0),
20 libudev-dev,
21- libunity-api-dev (>= 7.103),
22+ libunity-api-dev (>= 7.104),
23 liburl-dispatcher1-dev,
24 libxkbcommon-dev,
25 libxrender-dev,
26@@ -93,7 +93,7 @@
27 Conflicts: libqtmir,
28 libunity-mir1,
29 Provides: unity-application-impl,
30- unity-application-impl-11,
31+ unity-application-impl-12,
32 Description: Qt plugin for Unity specific Mir APIs
33 QtMir provides Qt/QML bindings for Mir features that are exposed through the
34 qtmir-desktop or qtmir-android QPA plugin such as Application management
35
36=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
37--- src/modules/Unity/Application/mirsurface.cpp 2015-12-10 12:54:19 +0000
38+++ src/modules/Unity/Application/mirsurface.cpp 2015-12-10 12:54:20 +0000
39@@ -297,7 +297,7 @@
40 int framesPending = m_surface->buffers_ready_for_compositor(userId);
41 if (framesPending > 0) {
42 m_textureUpdated = false;
43-
44+
45 locker.unlock();
46 if (updateTexture()) {
47 qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::dropPendingBuffer() dropped=1 left=" << framesPending-1;
48
49=== modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
50--- src/modules/Unity/Application/mirsurfaceitem.cpp 2015-11-25 15:38:54 +0000
51+++ src/modules/Unity/Application/mirsurfaceitem.cpp 2015-12-10 12:54:20 +0000
52@@ -92,6 +92,7 @@
53 , m_surfaceHeight(0)
54 , m_orientationAngle(nullptr)
55 , m_consumesInput(false)
56+ , m_fillMode(Stretch)
57 {
58 qCDebug(QTMIR_SURFACES) << "MirSurfaceItem::MirSurfaceItem";
59
60@@ -240,15 +241,31 @@
61 node->setMipmapFiltering(QSGTexture::None);
62 node->setHorizontalWrapMode(QSGTexture::ClampToEdge);
63 node->setVerticalWrapMode(QSGTexture::ClampToEdge);
64- node->setSubSourceRect(QRectF(0, 0, 1, 1));
65 } else {
66 if (!m_lastFrameNumberRendered || (*m_lastFrameNumberRendered != m_surface->currentFrameNumber())) {
67 node->markDirty(QSGNode::DirtyMaterial);
68 }
69 }
70
71- node->setTargetRect(QRectF(0, 0, width(), height()));
72- node->setInnerTargetRect(QRectF(0, 0, width(), height()));
73+ if (m_fillMode == PadOrCrop) {
74+ const QSize &textureSize = m_textureProvider->texture()->textureSize();
75+
76+ QRectF targetRect;
77+ targetRect.setWidth(qMin(width(), static_cast<qreal>(textureSize.width())));
78+ targetRect.setHeight(qMin(height(), static_cast<qreal>(textureSize.height())));
79+
80+ qreal u = targetRect.width() / textureSize.width();
81+ qreal v = targetRect.height() / textureSize.height();
82+ node->setSubSourceRect(QRectF(0, 0, u, v));
83+
84+ node->setTargetRect(targetRect);
85+ node->setInnerTargetRect(targetRect);
86+ } else {
87+ // Stretch
88+ node->setSubSourceRect(QRectF(0, 0, 1, 1));
89+ node->setTargetRect(QRectF(0, 0, width(), height()));
90+ node->setInnerTargetRect(QRectF(0, 0, width(), height()));
91+ }
92
93 node->setFiltering(smooth() ? QSGTexture::Linear : QSGTexture::Nearest);
94 node->setAntialiasing(antialiasing());
95@@ -730,6 +747,19 @@
96 }
97 }
98
99+MirSurfaceItem::FillMode MirSurfaceItem::fillMode() const
100+{
101+ return m_fillMode;
102+}
103+
104+void MirSurfaceItem::setFillMode(FillMode value)
105+{
106+ if (m_fillMode != value) {
107+ m_fillMode = value;
108+ Q_EMIT fillModeChanged(m_fillMode);
109+ }
110+}
111+
112 } // namespace qtmir
113
114 #include "mirsurfaceitem.moc"
115
116=== modified file 'src/modules/Unity/Application/mirsurfaceitem.h'
117--- src/modules/Unity/Application/mirsurfaceitem.h 2015-11-25 15:38:44 +0000
118+++ src/modules/Unity/Application/mirsurfaceitem.h 2015-12-10 12:54:20 +0000
119@@ -68,6 +68,9 @@
120 int surfaceHeight() const override;
121 void setSurfaceHeight(int value) override;
122
123+ FillMode fillMode() const override;
124+ void setFillMode(FillMode value) override;
125+
126 ////////
127 // QQuickItem
128
129@@ -168,6 +171,8 @@
130 Mir::OrientationAngle *m_orientationAngle;
131
132 bool m_consumesInput;
133+
134+ FillMode m_fillMode;
135 };
136
137 } // namespace qtmir

Subscribers

People subscribed via source and target branches