PipeWire capturer: improvements to SharedScreenCastStream test

Remove useless comments and properly test frame values. Also rename the
FakeScreenCastStream to TestScreenCastStreamProvider.

Bug: webrtc:13429
Change-Id: I9b1943f0903101a1d9228cded541d3766879d84f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/279740
Reviewed-by: Alexander Cooper <alcooper@chromium.org>
Commit-Queue: Jan Grulich <grulja@gmail.com>
Cr-Commit-Position: refs/heads/main@{#38450}

modules/desktop_capture/BUILD.gn

@@ -101,8 +101,8 @@ if (rtc_include_tests) {
 
       sources = [
         "linux/wayland/shared_screencast_stream_unittest.cc",
-        "linux/wayland/test/fake_screencast_stream.cc",
+        "linux/wayland/test/test_screencast_stream_provider.cc",
-        "linux/wayland/test/fake_screencast_stream.h",
+        "linux/wayland/test/test_screencast_stream_provider.h",
       ]
 
       configs += [

modules/desktop_capture/linux/wayland/shared_screencast_stream.cc

@@ -649,14 +649,12 @@ void SharedScreenCastStreamPrivate::ProcessBuffer(pw_buffer* buffer) {
         mouse_cursor_ = std::make_unique<MouseCursor>(
             mouse_frame, DesktopVector(cursor->hotspot.x, cursor->hotspot.y));
 
-        // For testing purpose
         if (observer_) {
           observer_->OnCursorShapeChanged();
         }
       }
       mouse_cursor_position_.set(cursor->position.x, cursor->position.y);
 
-      // For testing purpose
       if (observer_) {
         observer_->OnCursorPositionChanged();
       }
@@ -734,7 +732,6 @@ void SharedScreenCastStreamPrivate::ProcessBuffer(pw_buffer* buffer) {
   }
 
   if (!src) {
-    // For testing purpose
     if (observer_) {
       observer_->OnFailedToProcessBuffer();
     }
@@ -862,7 +859,6 @@ void SharedScreenCastStreamPrivate::ProcessBuffer(pw_buffer* buffer) {
     }
   }
 
-  // For testing purpose
   if (observer_) {
     observer_->OnDesktopFrameChanged();
   }

modules/desktop_capture/linux/wayland/shared_screencast_stream.h

@@ -81,11 +81,6 @@ class RTC_EXPORT SharedScreenCastStream
 
  private:
   friend class SharedScreenCastStreamPrivate;
-  // Allows test cases to use private functionality
-  friend class PipeWireStreamTest;
-
-  // FIXME: is this a useful thing to be public?
-  explicit SharedScreenCastStream(Observer* notifier);
 
   SharedScreenCastStream(const SharedScreenCastStream&) = delete;
   SharedScreenCastStream& operator=(const SharedScreenCastStream&) = delete;

modules/desktop_capture/linux/wayland/shared_screencast_stream_unittest.cc

@@ -16,7 +16,7 @@
 #include "api/units/time_delta.h"
 #include "modules/desktop_capture/desktop_capturer.h"
 #include "modules/desktop_capture/desktop_frame.h"
-#include "modules/desktop_capture/linux/wayland/test/fake_screencast_stream.h"
+#include "modules/desktop_capture/linux/wayland/test/test_screencast_stream_provider.h"
 #include "modules/desktop_capture/rgba_color.h"
 #include "rtc_base/event.h"
 #include "test/gmock.h"
@@ -29,26 +29,29 @@ using ::testing::Invoke;
 namespace webrtc {
 
 constexpr TimeDelta kShortWait = TimeDelta::Seconds(2);
-constexpr TimeDelta kLongWait = TimeDelta::Seconds(10);
+constexpr TimeDelta kLongWait = TimeDelta::Seconds(15);
 
 constexpr int kBytesPerPixel = 4;
 constexpr int32_t kWidth = 800;
 constexpr int32_t kHeight = 640;
 
 class PipeWireStreamTest : public ::testing::Test,
-                           public FakeScreenCastStream::Observer,
+                           public TestScreenCastStreamProvider::Observer,
                            public SharedScreenCastStream::Observer {
  public:
   PipeWireStreamTest()
-      : fake_screencast_stream_(
+      : test_screencast_stream_provider_(
-            std::make_unique<FakeScreenCastStream>(this, kWidth, kHeight)),
+            std::make_unique<TestScreenCastStreamProvider>(this,
-        shared_screencast_stream_(new SharedScreenCastStream()) {
+                                                           kWidth,
+                                                           kHeight)) {
+    shared_screencast_stream_ = SharedScreenCastStream::CreateDefault();
     shared_screencast_stream_->SetObserver(this);
   }
 
   ~PipeWireStreamTest() override {}
 
   // FakeScreenCastPortal::Observer
+  MOCK_METHOD(void, OnBufferAdded, (), (override));
   MOCK_METHOD(void, OnFrameRecorded, (), (override));
   MOCK_METHOD(void, OnStreamReady, (uint32_t stream_node_id), (override));
   MOCK_METHOD(void, OnStartStreaming, (), (override));
@@ -67,32 +70,40 @@ class PipeWireStreamTest : public ::testing::Test,
  protected:
   uint recorded_frames_ = 0;
   bool streaming_ = false;
-  std::unique_ptr<FakeScreenCastStream> fake_screencast_stream_;
+  std::unique_ptr<TestScreenCastStreamProvider>
+      test_screencast_stream_provider_;
   rtc::scoped_refptr<SharedScreenCastStream> shared_screencast_stream_;
 };
 
 TEST_F(PipeWireStreamTest, TestPipeWire) {
   // Set expectations for PipeWire to successfully connect both streams
   rtc::Event waitConnectEvent;
+  rtc::Event waitAddBufferEvent;
+
   EXPECT_CALL(*this, OnStreamReady(_))
       .WillOnce(Invoke(this, &PipeWireStreamTest::StartScreenCastStream));
   EXPECT_CALL(*this, OnStartStreaming).WillOnce([&waitConnectEvent] {
     waitConnectEvent.Set();
   });
+  EXPECT_CALL(*this, OnBufferAdded).WillRepeatedly([&waitAddBufferEvent] {
+    waitAddBufferEvent.Set();
+  });
 
   // Give it some time to connect, the order between these shouldn't matter, but
   // we need to be sure we are connected before we proceed to work with frames.
   waitConnectEvent.Wait(kLongWait);
 
+  // Wait for an empty buffer to be added
+  waitAddBufferEvent.Wait(kShortWait);
+
   rtc::Event frameRetrievedEvent;
   EXPECT_CALL(*this, OnFrameRecorded).Times(3);
   EXPECT_CALL(*this, OnDesktopFrameChanged)
       .WillRepeatedly([&frameRetrievedEvent] { frameRetrievedEvent.Set(); });
 
   // Record a frame in FakePipeWireStream
-  RgbaColor red_color(255, 0, 0);
+  RgbaColor red_color(0, 0, 255);
-  fake_screencast_stream_->RecordFrame(red_color);
+  test_screencast_stream_provider_->RecordFrame(red_color);
-  frameRetrievedEvent.Wait(kShortWait);
 
   // Retrieve a frame from SharedScreenCastStream
   frameRetrievedEvent.Wait(kShortWait);
@@ -105,11 +116,11 @@ TEST_F(PipeWireStreamTest, TestPipeWire) {
   EXPECT_EQ(frame->rect().width(), kWidth);
   EXPECT_EQ(frame->rect().height(), kHeight);
   EXPECT_EQ(frame->stride(), frame->rect().width() * kBytesPerPixel);
-  EXPECT_EQ(frame->data()[0], static_cast<uint8_t>(red_color.ToUInt32()));
+  EXPECT_EQ(RgbaColor(frame->data()), red_color);
 
   // Test DesktopFrameQueue
   RgbaColor green_color(0, 255, 0);
-  fake_screencast_stream_->RecordFrame(green_color);
+  test_screencast_stream_provider_->RecordFrame(green_color);
   frameRetrievedEvent.Wait(kShortWait);
   std::unique_ptr<SharedDesktopFrame> frame2 =
       shared_screencast_stream_->CaptureFrame();
@@ -118,7 +129,7 @@ TEST_F(PipeWireStreamTest, TestPipeWire) {
   EXPECT_EQ(frame2->rect().width(), kWidth);
   EXPECT_EQ(frame2->rect().height(), kHeight);
   EXPECT_EQ(frame2->stride(), frame->rect().width() * kBytesPerPixel);
-  EXPECT_EQ(frame2->data()[0], static_cast<uint8_t>(green_color.ToUInt32()));
+  EXPECT_EQ(RgbaColor(frame2->data()), green_color);
 
   // Thanks to DesktopFrameQueue we should be able to have two frames shared
   EXPECT_EQ(frame->IsShared(), true);
@@ -127,14 +138,18 @@ TEST_F(PipeWireStreamTest, TestPipeWire) {
 
   // This should result into overwriting a frame in use
   rtc::Event frameRecordedEvent;
-  RgbaColor blue_color(0, 0, 255);
+  RgbaColor blue_color(255, 0, 0);
   EXPECT_CALL(*this, OnFailedToProcessBuffer).WillOnce([&frameRecordedEvent] {
     frameRecordedEvent.Set();
   });
 
-  fake_screencast_stream_->RecordFrame(blue_color);
+  test_screencast_stream_provider_->RecordFrame(blue_color);
   frameRecordedEvent.Wait(kShortWait);
 
+  // First frame should be now overwritten with blue color
+  frameRetrievedEvent.Wait(kShortWait);
+  EXPECT_EQ(RgbaColor(frame->data()), blue_color);
+
   // Test disconnection from stream
   EXPECT_CALL(*this, OnStopStreaming);
   shared_screencast_stream_->StopScreenCastStream();

modules/desktop_capture/linux/wayland/test/shared_screencast_stream_test.py

@@ -28,16 +28,16 @@ def _ParseArgs():
       description='Run shared_screencast_screen test.')
   parser.add_argument('build_dir',
                       help='Path to the build directory (e.g. out/Release).')
+  parser.add_argument(
+      '--isolated-script-test-output',
+      default=None,
+      help='Path to output JSON file which Chromium infra requires.')
   # Unused args
   # We just need to avoid passing these to the test
   parser.add_argument(
       '--isolated-script-test-perf-output',
       default=None,
       help='Path to store perf results in histogram proto format.')
-  parser.add_argument(
-      '--isolated-script-test-output',
-      default=None,
-      help='Path to output JSON file which Chromium infra requires.')
 
   return parser.parse_known_args()
 

modules/desktop_capture/linux/wayland/test/test_screencast_stream_provider.cc

@@ -9,7 +9,7 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
-#include "modules/desktop_capture/linux/wayland/test/fake_screencast_stream.h"
+#include "modules/desktop_capture/linux/wayland/test/test_screencast_stream_provider.h"
 
 #include <fcntl.h>
 #include <sys/mman.h>
@@ -37,9 +37,9 @@ const char kPipeWireLib[] = "libpipewire-0.3.so.0";
 
 constexpr int kBytesPerPixel = 4;
 
-FakeScreenCastStream::FakeScreenCastStream(Observer* observer,
+TestScreenCastStreamProvider::TestScreenCastStreamProvider(Observer* observer,
-                                           uint32_t width,
+                                                           uint32_t width,
-                                           uint32_t height)
+                                                           uint32_t height)
     : observer_(observer), width_(width), height_(height) {
 #if defined(WEBRTC_DLOPEN_PIPEWIRE)
   StubPathMap paths;
@@ -126,7 +126,7 @@ FakeScreenCastStream::FakeScreenCastStream(Observer* observer,
   return;
 }
 
-FakeScreenCastStream::~FakeScreenCastStream() {
+TestScreenCastStreamProvider::~TestScreenCastStreamProvider() {
   if (pw_main_loop_) {
     pw_thread_loop_stop(pw_main_loop_);
   }
@@ -148,7 +148,7 @@ FakeScreenCastStream::~FakeScreenCastStream() {
   }
 }
 
-void FakeScreenCastStream::RecordFrame(RgbaColor rgba_color) {
+void TestScreenCastStreamProvider::RecordFrame(RgbaColor rgba_color) {
   const char* error;
   if (pw_stream_get_state(pw_stream_, &error) != PW_STREAM_STATE_STREAMING) {
     if (error) {
@@ -195,36 +195,39 @@ void FakeScreenCastStream::RecordFrame(RgbaColor rgba_color) {
   }
 }
 
-void FakeScreenCastStream::StartStreaming() {
+void TestScreenCastStreamProvider::StartStreaming() {
   if (pw_stream_ && pw_node_id_ != 0) {
     pw_stream_set_active(pw_stream_, true);
   }
 }
 
-void FakeScreenCastStream::StopStreaming() {
+void TestScreenCastStreamProvider::StopStreaming() {
   if (pw_stream_ && pw_node_id_ != 0) {
     pw_stream_set_active(pw_stream_, false);
   }
 }
 
 // static
-void FakeScreenCastStream::OnCoreError(void* data,
+void TestScreenCastStreamProvider::OnCoreError(void* data,
-                                       uint32_t id,
+                                               uint32_t id,
-                                       int seq,
+                                               int seq,
-                                       int res,
+                                               int res,
-                                       const char* message) {
+                                               const char* message) {
-  FakeScreenCastStream* that = static_cast<FakeScreenCastStream*>(data);
+  TestScreenCastStreamProvider* that =
+      static_cast<TestScreenCastStreamProvider*>(data);
   RTC_DCHECK(that);
 
   RTC_LOG(LS_ERROR) << "PipeWire test: PipeWire remote error: " << message;
 }
 
 // static
-void FakeScreenCastStream::OnStreamStateChanged(void* data,
+void TestScreenCastStreamProvider::OnStreamStateChanged(
-                                                pw_stream_state old_state,
+    void* data,
-                                                pw_stream_state state,
+    pw_stream_state old_state,
-                                                const char* error_message) {
+    pw_stream_state state,
-  FakeScreenCastStream* that = static_cast<FakeScreenCastStream*>(data);
+    const char* error_message) {
+  TestScreenCastStreamProvider* that =
+      static_cast<TestScreenCastStreamProvider*>(data);
   RTC_DCHECK(that);
 
   switch (state) {
@@ -260,10 +263,12 @@ void FakeScreenCastStream::OnStreamStateChanged(void* data,
 }
 
 // static
-void FakeScreenCastStream::OnStreamParamChanged(void* data,
+void TestScreenCastStreamProvider::OnStreamParamChanged(
-                                                uint32_t id,
+    void* data,
-                                                const struct spa_pod* format) {
+    uint32_t id,
-  FakeScreenCastStream* that = static_cast<FakeScreenCastStream*>(data);
+    const struct spa_pod* format) {
+  TestScreenCastStreamProvider* that =
+      static_cast<TestScreenCastStreamProvider*>(data);
   RTC_DCHECK(that);
 
   RTC_LOG(LS_INFO) << "PipeWire test: PipeWire stream format changed.";
@@ -302,8 +307,10 @@ void FakeScreenCastStream::OnStreamParamChanged(void* data,
 }
 
 // static
-void FakeScreenCastStream::OnStreamAddBuffer(void* data, pw_buffer* buffer) {
+void TestScreenCastStreamProvider::OnStreamAddBuffer(void* data,
-  FakeScreenCastStream* that = static_cast<FakeScreenCastStream*>(data);
+                                                     pw_buffer* buffer) {
+  TestScreenCastStreamProvider* that =
+      static_cast<TestScreenCastStreamProvider*>(data);
   RTC_DCHECK(that);
 
   struct spa_data* spa_data = buffer->buffer->datas;
@@ -345,14 +352,17 @@ void FakeScreenCastStream::OnStreamAddBuffer(void* data, pw_buffer* buffer) {
   if (spa_data->data == MAP_FAILED) {
     RTC_LOG(LS_ERROR) << "PipeWire test: Failed to mmap memory";
   } else {
+    that->observer_->OnBufferAdded();
     RTC_LOG(LS_INFO) << "PipeWire test: Memfd created successfully: "
                      << spa_data->data << spa_data->maxsize;
   }
 }
 
 // static
-void FakeScreenCastStream::OnStreamRemoveBuffer(void* data, pw_buffer* buffer) {
+void TestScreenCastStreamProvider::OnStreamRemoveBuffer(void* data,
-  FakeScreenCastStream* that = static_cast<FakeScreenCastStream*>(data);
+                                                        pw_buffer* buffer) {
+  TestScreenCastStreamProvider* that =
+      static_cast<TestScreenCastStreamProvider*>(data);
   RTC_DCHECK(that);
 
   struct spa_buffer* spa_buffer = buffer->buffer;
@@ -363,7 +373,7 @@ void FakeScreenCastStream::OnStreamRemoveBuffer(void* data, pw_buffer* buffer) {
   }
 }
 
-uint32_t FakeScreenCastStream::PipeWireNodeId() {
+uint32_t TestScreenCastStreamProvider::PipeWireNodeId() {
   return pw_node_id_;
 }
 

modules/desktop_capture/linux/wayland/test/test_screencast_stream_provider.h

@@ -8,8 +8,8 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
-#ifndef MODULES_DESKTOP_CAPTURE_LINUX_WAYLAND_TEST_FAKE_SCREENCAST_STREAM_H_
+#ifndef MODULES_DESKTOP_CAPTURE_LINUX_WAYLAND_TEST_TEST_SCREENCAST_STREAM_PROVIDER_H_
-#define MODULES_DESKTOP_CAPTURE_LINUX_WAYLAND_TEST_FAKE_SCREENCAST_STREAM_H_
+#define MODULES_DESKTOP_CAPTURE_LINUX_WAYLAND_TEST_TEST_SCREENCAST_STREAM_PROVIDER_H_
 
 #include <pipewire/pipewire.h>
 #include <spa/param/video/format-utils.h>
@@ -20,10 +20,11 @@
 
 namespace webrtc {
 
-class FakeScreenCastStream {
+class TestScreenCastStreamProvider {
  public:
   class Observer {
    public:
+    virtual void OnBufferAdded() = 0;
     virtual void OnFrameRecorded() = 0;
     virtual void OnStreamReady(uint32_t stream_node_id) = 0;
     virtual void OnStartStreaming() = 0;
@@ -34,10 +35,10 @@ class FakeScreenCastStream {
     virtual ~Observer() = default;
   };
 
-  explicit FakeScreenCastStream(Observer* observer,
+  explicit TestScreenCastStreamProvider(Observer* observer,
-                                uint32_t width,
+                                        uint32_t width,
-                                uint32_t height);
+                                        uint32_t height);
-  ~FakeScreenCastStream();
+  ~TestScreenCastStreamProvider();
 
   uint32_t PipeWireNodeId();
 
@@ -89,4 +90,4 @@ class FakeScreenCastStream {
 
 }  // namespace webrtc
 
-#endif  // MODULES_DESKTOP_CAPTURE_LINUX_WAYLAND_TEST_FAKE_SCREENCAST_STREAM_H_
+#endif  // MODULES_DESKTOP_CAPTURE_LINUX_WAYLAND_TEST_TEST_SCREENCAST_STREAM_PROVIDER_H_