From 86314cfb5dc09bba15a1607585e9ddd544078ac5 Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Tue, 17 Sep 2019 20:29:59 +0200 Subject: [PATCH] Cleaning up C++14 move into lambda TODOs. Bug: webrtc:10945 Change-Id: I4d2f358b0e33b37e4b4f7bfcf3f6cd55e8d46bf9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/153240 Commit-Queue: Sebastian Jansson Reviewed-by: Karl Wiberg Cr-Commit-Position: refs/heads/master@{#29212} --- common_video/incoming_video_stream.cc | 20 +++--- logging/rtc_event_log/rtc_event_log_impl.cc | 43 +++--------- .../task_utils/repeating_task_unittest.cc | 27 +++----- test/network/network_emulation_manager.cc | 44 +++---------- test/peer_scenario/scenario_connection.cc | 30 ++++----- .../simulated_time_controller_unittest.cc | 23 +++---- video/video_stream_encoder.cc | 66 ++++++++----------- video/video_stream_encoder.h | 2 - 8 files changed, 83 insertions(+), 172 deletions(-) diff --git a/common_video/incoming_video_stream.cc b/common_video/incoming_video_stream.cc index 69e9d9c844..d1f8beac5b 100644 --- a/common_video/incoming_video_stream.cc +++ b/common_video/incoming_video_stream.cc @@ -38,18 +38,14 @@ void IncomingVideoStream::OnFrame(const VideoFrame& video_frame) { TRACE_EVENT0("webrtc", "IncomingVideoStream::OnFrame"); RTC_CHECK_RUNS_SERIALIZED(&decoder_race_checker_); RTC_DCHECK(!incoming_render_queue_.IsCurrent()); - // TODO(srte): This struct should be replaced by a lambda with move capture - // when C++14 lambdas are allowed. - struct NewFrameTask { - void operator()() { - RTC_DCHECK(stream->incoming_render_queue_.IsCurrent()); - if (stream->render_buffers_.AddFrame(std::move(frame)) == 1) - stream->Dequeue(); - } - IncomingVideoStream* stream; - VideoFrame frame; - }; - incoming_render_queue_.PostTask(NewFrameTask{this, std::move(video_frame)}); + // TODO(srte): Using video_frame = std::move(video_frame) would move the frame + // into the lambda instead of copying it, but it doesn't work unless we change + // OnFrame to take its frame argument by value instead of const reference. + incoming_render_queue_.PostTask([this, video_frame = video_frame]() mutable { + RTC_DCHECK(incoming_render_queue_.IsCurrent()); + if (render_buffers_.AddFrame(std::move(video_frame)) == 1) + Dequeue(); + }); } void IncomingVideoStream::Dequeue() { diff --git a/logging/rtc_event_log/rtc_event_log_impl.cc b/logging/rtc_event_log/rtc_event_log_impl.cc index f020a7ea94..833f395dac 100644 --- a/logging/rtc_event_log/rtc_event_log_impl.cc +++ b/logging/rtc_event_log/rtc_event_log_impl.cc @@ -36,26 +36,6 @@ constexpr size_t kMaxEventsInHistory = 10000; // to prevent an attack via unreasonable memory use. constexpr size_t kMaxEventsInConfigHistory = 1000; -// TODO(eladalon): This class exists because C++11 doesn't allow transferring a -// unique_ptr to a lambda (a copy constructor is required). We should get -// rid of this when we move to C++14. -template -class ResourceOwningTask final : public QueuedTask { - public: - ResourceOwningTask(std::unique_ptr resource, - std::function)> handler) - : resource_(std::move(resource)), handler_(handler) {} - - bool Run() override { - handler_(std::move(resource_)); - return true; - } - - private: - std::unique_ptr resource_; - std::function)> handler_; -}; - std::unique_ptr CreateEncoder( RtcEventLog::EncodingType type) { switch (type) { @@ -113,9 +93,11 @@ bool RtcEventLogImpl::StartLogging(std::unique_ptr output, RTC_LOG(LS_INFO) << "Starting WebRTC event log. (Timestamp, UTC) = " << "(" << timestamp_us << ", " << utc_time_us << ")."; + RTC_DCHECK_RUN_ON(&logging_state_checker_); + logging_state_started_ = true; // Binding to |this| is safe because |this| outlives the |task_queue_|. - auto start = [this, output_period_ms, timestamp_us, - utc_time_us](std::unique_ptr output) { + task_queue_->PostTask([this, output_period_ms, timestamp_us, utc_time_us, + output = std::move(output)]() mutable { RTC_DCHECK_RUN_ON(task_queue_.get()); RTC_DCHECK(output->IsActive()); output_period_ms_ = output_period_ms; @@ -123,13 +105,7 @@ bool RtcEventLogImpl::StartLogging(std::unique_ptr output, num_config_events_written_ = 0; WriteToOutput(event_encoder_->EncodeLogStart(timestamp_us, utc_time_us)); LogEventsFromMemoryToOutput(); - }; - - RTC_DCHECK_RUN_ON(&logging_state_checker_); - logging_state_started_ = true; - - task_queue_->PostTask(std::make_unique>( - std::move(output), start)); + }); return true; } @@ -168,15 +144,12 @@ void RtcEventLogImpl::Log(std::unique_ptr event) { RTC_CHECK(event); // Binding to |this| is safe because |this| outlives the |task_queue_|. - auto event_handler = [this](std::unique_ptr unencoded_event) { + task_queue_->PostTask([this, event = std::move(event)]() mutable { RTC_DCHECK_RUN_ON(task_queue_.get()); - LogToMemory(std::move(unencoded_event)); + LogToMemory(std::move(event)); if (event_output_) ScheduleOutput(); - }; - - task_queue_->PostTask(std::make_unique>( - std::move(event), event_handler)); + }); } void RtcEventLogImpl::ScheduleOutput() { diff --git a/rtc_base/task_utils/repeating_task_unittest.cc b/rtc_base/task_utils/repeating_task_unittest.cc index 2532098dd6..469ee316f3 100644 --- a/rtc_base/task_utils/repeating_task_unittest.cc +++ b/rtc_base/task_utils/repeating_task_unittest.cc @@ -60,18 +60,6 @@ class MoveOnlyClosure { private: MockClosure* mock_; }; - -// Helper closure class to stop repeating task on a task queue. This is -// equivalent to [handle{move(handle)}] { handle.Stop(); } in c++14. -class TaskHandleStopper { - public: - explicit TaskHandleStopper(RepeatingTaskHandle handle) - : handle_(std::move(handle)) {} - void operator()() { handle_.Stop(); } - - private: - RepeatingTaskHandle handle_; -}; } // namespace TEST(RepeatingTaskTest, TaskIsStoppedOnStop) { @@ -91,7 +79,8 @@ TEST(RepeatingTaskTest, TaskIsStoppedOnStop) { Sleep(kShortInterval * (kShortIntervalCount + kMargin)); EXPECT_EQ(counter.load(), kShortIntervalCount); - task_queue.PostTask(TaskHandleStopper(std::move(handle))); + task_queue.PostTask( + [handle = std::move(handle)]() mutable { handle.Stop(); }); // Sleep long enough that the task would run at least once more if not // stopped. Sleep(kLongInterval * 2); @@ -144,7 +133,8 @@ TEST(RepeatingTaskTest, CancelDelayedTaskBeforeItRuns) { TaskQueueForTest task_queue("queue"); auto handle = RepeatingTaskHandle::DelayedStart( task_queue.Get(), TimeDelta::ms(100), MoveOnlyClosure(&mock)); - task_queue.PostTask(TaskHandleStopper(std::move(handle))); + task_queue.PostTask( + [handle = std::move(handle)]() mutable { handle.Stop(); }); EXPECT_TRUE(done.Wait(kTimeout.ms())); } @@ -156,7 +146,8 @@ TEST(RepeatingTaskTest, CancelTaskAfterItRuns) { TaskQueueForTest task_queue("queue"); auto handle = RepeatingTaskHandle::Start(task_queue.Get(), MoveOnlyClosure(&mock)); - task_queue.PostTask(TaskHandleStopper(std::move(handle))); + task_queue.PostTask( + [handle = std::move(handle)]() mutable { handle.Stop(); }); EXPECT_TRUE(done.Wait(kTimeout.ms())); } @@ -223,9 +214,11 @@ TEST(RepeatingTaskTest, Example) { RepeatingTaskHandle handle; object->StartPeriodicTask(&handle, task_queue.Get()); // Restart the task - task_queue.PostTask(TaskHandleStopper(std::move(handle))); + task_queue.PostTask( + [handle = std::move(handle)]() mutable { handle.Stop(); }); object->StartPeriodicTask(&handle, task_queue.Get()); - task_queue.PostTask(TaskHandleStopper(std::move(handle))); + task_queue.PostTask( + [handle = std::move(handle)]() mutable { handle.Stop(); }); struct Destructor { void operator()() { object.reset(); } std::unique_ptr object; diff --git a/test/network/network_emulation_manager.cc b/test/network/network_emulation_manager.cc index febcd61dff..73f10214f7 100644 --- a/test/network/network_emulation_manager.cc +++ b/test/network/network_emulation_manager.cc @@ -27,29 +27,6 @@ namespace { constexpr uint32_t kMinIPv4Address = 0xC0A80000; // uint32_t representation of 192.168.255.255 address constexpr uint32_t kMaxIPv4Address = 0xC0A8FFFF; - -template -class ResourceOwningTask final : public QueuedTask { - public: - ResourceOwningTask(T&& resource, Closure&& handler) - : resource_(std::move(resource)), - handler_(std::forward(handler)) {} - - bool Run() override { - handler_(std::move(resource_)); - return true; - } - - private: - T resource_; - Closure handler_; -}; -template -std::unique_ptr CreateResourceOwningTask(T resource, - Closure&& closure) { - return std::make_unique>( - std::forward(resource), std::forward(closure)); -} } // namespace NetworkEmulationManagerImpl::NetworkEmulationManagerImpl() @@ -79,10 +56,9 @@ EmulatedNetworkNode* NetworkEmulationManagerImpl::CreateEmulatedNode( auto node = std::make_unique( clock_, &task_queue_, std::move(network_behavior)); EmulatedNetworkNode* out = node.get(); - task_queue_.PostTask(CreateResourceOwningTask( - std::move(node), [this](std::unique_ptr node) { - network_nodes_.push_back(std::move(node)); - })); + task_queue_.PostTask([this, node = std::move(node)]() mutable { + network_nodes_.push_back(std::move(node)); + }); return out; } @@ -203,9 +179,8 @@ NetworkEmulationManagerImpl::CreateRandomWalkCrossTraffic( std::make_unique(config, traffic_route); RandomWalkCrossTraffic* out = traffic.get(); - task_queue_.PostTask(CreateResourceOwningTask( - std::move(traffic), - [this, config](std::unique_ptr traffic) { + task_queue_.PostTask( + [this, config, traffic = std::move(traffic)]() mutable { auto* traffic_ptr = traffic.get(); random_cross_traffics_.push_back(std::move(traffic)); RepeatingTaskHandle::Start(task_queue_.Get(), @@ -213,7 +188,7 @@ NetworkEmulationManagerImpl::CreateRandomWalkCrossTraffic( traffic_ptr->Process(Now()); return config.min_packet_interval; }); - })); + }); return out; } @@ -224,9 +199,8 @@ NetworkEmulationManagerImpl::CreatePulsedPeaksCrossTraffic( auto traffic = std::make_unique(config, traffic_route); PulsedPeaksCrossTraffic* out = traffic.get(); - task_queue_.PostTask(CreateResourceOwningTask( - std::move(traffic), - [this, config](std::unique_ptr traffic) { + task_queue_.PostTask( + [this, config, traffic = std::move(traffic)]() mutable { auto* traffic_ptr = traffic.get(); pulsed_cross_traffics_.push_back(std::move(traffic)); RepeatingTaskHandle::Start(task_queue_.Get(), @@ -234,7 +208,7 @@ NetworkEmulationManagerImpl::CreatePulsedPeaksCrossTraffic( traffic_ptr->Process(Now()); return config.min_packet_interval; }); - })); + }); return out; } diff --git a/test/peer_scenario/scenario_connection.cc b/test/peer_scenario/scenario_connection.cc index 6f5179989d..d79141cbf7 100644 --- a/test/peer_scenario/scenario_connection.cc +++ b/test/peer_scenario/scenario_connection.cc @@ -138,28 +138,26 @@ void ScenarioIceConnectionImpl::SendRtpPacket( rtc::ArrayView packet_view) { rtc::CopyOnWriteBuffer packet(packet_view.data(), packet_view.size(), ::cricket::kMaxRtpPacketLen); - // TODO(srte): Move |packet| into lambda when we have c++14. - network_thread_->PostTask(RTC_FROM_HERE, [this, packet]() mutable { - RTC_DCHECK_RUN_ON(network_thread_); - if (rtp_transport_ == nullptr) - return; - rtp_transport_->SendRtpPacket(&packet, rtc::PacketOptions(), - cricket::PF_SRTP_BYPASS); - }); + network_thread_->PostTask( + RTC_FROM_HERE, [this, packet = std::move(packet)]() mutable { + RTC_DCHECK_RUN_ON(network_thread_); + if (rtp_transport_ != nullptr) + rtp_transport_->SendRtpPacket(&packet, rtc::PacketOptions(), + cricket::PF_SRTP_BYPASS); + }); } void ScenarioIceConnectionImpl::SendRtcpPacket( rtc::ArrayView packet_view) { rtc::CopyOnWriteBuffer packet(packet_view.data(), packet_view.size(), ::cricket::kMaxRtpPacketLen); - // TODO(srte): Move |packet| into lambda when we have c++14. - network_thread_->PostTask(RTC_FROM_HERE, [this, packet]() mutable { - RTC_DCHECK_RUN_ON(network_thread_); - if (rtp_transport_ == nullptr) - return; - rtp_transport_->SendRtcpPacket(&packet, rtc::PacketOptions(), - cricket::PF_SRTP_BYPASS); - }); + network_thread_->PostTask( + RTC_FROM_HERE, [this, packet = std::move(packet)]() mutable { + RTC_DCHECK_RUN_ON(network_thread_); + if (rtp_transport_ != nullptr) + rtp_transport_->SendRtcpPacket(&packet, rtc::PacketOptions(), + cricket::PF_SRTP_BYPASS); + }); } void ScenarioIceConnectionImpl::SetRemoteSdp(SdpType type, const std::string& remote_sdp) { diff --git a/test/time_controller/simulated_time_controller_unittest.cc b/test/time_controller/simulated_time_controller_unittest.cc index 7ff4288d2d..5fc944358d 100644 --- a/test/time_controller/simulated_time_controller_unittest.cc +++ b/test/time_controller/simulated_time_controller_unittest.cc @@ -28,18 +28,6 @@ using ::testing::MockFunction; using ::testing::NiceMock; using ::testing::Return; constexpr Timestamp kStartTime = Timestamp::Seconds<1000>(); - -// Helper closure class to stop repeating task on a task queue. This is -// equivalent to [handle{move(handle)}] { handle.Stop(); } in c++14. -class TaskHandleStopper { - public: - explicit TaskHandleStopper(RepeatingTaskHandle handle) - : handle_(std::move(handle)) {} - void operator()() { handle_.Stop(); } - - private: - RepeatingTaskHandle handle_; -}; } // namespace TEST(SimulatedTimeControllerTest, TaskIsStoppedOnStop) { @@ -61,7 +49,9 @@ TEST(SimulatedTimeControllerTest, TaskIsStoppedOnStop) { time_simulation.Sleep(kShortInterval * (kShortIntervalCount + kMargin)); EXPECT_EQ(counter.load(), kShortIntervalCount); - task_queue.PostTask(TaskHandleStopper(std::move(handle))); + task_queue.PostTask( + [handle = std::move(handle)]() mutable { handle.Stop(); }); + // Sleep long enough that the task would run at least once more if not // stopped. time_simulation.Sleep(kLongInterval * 2); @@ -108,9 +98,12 @@ TEST(SimulatedTimeControllerTest, Example) { RepeatingTaskHandle handle; object->StartPeriodicTask(&handle, &task_queue); // Restart the task - task_queue.PostTask(TaskHandleStopper(std::move(handle))); + task_queue.PostTask( + [handle = std::move(handle)]() mutable { handle.Stop(); }); object->StartPeriodicTask(&handle, &task_queue); - task_queue.PostTask(TaskHandleStopper(std::move(handle))); + task_queue.PostTask( + [handle = std::move(handle)]() mutable { handle.Stop(); }); + struct Destructor { void operator()() { object.reset(); } std::unique_ptr object; diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 5bdfa72539..e1ca55722d 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -630,49 +630,35 @@ void VideoStreamEncoder::SetStartBitrate(int start_bitrate_bps) { void VideoStreamEncoder::ConfigureEncoder(VideoEncoderConfig config, size_t max_data_payload_length) { - // TODO(srte): This struct should be replaced by a lambda with move capture - // when C++14 lambda is allowed. - struct ConfigureEncoderTask { - void operator()() { - encoder->ConfigureEncoderOnTaskQueue(std::move(config), - max_data_payload_length); - } - VideoStreamEncoder* encoder; - VideoEncoderConfig config; - size_t max_data_payload_length; - }; encoder_queue_.PostTask( - ConfigureEncoderTask{this, std::move(config), max_data_payload_length}); -} + [this, config = std::move(config), max_data_payload_length]() mutable { + RTC_DCHECK_RUN_ON(&encoder_queue_); + RTC_DCHECK(sink_); + RTC_LOG(LS_INFO) << "ConfigureEncoder requested."; -void VideoStreamEncoder::ConfigureEncoderOnTaskQueue( - VideoEncoderConfig config, - size_t max_data_payload_length) { - RTC_DCHECK_RUN_ON(&encoder_queue_); - RTC_DCHECK(sink_); - RTC_LOG(LS_INFO) << "ConfigureEncoder requested."; + pending_encoder_creation_ = + (!encoder_ || encoder_config_.video_format != config.video_format || + max_data_payload_length_ != max_data_payload_length); + encoder_config_ = std::move(config); + max_data_payload_length_ = max_data_payload_length; + pending_encoder_reconfiguration_ = true; - pending_encoder_creation_ = - (!encoder_ || encoder_config_.video_format != config.video_format || - max_data_payload_length_ != max_data_payload_length); - encoder_config_ = std::move(config); - max_data_payload_length_ = max_data_payload_length; - pending_encoder_reconfiguration_ = true; - - // Reconfigure the encoder now if the encoder has an internal source or - // if the frame resolution is known. Otherwise, the reconfiguration is - // deferred until the next frame to minimize the number of reconfigurations. - // The codec configuration depends on incoming video frame size. - if (last_frame_info_) { - ReconfigureEncoder(); - } else { - codec_info_ = settings_.encoder_factory->QueryVideoEncoder( - encoder_config_.video_format); - if (HasInternalSource()) { - last_frame_info_ = VideoFrameInfo(176, 144, false); - ReconfigureEncoder(); - } - } + // Reconfigure the encoder now if the encoder has an internal source or + // if the frame resolution is known. Otherwise, the reconfiguration is + // deferred until the next frame to minimize the number of + // reconfigurations. The codec configuration depends on incoming video + // frame size. + if (last_frame_info_) { + ReconfigureEncoder(); + } else { + codec_info_ = settings_.encoder_factory->QueryVideoEncoder( + encoder_config_.video_format); + if (HasInternalSource()) { + last_frame_info_ = VideoFrameInfo(176, 144, false); + ReconfigureEncoder(); + } + } + }); } static absl::optional diff --git a/video/video_stream_encoder.h b/video/video_stream_encoder.h index 46df362ebd..ba9f519475 100644 --- a/video/video_stream_encoder.h +++ b/video/video_stream_encoder.h @@ -142,8 +142,6 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, DataRate stable_encoder_target; }; - void ConfigureEncoderOnTaskQueue(VideoEncoderConfig config, - size_t max_data_payload_length); void ReconfigureEncoder() RTC_RUN_ON(&encoder_queue_); void ConfigureQualityScaler(const VideoEncoder::EncoderInfo& encoder_info);