Mir

[regression] [nonblockswap] Client judders, skipping frames periodically

Bug #1308843 reported by Daniel van Vugt
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mir
Fix Released
High
Mir development team
Mir 0.3.0
mir (Ubuntu)
Fix Released
High
Unassigned

Bug Description

Running mir_demo_server_shell with mir_demo_client_egltriangle I see frame skips every 10 seconds or so, which is visible in the output too:

61 FPS
60 FPS
59 FPS
60 FPS
60 FPS
60 FPS
60 FPS
60 FPS
60 FPS
60 FPS
61 FPS
60 FPS
59 FPS
60 FPS
60 FPS
60 FPS
60 FPS
60 FPS
60 FPS
60 FPS
61 FPS

This only seems to happen when a single monitor is plugged in.

OLD DESCRIPTION:
I used to have an intel GPU (GM45 with SU9600) which would clock down the refresh rate to 40Hz for power savings. Although I don't have it any more, I can see we probably have broken that use case. Clients will probably skip frames now and look "unsmooth".

I think the simple solution is to ensure the new buffer consumer thread never runs faster than any real display. It should probably be slowed down to about 10Hz.

See original description

Related branches

lp:~vanvugt/mir/judder
On hold for merging into lp:mir
PS Jenkins bot (community): Needs Fixing (continuous-integration)
Alexandros Frantzis: Pending requested
Alan Griffiths: Pending requested
Mir development team: Pending requested
Diff: 297 lines (+166/-35)
3 files modified
src/server/compositor/multi_threaded_compositor.cpp (+29/-26)
tests/acceptance-tests/test_client_surface_swap_buffers.cpp (+3/-4)
tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+134/-5)
lp:~raof/mir/1hz-rendering-always
PS Jenkins bot (community): Approve (continuous-integration)
Alan Griffiths: Abstain
Daniel van Vugt: Abstain
Andreas Pokorny (community): Approve
Alexandros Frantzis (community): Abstain
Alberto Aguirre (community): Approve
Diff: 3043 lines (+1552/-378)
38 files modified
include/platform/mir/graphics/buffer_id.h (+1/-1)
include/server/mir/asio_main_loop.h (+2/-0)
include/server/mir/compositor/frame_dropping_policy.h (+69/-0)
include/server/mir/compositor/frame_dropping_policy_factory.h (+57/-0)
include/server/mir/default_server_configuration.h (+3/-0)
include/server/mir/time/timer.h (+8/-1)
include/test/mir_test/fake_clock.h (+72/-0)
include/test/mir_test/signal.h (+63/-0)
include/test/mir_test_doubles/mock_frame_dropping_policy_factory.h (+83/-0)
include/test/mir_test_doubles/mock_timer.h (+52/-0)
include/test/mir_test_doubles/stub_frame_dropping_policy_factory.h (+58/-0)
src/server/asio_main_loop.cpp (+23/-9)
src/server/compositor/CMakeLists.txt (+1/-0)
src/server/compositor/buffer_queue.cpp (+37/-1)
src/server/compositor/buffer_queue.h (+5/-1)
src/server/compositor/buffer_stream_factory.cpp (+6/-4)
src/server/compositor/buffer_stream_factory.h (+4/-4)
src/server/compositor/default_configuration.cpp (+15/-1)
src/server/compositor/multi_threaded_compositor.cpp (+53/-135)
src/server/compositor/timeout_frame_dropping_policy_factory.cpp (+92/-0)
src/server/compositor/timeout_frame_dropping_policy_factory.h (+56/-0)
tests/acceptance-tests/test_client_surface_swap_buffers.cpp (+8/-13)
tests/integration-tests/compositor/test_buffer_stream.cpp (+45/-15)
tests/integration-tests/compositor/test_swapping_swappers.cpp (+3/-1)
tests/integration-tests/graphics/android/test_buffer_integration.cpp (+4/-1)
tests/integration-tests/graphics/android/test_internal_client.cpp (+2/-1)
tests/mir_test/CMakeLists.txt (+2/-0)
tests/mir_test/fake_clock.cpp (+53/-0)
tests/mir_test/signal.cpp (+46/-0)
tests/mir_test_doubles/CMakeLists.txt (+2/-0)
tests/mir_test_doubles/mock_frame_dropping_policy_factory.cpp (+70/-0)
tests/mir_test_doubles/mock_timer.cpp (+149/-0)
tests/unit-tests/compositor/CMakeLists.txt (+1/-0)
tests/unit-tests/compositor/test_buffer_queue.cpp (+128/-46)
tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+15/-78)
tests/unit-tests/compositor/test_timeout_frame_dropping_policy.cpp (+186/-0)
tests/unit-tests/graphics/mesa/test_linux_virtual_terminal.cpp (+1/-0)
tests/unit-tests/test_asio_main_loop.cpp (+77/-66)
lp:~afrantzis/mir/consume-only-not-rendered-buffers
Rejected for merging into lp:mir
PS Jenkins bot (community): Approve (continuous-integration)
Daniel van Vugt: Needs Fixing
Kevin DuBois (community): Needs Fixing
Alan Griffiths: Needs Information
Diff: 365 lines (+82/-31)
11 files modified
examples/render_overlays.cpp (+11/-1)
include/platform/mir/graphics/renderable.h (+2/-0)
include/test/mir_test_doubles/fake_renderable.h (+10/-1)
include/test/mir_test_doubles/mock_renderable.h (+1/-0)
include/test/mir_test_doubles/stub_renderable.h (+13/-0)
src/server/compositor/multi_threaded_compositor.cpp (+23/-27)
src/server/scene/basic_surface.cpp (+8/-0)
src/server/scene/basic_surface.h (+3/-0)
src/server/scene/surface_stack.cpp (+3/-0)
tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+2/-2)
tests/unit-tests/graphics/android/test_hwc_device.cpp (+6/-0)
lp:~mir-team/mir/10Hz-rendering-always
PS Jenkins bot (community): Approve (continuous-integration)
Daniel van Vugt: Disapprove
Chris Halse Rogers: Approve
Kevin DuBois (community): Approve
Alexandros Frantzis (community): Approve
Diff: 2890 lines (+1520/-333)
38 files modified
include/platform/mir/graphics/buffer_id.h (+1/-1)
include/server/mir/asio_main_loop.h (+2/-0)
include/server/mir/compositor/frame_dropping_policy.h (+69/-0)
include/server/mir/compositor/frame_dropping_policy_factory.h (+57/-0)
include/server/mir/default_server_configuration.h (+3/-0)
include/server/mir/time/timer.h (+8/-1)
include/test/mir_test/fake_clock.h (+72/-0)
include/test/mir_test/signal.h (+63/-0)
include/test/mir_test_doubles/mock_frame_dropping_policy_factory.h (+83/-0)
include/test/mir_test_doubles/mock_timer.h (+52/-0)
include/test/mir_test_doubles/stub_frame_dropping_policy_factory.h (+58/-0)
src/server/asio_main_loop.cpp (+20/-6)
src/server/compositor/CMakeLists.txt (+1/-0)
src/server/compositor/buffer_queue.cpp (+54/-3)
src/server/compositor/buffer_queue.h (+6/-1)
src/server/compositor/buffer_stream_factory.cpp (+6/-4)
src/server/compositor/buffer_stream_factory.h (+4/-4)
src/server/compositor/default_configuration.cpp (+15/-1)
src/server/compositor/multi_threaded_compositor.cpp (+53/-135)
src/server/compositor/timeout_frame_dropping_policy_factory.cpp (+92/-0)
src/server/compositor/timeout_frame_dropping_policy_factory.h (+56/-0)
tests/acceptance-tests/test_client_surface_swap_buffers.cpp (+8/-13)
tests/integration-tests/compositor/test_buffer_stream.cpp (+44/-14)
tests/integration-tests/compositor/test_swapping_swappers.cpp (+3/-1)
tests/integration-tests/graphics/android/test_buffer_integration.cpp (+4/-1)
tests/integration-tests/graphics/android/test_internal_client.cpp (+2/-1)
tests/mir_test/CMakeLists.txt (+2/-0)
tests/mir_test/fake_clock.cpp (+53/-0)
tests/mir_test/signal.cpp (+46/-0)
tests/mir_test_doubles/CMakeLists.txt (+2/-0)
tests/mir_test_doubles/mock_frame_dropping_policy_factory.cpp (+70/-0)
tests/mir_test_doubles/mock_timer.cpp (+149/-0)
tests/unit-tests/compositor/CMakeLists.txt (+1/-0)
tests/unit-tests/compositor/test_buffer_queue.cpp (+130/-47)
tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+15/-78)
tests/unit-tests/compositor/test_timeout_frame_dropping_policy.cpp (+186/-0)
tests/unit-tests/graphics/mesa/test_linux_virtual_terminal.cpp (+1/-0)
tests/unit-tests/test_asio_main_loop.cpp (+29/-22)
lp:ubuntu/utopic-proposed/mir
Daniel van Vugt (vanvugt)
tags: added: regression-update
removed: regression-uodate
Changed in mir:
milestone: none → 0.1.9
Daniel van Vugt (vanvugt)
summary: - [regression] Clients skip frames on 40Hz displays
+ [regression] Clients skip frames periodically
description: updated
Changed in mir:
importance: Medium → High
status: New → In Progress
assignee: nobody → Daniel van Vugt (vanvugt)
summary: - [regression] Clients skip frames periodically
+ [regression] Client judders, skipping frames periodically
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Re: [regression] Client judders, skipping frames periodically

