Mir

Merge lp://qastaging/~vanvugt/mir/unwanted-alpha into lp://qastaging/mir

Proposed by Daniel van Vugt
Status: Work in progress
Proposed branch: lp://qastaging/~vanvugt/mir/unwanted-alpha
Merge into: lp://qastaging/mir
Diff against target: 509 lines (+168/-38)
26 files modified
debian/control (+4/-4)
debian/mir-client-platform-android4.install (+1/-1)
debian/mir-client-platform-mesa4.install (+1/-1)
examples/eglapp.c (+2/-1)
examples/scroll.cpp (+2/-1)
include/client/mir_toolkit/mir_connection.h (+6/-1)
src/client/mir_connection.cpp (+3/-2)
src/client/mir_connection.h (+2/-1)
src/client/mir_connection_api.cpp (+14/-2)
src/client/symbols.map (+7/-1)
src/include/client/mir/client_platform.h (+2/-1)
src/platforms/CMakeLists.txt (+1/-1)
src/platforms/android/client/android_client_platform.cpp (+1/-1)
src/platforms/android/client/android_client_platform.h (+2/-1)
src/platforms/android/client/symbols.map (+1/-1)
src/platforms/mesa/client/client_platform.cpp (+18/-2)
src/platforms/mesa/client/client_platform.h (+2/-1)
src/platforms/mesa/client/symbols.map (+1/-1)
tests/acceptance-tests/test_client_library_errors.cpp (+2/-1)
tests/mir_test_framework/stub_client_platform_factory.cpp (+2/-1)
tests/mir_test_framework/symbols-client.map (+1/-1)
tests/unit-tests/client/android/test_android_client_platform.cpp (+8/-4)
tests/unit-tests/client/mesa/test_client_platform.cpp (+79/-3)
tests/unit-tests/client/test_client_buffer_stream.cpp (+2/-1)
tests/unit-tests/client/test_client_mir_surface.cpp (+2/-1)
tests/unit-tests/client/test_mir_connection.cpp (+2/-2)
To merge this branch: bzr merge lp://qastaging/~vanvugt/mir/unwanted-alpha
Reviewer Review Type Date Requested Status
Daniel van Vugt Needs Information
Kevin DuBois (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Alan Griffiths Abstain
Review via email: mp+266694@code.qastaging.launchpad.net

Commit message

Extend mir_connection_get_egl_pixel_format to take additional hints
from the client's requested attribute list. This way the existence
of an alpha channel (chosen by the driver but unwanted by the client)
doesn't force Mir into a slower compositing mode (LP: #1480755).

Description of the change

I could have just hacked examples/eglapp.c and declared the bug fixed, but that would not suffice for other toolkits (particularly games). So this more long-winded solution solves the problem for everyone.

I also investigated the possibility that our dumb demo code was simply not finding the best EGLConfig by virtue of always truncating the list of configs to length one. But even if you look at the full list, the problem is present for every available EGLConfig (EGL_ALPHA_SIZE==8 even when I didn't ask for it).

Annoyingly the first version of mir_connection_get_egl_pixel_format
is actually returning the right result (the pixel format the driver
is using) so there's nothing immediately obvious to fix. But the right
pixel format is not necessarily the fastest one. If we can see the
client hasn't asked for an alpha channel then we should disable the
alpha channel. This way composite bypass can still be used on platforms
that always return an alpha channel (e.g. Mesa on Haswell always returns EGL_ALPHA_SIZE==8).

I'm fairly sure we only want to fix this on Mesa where the bug was
found. On Android we have a greater chance of supporting translucent
overlays in future so don't want to (and don't need to) use as big a
hammer (see TODOs in hwc_layerlist.cpp).

To post a comment you must log in.
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

395 +TEST_F(MesaClientPlatformTest, takes_opacity_hint_from_attribs)

This test checks for the correct result in several different scenarios (multiple unrelated EXPECT_EQs). I'd prefer a test for each of them.

review: Abstain
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

+1 to separating out the test into its different expectations, but lgtm otherwise

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Chris suggested today that we should just fix Mesa instead. If Mesa returned RGBA of 8880 instead of 8888 when we are asking for 8880, then there would not be a bug to fix.

review: Needs Information

Unmerged revisions

2809. By Daniel van Vugt

Bump the client platform ABI, since it's now broken.

2808. By Daniel van Vugt

Restore ABI backward-compatibility

2807. By Daniel van Vugt

Extend the tests further

2806. By Daniel van Vugt

Enable the fix. Tests now pass.

2805. By Daniel van Vugt

Fix the test. Copy and paste stupidity.

2804. By Daniel van Vugt

Disable the fix and write some regression tests.

2803. By Daniel van Vugt

First working prototype

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches