qtvideo-node

Merge lp:~fboucault/qtvideo-node/fix_rotate_z_order into lp:qtvideo-node

  1. fix_rotate_z_order
  2. Merge into trunk
Proposed by Florian Boucault on 2015-03-09
Status: Merged
Approved by: Florian Boucault on 2015-03-12
Approved revision: 62
Merged at revision: 57
Proposed branch: lp:~fboucault/qtvideo-node/fix_rotate_z_order
Merge into: lp:qtvideo-node
Diff against target: 172 lines (+126/-1)
6 files modified
debian/control (+4/-0)
src/shadervideomaterial.cpp (+22/-0)
unittests/tst_videooutput/tst_videooutput.cpp (+19/-0)
unittests/tst_videooutput/tst_videooutput.pro (+10/-0)
unittests/tst_videooutput/tst_videooutput.qml (+69/-0)
unittests/unittests.pro (+2/-1)
To merge this branch: bzr merge lp:~fboucault/qtvideo-node/fix_rotate_z_order
Reviewer Review Type Date Requested Status
Bill Filler (community) Approve on 2015-03-10
Jim Hodapp code 2015-03-09 Approve on 2015-03-10
PS Jenkins bot continuous-integration Approve on 2015-03-09
Review via email: mp+252265@code.launchpad.net

Commit Message

Workaround issue where z-ordering of VideoOutput's QML item is incorrect when rotated.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Jim Hodapp (jhodapp) wrote :

Looks good, will test this soon but code looks good.

review: Approve (code)
Bill Filler (bfiller) wrote :

tested on krillin and mako, works well. Controls always appear

review: Approve
Jim Hodapp (jhodapp) wrote :

Tested, works well.

Bill Filler (bfiller) wrote :

we are going to need an AP test in camera-app to test this

Florian Boucault (fboucault) wrote :

I don't know of a way to programatically test this. I was thinking of
adding a manual test to the test plan.

On Wed, Mar 11, 2015 at 3:26 PM, Bill Filler <email address hidden>
wrote:

> we are going to need an AP test in camera-app to test this
> --
>
> https://code.launchpad.net/~fboucault/qtvideo-node/fix_rotate_z_order/+merge/252265
> You are the owner of lp:~fboucault/qtvideo-node/fix_rotate_z_order.
>

Jim Hodapp (jhodapp) wrote :

Don't you already have AP tests for the camera-app that test pressing the button and making sure that the button is present? Can't you adapt that code into make sure that it's still there during the cases where the buttons used to disappear?

Florian Boucault (fboucault) wrote :

None of the tests we have in the apps actually test if an Item is rendered,
they usually only test if the Item's visible property is set to true, which
is already the case.

On Wed, Mar 11, 2015 at 4:20 PM, Jim Hodapp <email address hidden>
wrote:

> Don't you already have AP tests for the camera-app that test pressing the
> button and making sure that the button is present? Can't you adapt that
> code into make sure that it's still there during the cases where the
> buttons used to disappear?
> --
>
> https://code.launchpad.net/~fboucault/qtvideo-node/fix_rotate_z_order/+merge/252265
> You are the owner of lp:~fboucault/qtvideo-node/fix_rotate_z_order.
>

Florian Boucault (fboucault) wrote :

I'll try doing a pixel per pixel comparison of a simple UI that contains a
VideoOutput and another Item: comparing how it should look like vs how it
is actually rendered.

On Wed, Mar 11, 2015 at 5:21 PM, Florian Boucault <
<email address hidden>> wrote:

> None of the tests we have in the apps actually test if an Item is rendered,
> they usually only test if the Item's visible property is set to true, which
> is already the case.
>
> On Wed, Mar 11, 2015 at 4:20 PM, Jim Hodapp <email address hidden>
> wrote:
>
> > Don't you already have AP tests for the camera-app that test pressing the
> > button and making sure that the button is present? Can't you adapt that
> > code into make sure that it's still there during the cases where the
> > buttons used to disappear?
> > --
> >
> >
> https://code.launchpad.net/~fboucault/qtvideo-node/fix_rotate_z_order/+merge/252265
> > You are the owner of lp:~fboucault/qtvideo-node/fix_rotate_z_order.
> >
>
> --
>
> https://code.launchpad.net/~fboucault/qtvideo-node/fix_rotate_z_order/+merge/252265
> You are the owner of lp:~fboucault/qtvideo-node/fix_rotate_z_order.
>

Florian Boucault (fboucault) wrote :

Done.

> I'll try doing a pixel per pixel comparison of a simple UI that contains a
> VideoOutput and another Item: comparing how it should look like vs how it
> is actually rendered.
>
> On Wed, Mar 11, 2015 at 5:21 PM, Florian Boucault <
> <email address hidden>> wrote:
>
> > None of the tests we have in the apps actually test if an Item is rendered,
> > they usually only test if the Item's visible property is set to true, which
> > is already the case.
> >
> > On Wed, Mar 11, 2015 at 4:20 PM, Jim Hodapp <email address hidden>
> > wrote:
> >
> > > Don't you already have AP tests for the camera-app that test pressing the
> > > button and making sure that the button is present? Can't you adapt that
> > > code into make sure that it's still there during the cases where the
> > > buttons used to disappear?
> > > --
> > >
> > >
> > https://code.launchpad.net/~fboucault/qtvideo-
> node/fix_rotate_z_order/+merge/252265
> > > You are the owner of lp:~fboucault/qtvideo-node/fix_rotate_z_order.
> > >
> >
> > --
> >
> > https://code.launchpad.net/~fboucault/qtvideo-
> node/fix_rotate_z_order/+merge/252265
> > You are the owner of lp:~fboucault/qtvideo-node/fix_rotate_z_order.
> >

lp:~fboucault/qtvideo-node/fix_rotate_z_order updated on 2015-03-16
63. By Florian Boucault on 2015-03-12

Try more things

64. By Florian Boucault on 2015-03-16