Bisected. The problem appeared in r1545:

------------------------------------------------------------
revno: 1545 [merge]
author: Alexandros Frantzis <email address hidden>
committer: Tarmac
branch nick: development-branch
timestamp: Thu 2014-04-10 20:17:20 +0000
message:
  compositor: Consume buffers of surfaces that are not rendered on screen

  This ensures that eglSwapBuffers() in clients doesn't block if a surface is not
  rendered. Fixes: https://bugs.launchpad.net/bugs/1233564, https://bugs.launchpad.net/bugs/1292306.

  Approved by Alberto Aguirre, PS Jenkins bot.
------------------------------------------------------------

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

It appears my default refresh rate is actually closer to 59.95Hz but Mir's internal logic rounds up to 60 so that information is hidden. That would explain why the physical display is beating/juddering, because it's not 60Hz like the fake consumer is.

kevin gunn (kgunn72)
summary: - [regression] Client judders, skipping frames periodically
+ [regression] [nonblockswap] Client judders, skipping frames periodically
tags: added: nonblockswap
Revision history for this message
kevin gunn (kgunn72) wrote :

As discussed in the stand-up today.
There's no ill effect seen from a end user point of view on the phone, tablet or xmir.
Based on that, we're accepting this bug as part of the 019 milestone in order to deliver the overall improvement of unblocking events on the phone/tablet.

