Mir

Code review comment for lp:~alan-griffiths/mir/fix-1535894

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

> 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.

« Back to merge proposal