Mir

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

Proposed by Daniel van Vugt
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 1948
Proposed branch: lp://qastaging/~vanvugt/mir/simplify-alpha
Merge into: lp://qastaging/mir
Diff against target: 257 lines (+8/-41)
14 files modified
debian/changelog (+1/-0)
include/platform/mir/graphics/renderable.h (+0/-1)
platform-ABI-sha1sums (+1/-1)
playground/render_overlays.cpp (+0/-5)
server-ABI-sha1sums (+1/-1)
src/platform/graphics/android/hwc_device.cpp (+1/-1)
src/platform/graphics/android/hwc_fallback_gl_renderer.cpp (+1/-1)
src/platform/graphics/android/hwc_layerlist.cpp (+2/-2)
src/platform/symbols.map (+0/-1)
src/server/input/touchspot_controller.cpp (+0/-5)
src/server/scene/basic_surface.cpp (+0/-8)
tests/include/mir_test_doubles/fake_renderable.h (+0/-5)
tests/include/mir_test_doubles/mock_renderable.h (+0/-1)
tests/include/mir_test_doubles/stub_renderable.h (+1/-9)
To merge this branch: bzr merge lp://qastaging/~vanvugt/mir/simplify-alpha
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve
Kevin DuBois (community) Approve
Alberto Aguirre (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+236075@code.qastaging.launchpad.net

Commit message

Simplify alpha-related attributes, removing the recently added
alpha_enabled(). It adds no functionality that's not already
available via alpha() or shaped(). So drop it.

As a bonus, this also fixes LP: #1373698.

Description of the change

The two ABI breaks have already been bumped in the current 0.8 series. No further bump required.

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I just noticed I had accidentally fixed too much in hwc_device.cpp, which stopped nested servers from overlaying. Now un-fixed hwc_device.cpp so it at least has the same old bug as before ("FIXME"), and overlays will still work :S

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

Needs discussion:
I left my thoughts on the Renderable interface here:
https://code.launchpad.net/~kdub/mir/fix-1373698/+merge/236023/comments/578069

Needs fixing:
95 + if (renderable->shaped() || renderable->alpha() < 1.0f)
108 + renderable->shaped() || renderable->alpha() < 1.0f,
We don't support planeAlpha blending in HWC yet, so we don't need the check of alpha() here just yet. The check in renderable_list_is_hwc_incompatible prevents these lines from being reached if plane alpha is present.

80 + if (// renderable->shaped() || // Unsafe to uncomment yet LP: #1374358
HWC can do alpha blending, so the commented out line should be removed.

62 -bool plane_alpha_is_translucent(mg::Renderable const& renderable)
Lets not remove this. I don't think the simpler check causes any 'real' problems, but since we're translating a float to an 8bit, anything greater than (1-2^9) should translate into 0xFF for the alpha value.

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

Fixed.

And in case anyone's wondering, the regression test for LP: #1373698 already exists in test_gl_renderer.cpp where we mock shaped() and check for calls to glEnable(GL_BLEND).

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Looks good.

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

okay, looks good now. I guess I'll resubmit the other branch with the name cleanup, and this branch will remove of one of the alpha functions and the bug.

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

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