As for addressing this bug, the team seems to be in agreement on using Chris' proposed solution as an improvement (solving in switching bundle) and a way to correct this issue.
https://code.launchpad.net/~raof/mir/1hz-rendering-always/+merge/216246

kevin gunn (kgunn72)
Changed in mir:
milestone: 0.1.9 → 0.1.10
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Just to clarify, living with this regression is probably OK for displays of 60Hz, close to 60Hz or greater. In that case you'll only see periodic glitches like I do every few seconds.

However, displays of significantly less than 60Hz may be dramatically affected, glitching several times per second. I haven't set up the hardware to prove this, but expect it could be unbearable for say 50Hz modes. So we do need to fix this ASAP. Visual quality is a key feature of Mir we don't want to go backwards very often.

Daniel van Vugt (vanvugt)
Changed in mir:
assignee: Daniel van Vugt (vanvugt) → Mir development team (mir-team)
Daniel van Vugt (vanvugt)
tags: added: regression
removed: regression-update
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

Fix committed into lp:mir/devel at revision None, scheduled for release in mir, milestone Unknown

Changed in mir:
status: In Progress → Fix Committed
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Fix reverted in r1653 due to more serious regressions.

Changed in mir:
status: Fix Committed → In Progress
Cemil Azizoglu (cemil-azizoglu)
Changed in mir:
milestone: 0.2.0 → 0.3.0
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

Fix committed into lp:mir/devel at revision None, scheduled for release in mir, milestone Unknown

Changed in mir:
status: In Progress → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package mir - 0.3.0+14.10.20140618.1-0ubuntu1

---------------
mir (0.3.0+14.10.20140618.1-0ubuntu1) utopic; urgency=medium

  [ Cemil Azizoglu ]
  * New upstream release 0.3.0 (https://launchpad.net/mir/+milestone/0.3.0)
    - mirclient ABI unchanged, still at 7. Clients do not need rebuilding.
    - mirserver ABI bumped to 21. Shells need rebuilding.
      . Divide swap_client_buffers into separate functions.
      . Allow buffer swapping even when compositor is turned off or blocked.
        (LP: #1308843, 1308844)
      . Wire the cursor client API through the session mediator to the surface
        observer.
      . Generate destination alpha for alpha enabled display buffers.
      . Named threads.
      . Support for prompt sessions :
        https://wiki.ubuntu.com/Security/TrustStoreAndSessions.

    - mirscreencast screenshots now produce images with correct color instead
      of transparency.
    - Rename bind_to_texture->gl_bind_to_texture.
    - Refinements in test infrastructure.
    - Phablet: allow disabling the overlays via a command line switch.
    - Unify the DisplayBuffer interface's optimization functions
      behind post_renderables_if_optimizable.
    - Clean up the DisplayDevice interface so that it doesn't require the
      functions are called in any particular order.
    - Phablet: graphics: android: preserve buffer ownership for onscreen
      overlay layers until the subsequent display posting.
    - Allow the platform to register emergency cleanup handlers in order to
      restore the graphics system to a sane state when the server fails
      abruptly.

    - Bugs fixed:
       . Mirscreencast outputs translucency instead of shadows, producing
         incorrect images. (LP: #1301210)
       . JSON formatting. (LP: #1324902)
       . Fix input_area_contains to work properly in global coordinates even
         when input_rectangles is updated. (LP: #1261647)
       . Allow a moment for clients to acquire a buffer. (LP: #1317370)
       . Generate correct alpha by changing blending equation to assume
         pre-multiplied alpha sources. (LP: #1318852)
       . Rework the recomposition messaging so that the DisplayBufferCompositor
         is no longer involved. (LP: #1319907)
  [ Ubuntu daily release ]
  * New rebuild forced
 -- Ubuntu daily release <email address hidden> Wed, 18 Jun 2014 17:51:53 +0000

Changed in mir (Ubuntu):
status: New → Fix Released
kevin gunn (kgunn72)
Changed in mir:
status: Fix Committed → Fix Released
Daniel van Vugt (vanvugt)
Changed in mir (Ubuntu):
importance: Undecided → High
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.