Disable tst_videooutput

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/control'
2--- debian/control 2014-03-27 16:12:37 +0000
3+++ debian/control 2015-03-16 12:11:50 +0000
4@@ -15,6 +15,10 @@
5 qtdeclarative5-dev,
6 qtdeclarative5-private-dev,
7 qtmultimedia5-dev,
8+ qml-module-qttest,
9+ qml-module-qtmultimedia,
10+ qml-module-qtquick2,
11+ xvfb,
12 Standards-Version: 3.9.4
13 Homepage: https://launchpad.net/qtvideo-node
14 # If you aren't a member of ~phablet-team but need to upload packaging changes,
15
16=== modified file 'src/shadervideomaterial.cpp'
17--- src/shadervideomaterial.cpp 2014-10-20 21:21:49 +0000
18+++ src/shadervideomaterial.cpp 2015-03-16 12:11:50 +0000
19@@ -45,6 +45,28 @@
20 m_readyToRender(false),
21 m_orientation(SharedSignal::Orientation::rotate0)
22 {
23+ /* FIXME: workaround incorrect z-ordering when VideoOutput is rotated.
24+ * In the following case VideoOutput is rendered on top of all other
25+ * Items in the scene but one. The issue is most likely related with
26+ * how QtQuick scenegraph's batch renderer draws the nodes.
27+ * When angle is 0, the ShaderVideoNode is marked as merged and there is
28+ * no issue. When set to a different value it is marked forever as unmerged
29+ * and the z-ordering becomes incorrect. Setting the CustomCompileStep flag
30+ * makes it marked as unmerged from the beginning and the z-ordering issue
31+ * never occurs.
32+ *
33+ * VideoOutput {
34+ * transform: [
35+ * Rotation {
36+ * axis.x: 0; axis.y: 1; axis.z: 0
37+ * angle: -20
38+ * }
39+ * ]
40+ * }
41+ *
42+ * Ref.: https://bugs.launchpad.net/camera-app/+bug/1373607
43+ */
44+ setFlag(CustomCompileStep, true);
45 connect(SharedSignal::instance(), SIGNAL(setOrientation(const SharedSignal::Orientation&, const QSize&)),
46 this, SLOT(onSetOrientation(const SharedSignal::Orientation&, const QSize&)));
47 }
48
49=== added directory 'unittests/tst_videooutput'
50=== added file 'unittests/tst_videooutput/small.mp4'
51Binary files unittests/tst_videooutput/small.mp4 1970-01-01 00:00:00 +0000 and unittests/tst_videooutput/small.mp4 2015-03-16 12:11:50 +0000 differ
52=== added file 'unittests/tst_videooutput/tst_videooutput.cpp'
53--- unittests/tst_videooutput/tst_videooutput.cpp 1970-01-01 00:00:00 +0000
54+++ unittests/tst_videooutput/tst_videooutput.cpp 2015-03-16 12:11:50 +0000
55@@ -0,0 +1,19 @@
56+/*
57+ * Copyright (C) 2015 Canonical, Ltd.
58+ *
59+ * This program is free software; you can redistribute it and/or modify
60+ * it under the terms of the GNU Lesser General Public License as published by
61+ * the Free Software Foundation; version 3.
62+ *
63+ * This program is distributed in the hope that it will be useful,
64+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
65+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
66+ * GNU Lesser General Public License for more details.
67+ *
68+ * You should have received a copy of the GNU Lesser General Public License
69+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
70+ */
71+
72+#include <QtQuickTest/quicktest.h>
73+QUICK_TEST_MAIN(videooutput)
74+
75
76=== added file 'unittests/tst_videooutput/tst_videooutput.pro'
77--- unittests/tst_videooutput/tst_videooutput.pro 1970-01-01 00:00:00 +0000
78+++ unittests/tst_videooutput/tst_videooutput.pro 2015-03-16 12:11:50 +0000
79@@ -0,0 +1,10 @@
80+TEMPLATE = app
81+TARGET = tst_videooutput
82+CONFIG += no_keywords warn_on qmltestcase
83+SOURCES += tst_videooutput.cpp
84+
85+OTHER_FILES += $$system(ls *.qml)
86+
87+check.commands = "xvfb-run -s '-screen 0 1024x768x24' -a ./$${TARGET}"
88+check.depends = $${TARGET}
89+QMAKE_EXTRA_TARGETS += check
90
91=== added file 'unittests/tst_videooutput/tst_videooutput.qml'
92--- unittests/tst_videooutput/tst_videooutput.qml 1970-01-01 00:00:00 +0000
93+++ unittests/tst_videooutput/tst_videooutput.qml 2015-03-16 12:11:50 +0000
94@@ -0,0 +1,69 @@
95+/*
96+ * Copyright 2015 Canonical Ltd.
97+ *
98+ * This program is free software; you can redistribute it and/or modify
99+ * it under the terms of the GNU Lesser General Public License as published by
100+ * the Free Software Foundation; version 3.
101+ *
102+ * This program is distributed in the hope that it will be useful,
103+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
104+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
105+ * GNU Lesser General Public License for more details.
106+ *
107+ * You should have received a copy of the GNU Lesser General Public License
108+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
109+ */
110+
111+import QtQuick 2.2
112+import QtMultimedia 5.0
113+import QtTest 1.1
114+
115+TestCase {
116+ id: root
117+ name: "VideoOutput"
118+ when: windowShown
119+ visible: true
120+ width: 400
121+ height: 400
122+
123+ function test_z_ordering_after_rotation() {
124+ player.play();
125+ player.seek(1000);
126+ rotation.angle = -2;
127+ waitForRendering(videoOutput);
128+ var image = grabImage(root);
129+ var actualColor = String(image.pixel(20, 20));
130+ var expectedColor = String(middleRectangle.color);
131+ compare(actualColor, expectedColor);
132+ }
133+
134+ VideoOutput {
135+ id: videoOutput
136+ anchors.fill: parent
137+ transform: [
138+ Rotation {
139+ id: rotation
140+ axis.x: 0; axis.y: 1; axis.z: 0
141+ angle: 0
142+ }
143+ ]
144+ fillMode: VideoOutput.Stretch
145+ source: MediaPlayer {
146+ id: player
147+ source: "small.mp4"
148+ }
149+ }
150+
151+ Rectangle {
152+ id: middleRectangle
153+ anchors.fill: parent
154+ color: "green"
155+ }
156+
157+ Rectangle {
158+ id: topRectangle
159+ anchors.fill: parent
160+ anchors.leftMargin: 50
161+ color: "red"
162+ }
163+}
164
165=== modified file 'unittests/unittests.pro'
166--- unittests/unittests.pro 2012-11-09 09:43:10 +0000
167+++ unittests/unittests.pro 2015-03-16 12:11:50 +0000
168@@ -1,2 +1,3 @@
169 TEMPLATE = subdirs
170-SUBDIRS += shadervideonode
171+SUBDIRS += shadervideonode \
172+# tst_videooutput # disabled as it fails on jenkins

Subscribers

People subscribed via source and target branches