Merge lp:~alan-griffiths/unity-system-compositor/spinner-rework into lp:unity-system-compositor
| Status: | Merged |
|---|---|
| Approved by: | Alan Griffiths on 2015-06-01 |
| Approved revision: | 240 |
| Merged at revision: | 213 |
| Proposed branch: | lp:~alan-griffiths/unity-system-compositor/spinner-rework |
| Merge into: | lp:unity-system-compositor |
| Prerequisite: | lp:~mir-team/unity-system-compositor/respond-to-event-cleanup |
| Diff against target: |
1103 lines (+521/-368) 7 files modified
spinner/CMakeLists.txt (+1/-1) spinner/eglapp.cpp (+106/-231) spinner/eglapp.h (+4/-17) spinner/eglspinner.cpp (+146/-118) spinner/miregl.cpp (+198/-0) spinner/miregl.h (+64/-0) tests/integration-tests/test_dbus_event_loop.cpp (+2/-1) |
| To merge this branch: | bzr merge lp:~alan-griffiths/unity-system-compositor/spinner-rework |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| PS Jenkins bot | continuous-integration | Approve on 2015-05-28 | |
| Alexandros Frantzis (community) | Approve on 2015-05-28 | ||
| Kevin DuBois (community) | 2015-05-26 | Approve on 2015-05-26 | |
|
Review via email:
|
|||
Commit Message
Rework of spinner app to avoid deprecated Mir APIs, correctly set fullscreen state and support multiple outputs.
Description of the Change
Rework of spinner app to avoid deprecated Mir APIs, correctly set fullscreen state and support multiple outputs.
| Kevin DuBois (kdub) wrote : | # |
Looks like a good translation of the existing code, but some things could use fixing (esp the resource cleanup)
887+ eglMakeCurrent(
Unless theres a reason to keep the context current, the line should be:
eglMakeCurrent(
pre-existing:
862+ eglctx = eglCreateContex
874+ auto const eglsurface = eglCreateWindow
resources should be destroyed with eglDestroySurface and eglDestroyContext
847+ if (!eglInitialize
would be good to check that the version is 1.4 (major == 1) && (minor == 4)
nits: (although, maybe not nits for the USC style guide, not sure where to find that)
515+const char vShaderSrcSpinner[] =
516+ "attribute vec4 vPosition; \n"
526+const char fShaderSrcGlow[] =
527+ "precision mediump float; \n"
spacing
651,652,656-660: c-casts, (although not sure if thats a usc policy)
648+ for (auto const& surface : surfaces)
braces for multiline statement following
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:236
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Alan Griffiths (alan-griffiths) wrote : | # |
[ RUN ] ADBusEventLoop.
/tmp/buildd/
Value of: delay
Expected: is >= 8-byte object <64-00 00-00 00-00 00-00>
Actual: 8-byte object <8F-52 F1-05 00-00 00-00>
[ FAILED ] ADBusEventLoop.
Not code touched by this MP (and can't reproduce locally).
But two failures in a row can't be an accident. Adding a little debug code in the hope of enlightenment.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:237
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Alan Griffiths (alan-griffiths) wrote : | # |
[ RUN ] ADBusEventLoop.
DEBUG delay=99878419 in ms=99, lbound=100
/tmp/buildd/
Value of: delay
Expected: is >= 8-byte object <64-00 00-00 00-00 00-00>
Actual: 8-byte object <13-06 F4-05 00-00 00-00>
[ FAILED ] ADBusEventLoop.
Ok, the measured delay is just under the timeout. Which could be because the timeout is enqueued (and therefore could initiate) before noting the start time.
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:238
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Alexandros Frantzis (afrantzis) wrote : | # |
Not a full review yet:
> Ok, the measured delay is just under the timeout. Which could be because the timeout is
> enqueued (and therefore could initiate) before noting the start time.
This is correct. Thanks for the fix.
> nits: (although, maybe not nits for the USC style guide, not sure where to find that)
Let's just use the Mir style guide for USC (I think most of us have been doing that when updating USC anyway).
| Alexandros Frantzis (afrantzis) wrote : | # |
Looks good.
Nits:
167+ printf("Active output [%u] at (%d, %d) is %dx%d\n",168+ output-
for_each_
1094+ egl_release_
In general, releasing a current context is a pessimization, but not a big deal for the spinner app.
+//#define MID_AUBERGINE 0.368627451f, 0.152941176f, 0.31372549f498
+//#define ORANGE 0.866666667f, 0.282352941f, 0.141414141f499
...
We could just remove these. We can look up the ubuntu color scheme definitions again if we want to change colors.
712 glClearColor(BLACK, mir_eglapp_
Pre-existing: not needed. We set the clear color (in the main loop) before we invoke glClear().
| Alan Griffiths (alan-griffiths) wrote : | # |
I've left the colors in the comment as a cheap convenience for a future maintainer.
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:240
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://

FAILED: Continuous integration, rev:232 jenkins. qa.ubuntu. com/job/ unity-system- compositor- ci/215/ jenkins. qa.ubuntu. com/job/ unity-system- compositor- wily-amd64- ci/3/console jenkins. qa.ubuntu. com/job/ unity-system- compositor- wily-armhf- ci/3 jenkins. qa.ubuntu. com/job/ unity-system- compositor- wily-armhf- ci/3/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ unity-system- compositor- wily-i386- ci/3
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/unity- system- compositor- ci/215/ rebuild
http://