Merge lp:~gerboland/qtmir/multimonitor into lp:qtmir
| Status: | Superseded |
|---|---|
| Proposed branch: | lp:~gerboland/qtmir/multimonitor |
| Merge into: | lp:qtmir |
| Diff against target: |
4409 lines (+2661/-540) 66 files modified
CMakeLists.txt (+1/-1) debian/control (+2/-2) demos/qml-demo-shell/ResizeArea.qml (+128/-0) demos/qml-demo-shell/Shell.qml (+19/-1) demos/qml-demo-shell/TitleBar.qml (+4/-1) demos/qml-demo-shell/Window.qml (+4/-81) demos/qml-demo-shell/qml-demo-shell.qml (+19/-0) src/modules/Unity/Application/CMakeLists.txt (+0/-1) src/modules/Unity/Application/mirbuffersgtexture.cpp (+5/-0) src/modules/Unity/Application/mirbuffersgtexture.h (+1/-0) src/modules/Unity/Application/mirsurface.cpp (+3/-1) src/modules/Unity/Application/plugin.cpp (+8/-1) src/modules/Unity/CMakeLists.txt (+1/-0) src/modules/Unity/Screens/CMakeLists.txt (+24/-0) src/modules/Unity/Screens/plugin.cpp (+41/-0) src/modules/Unity/Screens/qmldir (+2/-0) src/modules/Unity/Screens/screens.cpp (+107/-0) src/modules/Unity/Screens/screens.h (+82/-0) src/platforms/mirserver/CMakeLists.txt (+10/-3) src/platforms/mirserver/cursor.cpp (+152/-0) src/platforms/mirserver/cursor.h (+66/-0) src/platforms/mirserver/display.cpp (+0/-44) src/platforms/mirserver/display.h (+0/-37) src/platforms/mirserver/logging.h (+1/-0) src/platforms/mirserver/miropenglcontext.cpp (+25/-10) src/platforms/mirserver/miropenglcontext.h (+1/-0) src/platforms/mirserver/mirserver.cpp (+38/-4) src/platforms/mirserver/mirserver.h (+8/-2) src/platforms/mirserver/mirserverintegration.cpp (+51/-40) src/platforms/mirserver/mirserverintegration.h (+4/-7) src/platforms/mirserver/mirsingleton.cpp (+33/-0) src/platforms/mirserver/mirsingleton.h (+46/-0) src/platforms/mirserver/offscreensurface.cpp (+61/-0) src/platforms/mirserver/offscreensurface.h (+43/-0) src/platforms/mirserver/qmirserver.cpp (+12/-2) src/platforms/mirserver/qmirserver.h (+3/-0) src/platforms/mirserver/qmirserver_p.h (+2/-0) src/platforms/mirserver/qtcompositor.cpp (+9/-34) src/platforms/mirserver/qtcompositor.h (+13/-5) src/platforms/mirserver/qteventfeeder.cpp (+117/-90) src/platforms/mirserver/qteventfeeder.h (+13/-9) src/platforms/mirserver/screen.cpp (+103/-7) src/platforms/mirserver/screen.h (+36/-4) src/platforms/mirserver/screencontroller.cpp (+258/-0) src/platforms/mirserver/screencontroller.h (+96/-0) src/platforms/mirserver/screenwindow.cpp (+71/-80) src/platforms/mirserver/screenwindow.h (+13/-23) src/platforms/mirserver/tileddisplayconfigurationpolicy.cpp (+44/-0) src/platforms/mirserver/tileddisplayconfigurationpolicy.h (+35/-0) tests/common/fake_displayconfigurationoutput.h (+73/-0) tests/common/gmock_fixes.h (+124/-0) tests/common/mock_display.h (+53/-0) tests/common/mock_display_buffer.h (+43/-0) tests/common/mock_display_configuration.h (+35/-0) tests/common/mock_main_loop.h (+53/-0) tests/mirserver/CMakeLists.txt (+1/-0) tests/mirserver/QtEventFeeder/mock_qtwindowsystem.h (+16/-9) tests/mirserver/QtEventFeeder/qteventfeeder_test.cpp (+27/-16) tests/mirserver/Screen/CMakeLists.txt (+1/-0) tests/mirserver/Screen/screen_test.cpp (+38/-24) tests/mirserver/ScreenController/CMakeLists.txt (+28/-0) tests/mirserver/ScreenController/screencontroller_test.cpp (+189/-0) tests/mirserver/ScreenController/stub_display.h (+96/-0) tests/mirserver/ScreenController/stub_screen.h (+31/-0) tests/mirserver/ScreenController/testable_screencontroller.h (+37/-0) tests/modules/common/qtmir_test.h (+1/-1) |
| To merge this branch: | bzr merge lp:~gerboland/qtmir/multimonitor |
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Daniel d'Andrada (community) | 2015-09-02 | Approve on 2015-09-29 | |
| PS Jenkins bot | continuous-integration | 2015-09-02 | Approve on 2015-09-28 |
|
Review via email:
|
|||
This proposal supersedes a proposal from 2015-07-01.
This proposal has been superseded by a proposal from 2015-09-30.
Commit Message
Initial multimonitor support - react correctly to Mir DisplayConfigur
On Mir DisplayConfigur
1. blocks Mir until it has stopped all renderers and has their GL contexts released
2. reads the new DisplayConfigur
3. restarts all renderers
This also solves shutdown crash issues due to raciness of mir destroying the GL context backing the shell's QWindow before its renderer had stopped.
Add Unity.Screens qml module to advertise current screen state to QML.
Description of the Change
* Are there any related MPs required for this MP to build/function as expected? Please list.
N
* Did you perform an exploratory manual test run of your code change and any related functionality?
Y
* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A
| Gerry Boland (gerboland) wrote : | # |
| Gerry Boland (gerboland) wrote : | # |
Gah, input on second display not working
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:338
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
| Daniel d'Andrada (dandrader) wrote : | # |
src/modules/
"""
public Q_SLOTS:
void onScreenAdded(
void onScreenRemoved
"""
Shouldn't they be private?
-------
src/platforms/
"""
EGLDisplay m_eglDisplay;
EGLContext m_eglContext;
"""
You store them as member variables but they are still only used in the constructor.
-------
In MirServerIntegr
"""
qDebug() << "createPlatform
"""
A leftover.
"""
// If Screen was not specified, just grab an unused one, if available
"""
I don't get the first "if". This method doesn't seem to handle the case where a screen *is* specified. It doesn't even seem possible.
"""
if (!screens) {
qDebug() << "Screens are not initialized, unable to create a new QWindow/
return nullptr;
}
[...]
if (!screen) {
qDebug() << "No available Screens to create a new QWindow/
return nullptr;
}
"""
I think these should be turned into criticals (and also use the logging category API).
"""
qDebug() << "New" << window << "with geom" << window->geometry()
<< "is backed by a" << screen << "with geometry" << screen->geometry();
"""
You might want to keep this, but using the logging category API.
-------
In MirServerIntegr
"""
qDebug() << "ScreenController not initialized";
"""
More qDebug. Maybe this should be a qFatal?
-------
In src/platforms/
"""
#include <QDebug>
"""
You don't need that.
-------
In src/platforms/
"""
QWeakPointe
"""
Why return a weak pointer if all users of it do "screenControll
-------
QtEventFeeder:
I think it would be cleaner to have two constructors instead:
QtEventFeeder:
: QtEventFeeder(new QtWindowSystem(
{
}
QtEventFeeder:
{
- if (windowSystem) {
- mQtWindowSystem = windowSystem;
- } else {
- mQtWindowSystem = new QtWindowSystem;
- }
}
And remove the "= nullptr" from the signature of the second one.
-------
"""
void QtEventFeeder:
{
- auto type = mir_event_
- if (type != mir_event_
- return;
"""
Why remove it?
-------
"""
void QtEventFeeder:
{
[...]
+ mQtWindowSystem
}
"""
You have to pass the window chosen in QtEventFeeder:
-------
Please update...
| Daniel d'Andrada (dandrader) wrote : | # |
As for testing: When I connected my monitor to the micro hdmi port of my yoga 2 laptop, qtmir detected the new display and reacted accordingly. So I saw the unity logo on a green background in the external monitor.
But when I disconnected it, nothing happened on qtmir side. Connected back again and that was when I got the screen disconnected info in qtmir, it seems. Restarted the qml mirserver and now it wouldn't detect the external monitor at all.
Rebooted the laptop.
Run the mirserver again. Connected the external monitor. All fine. Disconnected and reconnected and boom. Laptop shuts itself down.
Despite the somewhat bad results, since this is just the first step in the multi-monitor story I'm still ok with having it merged.
| Gerry Boland (gerboland) wrote : | # |
I'll be addressing your code comments soon.
On your testing, please watch the console output carefully. ScreenControlle
Also, can you have the shell animating on both screens before you unplug?
Can you compare with mir_proving_server too?
I'm just trying to figure out if it is a QtMir bug, or a Mir one.
Thanks for the review!
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:354
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
| Gerry Boland (gerboland) wrote : | # |
I think I've followed all the recommendations you've made. I've only a couple of replies:
> """
> // If Screen was not specified, just grab an unused one, if available
> """
>
> I don't get the first "if". This method doesn't seem to handle the case where
> a screen *is* specified. It doesn't even seem possible.
You're right, it was terribly phrased. Have updated it to make sense now
> qDebug() << "Screens are not initialized, unable to create a new
> QWindow/
<snip>
> qDebug() << "No available Screens to create a new QWindow/
>
> I think these should be turned into criticals (and also use the logging
> category API).
I did make them critical, but I always want those errors to print so didn't make them part of a category.
> In src/platforms/
>
> """
> QWeakPointer<
> """
>
> Why return a weak pointer if all users of it do "screenControll
> So why not make it return a shared pointer already (making for cleaner code)?
Because the QMirServer has ownership of the ScreenController, and manages its lifetime. A ScreenController will only exist after the mir server is started, and must be deleted before mir shuts down. I can't have API users keeping it around longer than it should.
This may be exposing an API problem which can be resolved, but I didn't see any obvious solution. So this will have to do.
> src/platforms/
> """
> auto displayConfig = display-
> """
>
> I think you're abusing the use of "auto" here. I would prefer to know the
> class of this object.
You really want to know is it std::unique_
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:366
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
| Daniel d'Andrada (dandrader) wrote : | # |
On 06/08/15 08:49, Gerry Boland wrote:
>> qDebug() << "Screens are not initialized, unable to create a new
>> > QWindow/
> <snip>
>> > qDebug() << "No available Screens to create a new QWindow/
>> >
>> > I think these should be turned into criticals (and also use the logging
>> > category API).
> I did make them critical, but I always want those errors to print so didn't make them part of a category.
>
This doesn't stop you from using logging categories. You can configure
logging categories to print by severity. Eg: only from warnings and
upwards, or only criticals.
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:367
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
| Gerry Boland (gerboland) wrote : | # |
> On 06/08/15 08:49, Gerry Boland wrote:
> > I did make them critical, but I always want those errors to print so didn't
> make them part of a category.
> >
>
> This doesn't stop you from using logging categories. You can configure
> logging categories to print by severity. Eg: only from warnings and
> upwards, or only criticals.
I know that. But IMO this error should always print if that codepath hit, as it's an error path which I see no reason to be filter-able.
| Daniel d'Andrada (dandrader) wrote : | # |
"""
qtmir (0.4.6) vivid; urgency=medium
"""
nitpick: Shouldn't it be UNRELEASED instead of vivid?
| Daniel d'Andrada (dandrader) wrote : | # |
Good clean up! Only a couple of minor issues left:
-------
Please update copyright year of src/platforms/
-------
src/platforms/
It still has some qDebug() messages and commented-out code.
"""
if (window && window->window()) { qDebug() << "HIDE" << window;
"""
"""
"""
NB: the stuff above appears in two separate locations in the file
-------
In tests/mirserver
"""
ASSERT_
"""
Google test format for assertions is as follows: ASSERT_EQ(expected, actual)
But in this file you're doing the other way around, which will make for confusing messages in case of failure.
Same goes for EXPECT_EQ() and friends.
| Daniel d'Andrada (dandrader) wrote : | # |
Ah, and the qml-demo-shell changes, naturally. You probably gonna rebase this branch on top of lp:~dandrader/qtmir/mousePointer or lp:~dandrader/qtmir/mirSurface, right?
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:371
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
| Gerry Boland (gerboland) wrote : | # |
Ways to test:
1. on N4/N7, install the packages and reboot. Plug in external display with slimport cable - you should see part of unity8 on the external display. (unity8 crashed before)
2. on N4/N7/desktop, stop lightdm, and run the qml-demo-shell demo at root (QT_QPA_
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:373
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:376
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:377
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Daniel d'Andrada (dandrader) wrote : | # |
Code looks ok.
Tested on a N7 with unity8 and it works. Connecting an external monitor shows the cloned image there (albeit cut off) and disconnecting it doesn't crash unity8.
qml-demo-shell on the N7 freezes. Don't know if it's related to this MP.
Also could not test on my laptop as its flimsy micro-HDMI port seems to be broken, or preferably, my "HDMI to micro-HDMI" adaptor. :(
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:378
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Gerry Boland (gerboland) wrote : | # |
Testing this as a nested server, I have found
https:/
which needs the Mir team to investigate.
Which means this is unreliable to test as a nested server while adding/removing monitors and trying to draw on them all.
So the only things you can reliably test are
1. phone/tablet - testing a nested QtMir server (i.e. unity8) does not crash on plug/unplug of external monitor
2. desktop - running QtMir as host server - here plug/unplug of monitor will succeed.
| Gerry Boland (gerboland) wrote : | # |
Have resolved the issue I reported in the Mir bug above. I was using an API I wasn't supposed to. Now nested servers work reliably.
So in testing, the only situation where QtMir will not work reliably is:
- android, host server
mainly because of the buffer post thing.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:379
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Michael Terry (mterry) wrote : | # |
tests/mirserver
| Michael Terry (mterry) wrote : | # |
Using just this branch on top of rc-proposed, I don't see any changes when I plug in an external monitor. Nothing crashes, but nothing changes either. And I don't see any obvious screen-list dumps in the log, like you mentioned I might see.
So I'm guessing I need another branch?
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:380
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 381. By Gerry Boland on 2015-09-14
-
Merge trunk
- 382. By Gerry Boland on 2015-09-14
-
OpenGlContext - remove attempt at clean doneCurrent call, it just breaks things
- 383. By Gerry Boland on 2015-09-14
-
Disable any GL or Scenegraph persistence, ensures all GL resources are released on display config change
- 384. By Gerry Boland on 2015-09-14
-
Add reassuring debug message
- 385. By Gerry Boland on 2015-09-14
-
On scenegraph re-creation, was possible for MirBufferSGTexture to be used in the renderer without a texture being assigned. Prevent that
- 386. By Gerry Boland on 2015-09-14
-
Fix test fail
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:386
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
| Gerry Boland (gerboland) wrote : | # |
I declare this is ready for a final review pass. There are issues remaining, which appear to be USC/Mir-based:
https:/
But I'd prefer to land this and then work on those after.
| Gerry Boland (gerboland) wrote : | # |
Ways to test:
1. desktop - as root user:
systemctl stop lightdm
export QT_QPA_
qmlscene qml-demo-shell.qml
and then plug/unplug external monitor. You should see the simple shell appear on all screens.
2. phone, with unity8 up
plug/unplug monitor, verify shell does not crash.
Depending on USC's mood, you might see unity8 on the external display, or you might not. Touch input unreliable, due to USC we think.
| Daniel d'Andrada (dandrader) wrote : | # |
Code still looks ok and tests pass again.
Proceeding with manual tests now...
| Daniel d'Andrada (dandrader) wrote : | # |
> 2. phone, with unity8 up
> plug/unplug monitor, verify shell does not crash.
> Depending on USC's mood, you might see unity8 on the external display, or you
> might not. Touch input unreliable, due to USC we think.
Tried that on my Nexus 7.
When I plug in the external monitor, the UI on the device flashes for a split second. Then I unplug the monitor (I think it flashes again, not sure now). But then it doesn't to input anymore and eventually crashes. :(
- 387. By Gerry Boland on 2015-09-28
-
Merge trunk
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:387
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Gerry Boland (gerboland) wrote : | # |
> > 2. phone, with unity8 up
> > plug/unplug monitor, verify shell does not crash.
> > Depending on USC's mood, you might see unity8 on the external display, or
> you
> > might not. Touch input unreliable, due to USC we think.
>
> Tried that on my Nexus 7.
> When I plug in the external monitor, the UI on the device flashes for a split
> second. Then I unplug the monitor (I think it flashes again, not sure now).
> But then it doesn't to input anymore and eventually crashes. :(
Something to watch carefully is if unity8 crashes alone, or USC crashes which brings unity8 down with it. There are some input handling bugs in Mir/USC with multimonitor which are things we need the Mir team to fix.
| Gerry Boland (gerboland) wrote : | # |
https:/
| Daniel d'Andrada (dandrader) wrote : | # |
Tested with a fresh image (no multimonitor branch).
Plugging in an external monitor makes unity8 restart, and so does unplugging it.
Therefore this branch doesn't make matters worse. It just makes it crash in a different way.
| Daniel d'Andrada (dandrader) wrote : | # |
But it still feels odd to land a branch that doesn't get us anywhere... As it still crashes, I can't be confident that it's indeed doing the right thing...
| Daniel d'Andrada (dandrader) wrote : | # |
Running unity8 + qtmir/multimonitor on a wily laptop (tried with both mir 0.16 and mir 0.17), once I connect an external monitor it crashes.
Should it work? or is the crash expected?
| Gerry Boland (gerboland) wrote : | # |
No crashes are expected. I've just tested on Wily, and I can't reproduce a crash.
| Daniel d'Andrada (dandrader) wrote : | # |
Not getting a crash anymore... Don't know what happened.
Since mousePointer is gonna land before this branch, I've it rebased on top of mousePointer here: lp:~dandrader/qtmir/mousePointer-multimonitor. Should make your rebase painless.
- 388. By Gerry Boland on 2015-09-30
-
Merge Daniel's kind work lp:~dandrader/qtmir/mousePointer-multimonitor
- 389. By Gerry Boland on 2015-10-14
-
Merge mousePointer again
- 390. By Gerry Boland on 2015-10-14
-
Fix conflicts but have ScreenController test failing
- 391. By Gerry Boland on 2015-10-14
-
Fix failing test, RenderTarget not derived from NativeDisplayBuffer

The QML demo is pretty lame, but I'm holding on until your demo work in the mirSurface is approved.
Also there are FIXMEs related to input, which I would prefer to address later. This MR is big enough.