Merge lp://qastaging/~alan-griffiths/mir/fix-1535894 into lp://qastaging/mir

FAILED: Continuous integration, rev:3295
https://mir-jenkins.ubuntu.com/job/mir-ci/230/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/230/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/230/rebuild

DEBUG: jenkins job parameter [use_description_for_commit]: not found
DEBUG: Unable to get "use_description_for_commit" configuration for mir-ci
DEBUG: Job is configured to fallback to description: False

/me dons SEP glasses

FAILED: Continuous integration, rev:3295
http://jenkins.qa.ubuntu.com/job/mir-ci/6206/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5803
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4710/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5759
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/530
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/530/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/530/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5756
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5756/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8166
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27319

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/6206/rebuild

> /me dons SEP glasses

I will check whether that message should worry us, but the real problem seems to be:

21: [ RUN ] MultiThreadedCompositor.schedules_enough_frames
21: /��BUILDDIR��/mir-0.20.0+xenial20bzr3295/tests/unit-tests/compositor/test_multi_threaded_compositor.cpp:608: Failure
21: Expected: (retry) < (max_retries), actual: 100 vs 100
21: [ FAILED ] MultiThreadedCompositor.schedules_enough_frames (1200 ms)

For example in: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-advanced/arch=i386,compiler=gcc,platform=mesa,release=xenial/20/consoleFull

FAILED: Continuous integration, rev:3296
https://mir-jenkins.ubuntu.com/job/mir-ci/233/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/233/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/233/rebuild

Not sure if this is related to changes in this MP:

https://mir-jenkins.ubuntu.com/job/build-2-binpkg-advanced/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/23/consoleFull

11: [ RUN ] AndroidInputReceiverSetup.slow_raw_input_doesnt_cause_frameskipping
11: /��BUILDDIR��/mir-0.20.0+vivid23bzr3296/tests/unit-tests/client/input/test_android_input_receiver.cpp:276: Failure
11: Expected: (duration) < (one_frame), actual: 8-byte object <05-B9 96-01 00-00 00-00> vs 8-byte object <2A-50 FE-00 00-00 00-00>
11: [ FAILED ] AndroidInputReceiverSetup.slow_raw_input_doesnt_cause_frameskipping (380 ms)

> Not sure if this is related to changes in this MP:
>
> https://mir-jenkins.ubuntu.com/job/build-2-binpkg-advanced/arch=i386,compiler=
> gcc,platform=android,release=vivid+overlay/23/consoleFull
>
> 11: [ RUN ]
> AndroidInputReceiverSetup.slow_raw_input_doesnt_cause_frameskipping
> 11: /��BUILDDIR��/mir-0.20.0+vivid23bzr3296/tests/unit-
> tests/client/input/test_android_input_receiver.cpp:276: Failure
> 11: Expected: (duration) < (one_frame), actual: 8-byte object <05-B9 96-01
> 00-00 00-00> vs 8-byte object <2A-50 FE-00 00-00 00-00>
> 11: [ FAILED ]
> AndroidInputReceiverSetup.slow_raw_input_doesnt_cause_frameskipping (380 ms)

I don't see how - AFAICS the test doesn't contain a MultiThreadedCompositor.

Ah. It's lp:1394369

PASSED: Continuous integration, rev:3296
http://jenkins.qa.ubuntu.com/job/mir-ci/6209/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5806
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4713
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5762
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/389
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/533
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/533/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/533
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/533/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5759
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5759/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8169
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27328
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/385
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/385/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/239
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27336

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/6209/rebuild

Like the fix that came before it, this sounds like a workaround rather than a fix.

You are addressing the multi-monitor performance problem by only attempting to redraw one monitor per frame. That sounds like an admission of failure and that we can't render multi-monitor at full frame rate on Android. So I'd prefer to see that problem fixed and full frame rate on both displays simultaneously. Although we might be hitting the physical performance limits of the device, in which case changes like this is all we'd have left. Not yet proven though, and that would be a bit pessimistic.

I actually don't think we're hitting the device performance limitation as much as trying to run sub-optimal code on it:
https://bugs.launchpad.net/ubuntu/+source/unity8/+bug/1494795/comments/4

Then again, if we don't intend to try and animate both screens at once then that's probably OK...

I'll stop short of approving though, because there's an additional problem with this branch that will break free-form rendering like Compiz or mir_proving_server -- Some pixels are painted outside of the surface itself (like shadows or other effects like minimizing animation). As such, your view area test will be wrong, and will fail to update the secondary screen when it needs updating.

Worse again, this change will fail completely under full screen transformations like desktop zoom. And again will fail to refresh displays correctly, appearing to freeze instead.

I actually disapprove, but will abstain because can see it will help pocket desktop as it stands right now.

Oh I forgot, we already have this feature... :)

