From cc98238f6d5c9ab2d5a0deaac8db238343fbee57 Mon Sep 17 00:00:00 2001 From: Jan Grulich Date: Thu, 20 Oct 2022 22:56:58 +0200 Subject: [PATCH] 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 Commit-Queue: Jan Grulich Cr-Commit-Position: refs/heads/main@{#38450} --- modules/desktop_capture/BUILD.gn | 4 +- .../linux/wayland/shared_screencast_stream.cc | 4 -- .../linux/wayland/shared_screencast_stream.h | 5 -- .../shared_screencast_stream_unittest.cc | 45 ++++++++----- .../test/shared_screencast_stream_test.py | 8 +-- ....cc => test_screencast_stream_provider.cc} | 66 +++++++++++-------- ...am.h => test_screencast_stream_provider.h} | 17 ++--- 7 files changed, 83 insertions(+), 66 deletions(-) rename modules/desktop_capture/linux/wayland/test/{fake_screencast_stream.cc => test_screencast_stream_provider.cc} (83%) rename modules/desktop_capture/linux/wayland/test/{fake_screencast_stream.h => test_screencast_stream_provider.h} (81%) diff --git a/modules/desktop_capture/BUILD.gn b/modules/desktop_capture/BUILD.gn index db70df2fc7..482b03c958 100644 --- a/modules/desktop_capture/BUILD.gn +++ b/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/fake_screencast_stream.h", + "linux/wayland/test/test_screencast_stream_provider.cc", + "linux/wayland/test/test_screencast_stream_provider.h", ] configs += [ diff --git a/modules/desktop_capture/linux/wayland/shared_screencast_stream.cc b/modules/desktop_capture/linux/wayland/shared_screencast_stream.cc index a8704805af..615d42e2ee 100644 --- a/modules/desktop_capture/linux/wayland/shared_screencast_stream.cc +++ b/modules/desktop_capture/linux/wayland/shared_screencast_stream.cc @@ -649,14 +649,12 @@ void SharedScreenCastStreamPrivate::ProcessBuffer(pw_buffer* buffer) { mouse_cursor_ = std::make_unique( 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(); } diff --git a/modules/desktop_capture/linux/wayland/shared_screencast_stream.h b/modules/desktop_capture/linux/wayland/shared_screencast_stream.h index f18c889810..8ab951ca37 100644 --- a/modules/desktop_capture/linux/wayland/shared_screencast_stream.h +++ b/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; diff --git a/modules/desktop_capture/linux/wayland/shared_screencast_stream_unittest.cc b/modules/desktop_capture/linux/wayland/shared_screencast_stream_unittest.cc index 23296f5d14..196e1f7972 100644 --- a/modules/desktop_capture/linux/wayland/shared_screencast_stream_unittest.cc +++ b/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_( - std::make_unique(this, kWidth, kHeight)), - shared_screencast_stream_(new SharedScreenCastStream()) { + : test_screencast_stream_provider_( + std::make_unique(this, + 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 fake_screencast_stream_; + std::unique_ptr + test_screencast_stream_provider_; rtc::scoped_refptr 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); - fake_screencast_stream_->RecordFrame(red_color); - frameRetrievedEvent.Wait(kShortWait); + RgbaColor red_color(0, 0, 255); + test_screencast_stream_provider_->RecordFrame(red_color); // 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(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 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(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(); diff --git a/modules/desktop_capture/linux/wayland/test/shared_screencast_stream_test.py b/modules/desktop_capture/linux/wayland/test/shared_screencast_stream_test.py index 39292c88ea..bf97d0c30e 100644 --- a/modules/desktop_capture/linux/wayland/test/shared_screencast_stream_test.py +++ b/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() diff --git a/modules/desktop_capture/linux/wayland/test/fake_screencast_stream.cc b/modules/desktop_capture/linux/wayland/test/test_screencast_stream_provider.cc similarity index 83% rename from modules/desktop_capture/linux/wayland/test/fake_screencast_stream.cc rename to modules/desktop_capture/linux/wayland/test/test_screencast_stream_provider.cc index 8f973da060..c49823d510 100644 --- a/modules/desktop_capture/linux/wayland/test/fake_screencast_stream.cc +++ b/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 #include @@ -37,9 +37,9 @@ const char kPipeWireLib[] = "libpipewire-0.3.so.0"; constexpr int kBytesPerPixel = 4; -FakeScreenCastStream::FakeScreenCastStream(Observer* observer, - uint32_t width, - uint32_t height) +TestScreenCastStreamProvider::TestScreenCastStreamProvider(Observer* observer, + uint32_t width, + 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, - uint32_t id, - int seq, - int res, - const char* message) { - FakeScreenCastStream* that = static_cast(data); +void TestScreenCastStreamProvider::OnCoreError(void* data, + uint32_t id, + int seq, + int res, + const char* message) { + TestScreenCastStreamProvider* that = + static_cast(data); RTC_DCHECK(that); RTC_LOG(LS_ERROR) << "PipeWire test: PipeWire remote error: " << message; } // static -void FakeScreenCastStream::OnStreamStateChanged(void* data, - pw_stream_state old_state, - pw_stream_state state, - const char* error_message) { - FakeScreenCastStream* that = static_cast(data); +void TestScreenCastStreamProvider::OnStreamStateChanged( + void* data, + pw_stream_state old_state, + pw_stream_state state, + const char* error_message) { + TestScreenCastStreamProvider* that = + static_cast(data); RTC_DCHECK(that); switch (state) { @@ -260,10 +263,12 @@ void FakeScreenCastStream::OnStreamStateChanged(void* data, } // static -void FakeScreenCastStream::OnStreamParamChanged(void* data, - uint32_t id, - const struct spa_pod* format) { - FakeScreenCastStream* that = static_cast(data); +void TestScreenCastStreamProvider::OnStreamParamChanged( + void* data, + uint32_t id, + const struct spa_pod* format) { + TestScreenCastStreamProvider* that = + static_cast(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) { - FakeScreenCastStream* that = static_cast(data); +void TestScreenCastStreamProvider::OnStreamAddBuffer(void* data, + pw_buffer* buffer) { + TestScreenCastStreamProvider* that = + static_cast(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) { - FakeScreenCastStream* that = static_cast(data); +void TestScreenCastStreamProvider::OnStreamRemoveBuffer(void* data, + pw_buffer* buffer) { + TestScreenCastStreamProvider* that = + static_cast(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_; } diff --git a/modules/desktop_capture/linux/wayland/test/fake_screencast_stream.h b/modules/desktop_capture/linux/wayland/test/test_screencast_stream_provider.h similarity index 81% rename from modules/desktop_capture/linux/wayland/test/fake_screencast_stream.h rename to modules/desktop_capture/linux/wayland/test/test_screencast_stream_provider.h index 1b3bb06c47..d893aa63ab 100644 --- a/modules/desktop_capture/linux/wayland/test/fake_screencast_stream.h +++ b/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_ -#define 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_TEST_SCREENCAST_STREAM_PROVIDER_H_ #include #include @@ -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, - uint32_t width, - uint32_t height); - ~FakeScreenCastStream(); + explicit TestScreenCastStreamProvider(Observer* observer, + uint32_t width, + uint32_t height); + ~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_