Merge lp:~albaguirre/mir/fix-1359406 into lp:mir

[Alberto Aguirre]

android: do not post driver cancelled buffers (LP: #1359406)

android: do not post driver cancelled buffers (LP: #1359406)

This is the least obtrusive change that I could think of since the rest of the stack does not support the concept of returning a buffer without posting it.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1852
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2452/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1403
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1409/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1386/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/974/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/323/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2452/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Seems OK.

This may trigger spurious ABI breaks: mir_native_window.h
However my analysis as recently as last night tells me that header file is not used either directly or indirectly by external projects. So not really an ABI break.

review: Approve

[Kevin DuBois (kdub)]

Needs Info:
A bit of a different memory, but I tried something similar when I was spiking n10 support a long time ago, and remember some problem popping up. What devices were tested?

The 'proper' fix in my estimation involves modifying the protocol to support multiple client buffers or to indicate a 'nonrendered buffer'.

review: Needs Information

[Kevin DuBois (kdub)]

> A bit of a different memory, but I tried something similar when I was spiking

A bit of a _distant_ memory

[Alberto Aguirre (albaguirre)]

@Kevin,

I tested n4, n10, n7 and krillin.

Note only the devices with mali drivers call cancelBuffer (n10, krillin) and consequently the ones that were showed blank frames when an app is opening up (so one sees a flash of whatever content was behind - usually the dash).

Yes the ideal proper fix is for the stack to understand the concept of clients wanting to drop buffers completely without them being composited/displayed but I'm trying to keep the public ABI from breaking.

[Kevin DuBois (kdub)]

okay, lgtm given the avoid-abi-break constraint. Having the android platform handle cancel buffers will be an easier task once the next_buffer rpc call is deprecated.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1853
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2496/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1450
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1456/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1435/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1018/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/372/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2496/rebuild

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks sensible.

review: Approve

[Alan Griffiths (alan-griffiths)]

Not tested across devices, but looks sane.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1854
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2513/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1473
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1479
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1458/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1035
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1035/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/395
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/395/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2544/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12150

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2513/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1855
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2515/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1476
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1482
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1461
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1037/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/398
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/398/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2546
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12168

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2515/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1856
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2516/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1478
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1484
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1462
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1038
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1038/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/399
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/399/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2547
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12175

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2516/rebuild

review: Approve (continuous-integration)

=== modified file 'common-ABI-sha1sums'
--- common-ABI-sha1sums	2014-08-14 05:44:09 +0000
+++ common-ABI-sha1sums	2014-08-26 22:48:11 +0000
@@ -14,7 +14,7 @@
 76d576231de039e25f4cf30829504552dcb9fc53  include/shared/mir/graphics/android/android_driver_interpreter.h
 c02a1e68d5a8ccfd8710440e166c9e32bc974f1e  include/shared/mir/graphics/android/android_native_buffer.h
 05e7b043ccf49582a78e880967b3bc1f3512e010  include/shared/mir/graphics/android/fence.h
-9956c408e05c5785143c491f868d8be5c17ea40c  include/shared/mir/graphics/android/mir_native_window.h
+0168b68dea12355c22cd39fbfc50bca0e21fd6e5  include/shared/mir/graphics/android/mir_native_window.h
 8f6d9dc681f3e0d212e6a74f9170ea7b3eb2607f  include/shared/mir/graphics/android/native_buffer.h
 424d1ff5b9bc299fc43f0afe961c4dafc3227992  include/shared/mir/graphics/android/sync_fence.h
 e1be9faee8b844ca2ce617f8fd82c9ee08d56bed  include/shared/mir/graphics/native_buffer.h

=== modified file 'include/shared/mir/graphics/android/mir_native_window.h'
--- include/shared/mir/graphics/android/mir_native_window.h	2014-03-06 06:05:17 +0000
+++ include/shared/mir/graphics/android/mir_native_window.h	2014-08-26 22:48:11 +0000
@@ -22,6 +22,7 @@
 #include <system/window.h>
 #include <cstdarg>
 #include <memory>
+#include <vector>
 
 namespace mir
 {
@@ -30,6 +31,7 @@
 namespace android
 {
 class AndroidDriverInterpreter;
+class SyncFileOps;
 
 class MirNativeWindow : public ANativeWindow
 {
@@ -44,8 +46,9 @@
     int cancelBuffer(struct ANativeWindowBuffer* buffer, int fence);
     int setSwapInterval(int interval);
 private:
-
     std::shared_ptr<AndroidDriverInterpreter> const driver_interpreter;
+    std::shared_ptr<SyncFileOps> const sync_ops;
+    std::vector<struct ANativeWindowBuffer*> cancelled_buffers;
 };
 
 }

=== modified file 'platform-ABI-sha1sums'
--- platform-ABI-sha1sums	2014-08-20 11:58:29 +0000
+++ platform-ABI-sha1sums	2014-08-26 22:48:11 +0000
@@ -58,7 +58,7 @@
 76d576231de039e25f4cf30829504552dcb9fc53  include/shared/mir/graphics/android/android_driver_interpreter.h
 c02a1e68d5a8ccfd8710440e166c9e32bc974f1e  include/shared/mir/graphics/android/android_native_buffer.h
 05e7b043ccf49582a78e880967b3bc1f3512e010  include/shared/mir/graphics/android/fence.h
-9956c408e05c5785143c491f868d8be5c17ea40c  include/shared/mir/graphics/android/mir_native_window.h
+0168b68dea12355c22cd39fbfc50bca0e21fd6e5  include/shared/mir/graphics/android/mir_native_window.h
 8f6d9dc681f3e0d212e6a74f9170ea7b3eb2607f  include/shared/mir/graphics/android/native_buffer.h
 424d1ff5b9bc299fc43f0afe961c4dafc3227992  include/shared/mir/graphics/android/sync_fence.h
 e1be9faee8b844ca2ce617f8fd82c9ee08d56bed  include/shared/mir/graphics/native_buffer.h

=== modified file 'server-ABI-sha1sums'
--- server-ABI-sha1sums	2014-08-14 20:14:12 +0000
+++ server-ABI-sha1sums	2014-08-26 22:48:11 +0000
@@ -167,7 +167,7 @@
 76d576231de039e25f4cf30829504552dcb9fc53  include/shared/mir/graphics/android/android_driver_interpreter.h
 c02a1e68d5a8ccfd8710440e166c9e32bc974f1e  include/shared/mir/graphics/android/android_native_buffer.h
 05e7b043ccf49582a78e880967b3bc1f3512e010  include/shared/mir/graphics/android/fence.h
-9956c408e05c5785143c491f868d8be5c17ea40c  include/shared/mir/graphics/android/mir_native_window.h
+0168b68dea12355c22cd39fbfc50bca0e21fd6e5  include/shared/mir/graphics/android/mir_native_window.h
 8f6d9dc681f3e0d212e6a74f9170ea7b3eb2607f  include/shared/mir/graphics/android/native_buffer.h
 424d1ff5b9bc299fc43f0afe961c4dafc3227992  include/shared/mir/graphics/android/sync_fence.h
 e1be9faee8b844ca2ce617f8fd82c9ee08d56bed  include/shared/mir/graphics/native_buffer.h

=== modified file 'src/shared/graphics/android/mir_native_window.cpp'
--- src/shared/graphics/android/mir_native_window.cpp	2014-08-05 22:36:37 +0000
+++ src/shared/graphics/android/mir_native_window.cpp	2014-08-26 22:48:11 +0000
@@ -132,7 +132,7 @@
 }
 
 mga::MirNativeWindow::MirNativeWindow(std::shared_ptr<AndroidDriverInterpreter> const& interpreter)
- : driver_interpreter(interpreter)
+ : driver_interpreter(interpreter), sync_ops(std::make_shared<mga::RealSyncFileOps>())
 {
     ANativeWindow::query = &query_static;
     ANativeWindow::perform = &perform_static;
@@ -168,11 +168,20 @@
 int mga::MirNativeWindow::dequeueBuffer(struct ANativeWindowBuffer** buffer_to_driver, int* fence_fd)
 try
 {
-    auto buffer = driver_interpreter->driver_requests_buffer();
+    if (cancelled_buffers.size() != 0)
+    {
+        *buffer_to_driver = cancelled_buffers.back();
+        cancelled_buffers.pop_back();
+        *fence_fd = -1; //no fence associated with cancelled buffers
+    }
+    else
+    {
+        auto buffer = driver_interpreter->driver_requests_buffer();
 
-    //EGL driver is responsible for closing this native handle
-    *fence_fd = buffer->copy_fence();
-    *buffer_to_driver = buffer->anwb();
+        //EGL driver is responsible for closing this native handle
+        *fence_fd = buffer->copy_fence();
+        *buffer_to_driver = buffer->anwb();
+    }
     return 0;
 }
 catch (std::exception const& e)
@@ -184,9 +193,17 @@
 int mga::MirNativeWindow::dequeueBufferAndWait(struct ANativeWindowBuffer** buffer_to_driver)
 try
 {
-    auto buffer = driver_interpreter->driver_requests_buffer();
-    *buffer_to_driver = buffer->anwb();
-    buffer->ensure_available_for(mga::BufferAccess::write);
+    if (cancelled_buffers.size() != 0)
+    {
+        *buffer_to_driver = cancelled_buffers.back();
+        cancelled_buffers.pop_back();
+    }
+    else
+    {
+        auto buffer = driver_interpreter->driver_requests_buffer();
+        *buffer_to_driver = buffer->anwb();
+        buffer->ensure_available_for(mga::BufferAccess::write);
+    }
     return 0;
 }
 catch (std::exception const& e)
@@ -210,7 +227,10 @@
 int mga::MirNativeWindow::cancelBuffer(struct ANativeWindowBuffer* buffer, int fence)
 try
 {
-    driver_interpreter->driver_returns_buffer(buffer, fence);
+    mga::SyncFence sync_fence(sync_ops, mir::Fd(fence));
+    sync_fence.wait();
+
+    cancelled_buffers.push_back(buffer);
     return 0;
 }
 catch (std::exception const& e)

=== modified file 'tests/unit-tests/client/android/test_android_native_window.cpp'
--- tests/unit-tests/client/android/test_android_native_window.cpp	2014-08-05 22:36:37 +0000
+++ tests/unit-tests/client/android/test_android_native_window.cpp	2014-08-26 22:48:11 +0000
@@ -152,6 +152,24 @@
     EXPECT_EQ(fake_fd, fence_fd);
 }
 
+TEST_F(AndroidNativeWindowTest, native_window_dequeue_returns_previously_cancelled_buffer)
+{
+    using namespace testing;
+    ANativeWindowBuffer buffer;
+    int fence_fd = 33;
+
+    auto rc = window.cancelBuffer(&window, &buffer, fence_fd);
+    EXPECT_EQ(0, rc);
+
+    EXPECT_CALL(*mock_driver_interpreter, driver_requests_buffer())
+        .Times(0);
+
+    ANativeWindowBuffer* dequeued_buffer;
+    ANativeWindowBuffer* expected_buffer = &buffer;
+    window.dequeueBuffer(&window, &dequeued_buffer, &fence_fd);
+    EXPECT_EQ(dequeued_buffer, expected_buffer);
+}
+
 TEST_F(AndroidNativeWindowTest, native_window_dequeue_deprecated_hook_callable)
 {
     ANativeWindowBuffer* tmp;
@@ -179,6 +197,23 @@
     EXPECT_EQ(mock_buffer->anwb(), returned_buffer);
 }
 
+TEST_F(AndroidNativeWindowTest, native_window_dequeue_deprecated_returns_previously_cancelled_buffer)
+{
+    using namespace testing;
+    ANativeWindowBuffer buffer;
+
+    auto rc = window.cancelBuffer_DEPRECATED(&window, &buffer);
+    EXPECT_EQ(0, rc);
+
+    EXPECT_CALL(*mock_driver_interpreter, driver_requests_buffer())
+        .Times(0);
+
+    ANativeWindowBuffer* dequeued_buffer;
+    ANativeWindowBuffer* expected_buffer = &buffer;
+    window.dequeueBuffer_DEPRECATED(&window, &dequeued_buffer);
+    EXPECT_EQ(dequeued_buffer, expected_buffer);
+}
+
 /* queue hook tests */
 TEST_F(AndroidNativeWindowTest, native_window_queue_hook_callable)
 {
@@ -268,14 +303,14 @@
     EXPECT_EQ(0, ret);
 }
 
-TEST_F(AndroidNativeWindowTest, native_window_cancel_hook_behavior)
+TEST_F(AndroidNativeWindowTest, native_window_cancel_hook_does_not_call_driver_interpreter)
 {
     using namespace testing;
     ANativeWindowBuffer buffer;
     int fence_fd = 33;
 
     EXPECT_CALL(*mock_driver_interpreter, driver_returns_buffer(&buffer, _))
-        .Times(1);
+        .Times(0);
 
     auto rc = window.cancelBuffer(&window, &buffer, fence_fd);
     EXPECT_EQ(0, rc);
@@ -305,18 +340,6 @@
     EXPECT_THAT(window.queueBuffer_DEPRECATED(&window, nullptr), Eq(failure_code));
 }
 
-TEST_F(AndroidNativeWindowTest, returns_error_on_cancel_buffer_failure)
-{
-    using namespace testing;
-
-    EXPECT_CALL(*mock_driver_interpreter, driver_returns_buffer(_, _))
-        .WillOnce(Throw(std::runtime_error("")))
-        .WillOnce(Throw(std::runtime_error("")));
-
-    EXPECT_THAT(window.cancelBuffer(&window, nullptr, 0), Eq(failure_code));
-    EXPECT_THAT(window.cancelBuffer_DEPRECATED(&window, nullptr), Eq(failure_code));
-}
-
 TEST_F(AndroidNativeWindowTest, returns_error_on_query_failure)
 {
     using namespace testing;