void mc::DefaultDisplayBufferCompositor::composite(mc::SceneElementSequence&& scene_elements)
{
    report->began_frame(this);

    auto const& view_area = display_buffer.view_area();
    auto const& occlusions = mc::filter_occlusions_from(scene_elements, view_area);

Using this existing implementation would be preferable. It's just harder to modify now that we have display groups with the swap buffers call outside of the compositor. But not impossible. Plus doing the fix in the compositor would address my concerns for the future too.

All we need to do is return some result to the caller to tell them whether we have repainted (empty renderable_list), and so whether they should swap buffers.

Admittedly DefaultDisplayBufferCompositor is only used in USC and probably not in Unity8 any more. But we could do a similar fix in QtMir to address Unity8.

Fixing the problem in the compositor is ideal, because that's the only place where you might know about the additional rendering that's outside the expected confines of the surface rectangle.

It's not just other shells that don't exist yet which need this, but Unity8 needs it right now. Consider Unity8 on desktop with two monitors -- It does external rendering like minimize animations, shadows, and the whole launcher, that surface_stack.cpp doesn't know about and will get wrong using your proposed implementation.

> Fixing the problem in the compositor is ideal, because that's the only place
> where you might know about the additional rendering that's outside the
> expected confines of the surface rectangle.

I assume you mean "Fixing the problem in the DisplayBufferCompositor" (not the Compositor).

Shifting the logic from this MP and from -c 3274 to the DefaultDisplayBufferCompositor (with appropriate wiring) would address your concerns with both?

I recall that we had similar logic once in an earlier version, with the DisplayBufferCompositor returning a measure of uncomposited buffers. Can you recall why that was removed - we don't want to repeat the mistakes of the past.

> Admittedly DefaultDisplayBufferCompositor is only used in USC and probably not
> in Unity8 any more. But we could do a similar fix in QtMir to address Unity8.
...
> It's not just other shells that don't exist yet which need this, but Unity8
> needs it right now. Consider Unity8 on desktop with two monitors -- It does
> external rendering like minimize animations, shadows, and the whole launcher,
> that surface_stack.cpp doesn't know about and will get wrong using your
> proposed implementation.

This highlights a serious failing in our customization points.

Unity8 uses QtCompositor from QtMir, not MultiThreadedCompositor from Mir - and [Default]DisplayBufferCompositor is only used by the latter.

So the difference between this approach and your suggested alternative makes no difference to Unity8.

> Oh I forgot, we already have this feature... :)
>
> void mc::DefaultDisplayBufferCompositor::composite(mc::SceneElementSequence&&
> scene_elements)
> {
> report->began_frame(this);
>
> auto const& view_area = display_buffer.view_area();
> auto const& occlusions = mc::filter_occlusions_from(scene_elements,
> view_area);
>
> Using this existing implementation would be preferable. It's just harder to
> modify now that we have display groups with the swap buffers call outside of
> the compositor. But not impossible. Plus doing the fix in the compositor would
> address my concerns for the future too.
>
> All we need to do is return some result to the caller to tell them whether we
> have repainted (empty renderable_list), and so whether they should swap
> buffers.

Having thought about implementing this suggestion I don't think it works. (At least, not simply.)

The criteria for repainting is more complex that "!renderable_list.empty()". For example, when the last surface is removed from the view area the current proposal repaints, but, as I understand your suggestion it would not.

Another important case that isn't easily addressed by code in the context of DefaultDisplayBufferCompositor is when all the surfaces overlapping the view_area already have their latest buffer rendered. (This cannot be deduced from the SceneElement interface.)

fix looks alright to me, so +1

Curious as to why "void frames_pending(CompositorID, Rectangle)" was added, as opposed to improving "void frames_pending(CompositorID)" to take an additional parameter. Could also use a test.

> I recall that we had similar logic once in an earlier version, with the
> DisplayBufferCompositor returning a measure of uncomposited buffers. Can you
> recall why that was removed - we don't want to repeat the mistakes of the
> past.

Almost correct. What we had in an earlier version was a bool returned, which at the time indicated the DisplayBufferCompositor wanted to be scheduled for the following frame too. That's the Compiz way of doing things and it was removed (by kdub?) because it became dead code -- its first purpose of tracking pending buffers per surface was superseded, and its second purpose for compositor-based animations has not yet ever been implemented (I still want to though).

But that just highlights a bool isn't really enough, and is now confusing. The DisplayBufferCompositor might need to communicate more detailed information back to its caller than just a bool.

Yes indeed r3274 was flawed for the same reason and needs to be moved into DisplayBufferCompositor. Keeping in mind Unity8 doesn't use that and some equivalent enhancement is required to QtMir too.

Also, here's a manual test case showing this branch causes visible regressions:
  1. mir_proving_server --display-config sidebyside # needs 2 or more monitors
  2. mir_demo_client_egltriangle
  3. Move the window to the right half of the left monitor near the edge, but not overlapping.
  4. Zoom in with Super+mousewheel and move the mouse such that the zoomed window now appears on both monitors.
Using lp:mir - Triangle keeps animating smoothly.
Using this branch - Triangle freezes and stutters on the second monitor.

The same problem extends to regular viewing without any zoom. If you had a launcher notification animating on one screen, but no major window changes then the launcher animation would never be visible. Instead the screen with the launcher would appear to be frozen and idle, erroneously hiding the notification.

Someone might make the argument that in the Unity8 architecture this change only applies to system compositors and will work for Unity8. However that means we're now saying Mir multimonitor only works in a nested architectures where each system compositor surface fits one physical displays. That's not an acceptable limitation we should choose to introduce. Mir should still support non-nested configurations.