From 63299a3124dd6cd37e887ee274fec3716bb0d27b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Terelius?= Date: Tue, 5 Jul 2022 10:58:52 +0200 Subject: [PATCH] Add absl::string_view overload for RtcEventLogOutput::Write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:13579 Change-Id: I13f63fb6be6aa62c2e011c18327499fa16b5824e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/267641 Commit-Queue: Björn Terelius Reviewed-by: Harald Alvestrand Reviewed-by: Ali Tofigh Cr-Commit-Position: refs/heads/main@{#37440} --- api/BUILD.gn | 1 + api/rtc_event_log_output.h | 7 +++++++ api/rtc_event_log_output_file.cc | 10 +++++++--- api/rtc_event_log_output_file.h | 1 + api/rtc_event_log_output_file_unittest.cc | 20 +++++++++---------- .../goog_cc/test/goog_cc_printer.cc | 8 ++++---- pc/peer_connection_integrationtest.cc | 6 ++++-- pc/peer_connection_interface_unittest.cc | 3 ++- pc/test/integration_test_helpers.h | 1 + test/logging/file_log_writer.cc | 4 ++++ test/logging/file_log_writer.h | 1 + test/logging/memory_log_writer.cc | 4 ++++ test/scenario/column_printer.cc | 8 ++++---- test/scenario/stats_collection.cc | 4 ++-- 14 files changed, 52 insertions(+), 26 deletions(-) diff --git a/api/BUILD.gn b/api/BUILD.gn index bb2c150a44..72a5eb84ff 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -659,6 +659,7 @@ rtc_library("create_peer_connection_quality_test_frame_generator") { rtc_source_set("libjingle_logging_api") { visibility = [ "*" ] sources = [ "rtc_event_log_output.h" ] + absl_deps = [ "//third_party/abseil-cpp/absl/strings:strings" ] } rtc_library("rtc_event_log_output_file") { diff --git a/api/rtc_event_log_output.h b/api/rtc_event_log_output.h index cd16b27501..69951893c6 100644 --- a/api/rtc_event_log_output.h +++ b/api/rtc_event_log_output.h @@ -13,6 +13,8 @@ #include +#include "absl/strings/string_view.h" + namespace webrtc { // NOTE: This class is still under development and may change without notice. @@ -32,6 +34,11 @@ class RtcEventLogOutput { // after the first time `false` is returned. Write() may not be called on // an inactive output sink. virtual bool Write(const std::string& output) = 0; + // TODO(bugs.webrtc.org/13579): Make pure virtual and remove the string ref + // once all classes implement the string_view version. + virtual bool Write(absl::string_view output) { + return Write(std::string(output)); + } // Indicates that buffers should be written to disk if applicable. virtual void Flush() {} diff --git a/api/rtc_event_log_output_file.cc b/api/rtc_event_log_output_file.cc index 2e31c2df66..2ba3e021ba 100644 --- a/api/rtc_event_log_output_file.cc +++ b/api/rtc_event_log_output_file.cc @@ -55,14 +55,18 @@ bool RtcEventLogOutputFile::IsActive() const { } bool RtcEventLogOutputFile::Write(const std::string& output) { + return Write(absl::string_view(output)); +} + +bool RtcEventLogOutputFile::Write(absl::string_view output) { RTC_DCHECK(IsActiveInternal()); // No single write may be so big, that it would risk overflowing the // calculation of (written_bytes_ + output.length()). - RTC_DCHECK_LT(output.length(), kMaxReasonableFileSize); + RTC_DCHECK_LT(output.size(), kMaxReasonableFileSize); if (max_size_bytes_ == RtcEventLog::kUnlimitedOutput || - written_bytes_ + output.length() <= max_size_bytes_) { - if (file_.Write(output.c_str(), output.size())) { + written_bytes_ + output.size() <= max_size_bytes_) { + if (file_.Write(output.data(), output.size())) { written_bytes_ += output.size(); return true; } else { diff --git a/api/rtc_event_log_output_file.h b/api/rtc_event_log_output_file.h index d2901be1d0..1f459bd3cd 100644 --- a/api/rtc_event_log_output_file.h +++ b/api/rtc_event_log_output_file.h @@ -38,6 +38,7 @@ class RtcEventLogOutputFile final : public RtcEventLogOutput { bool IsActive() const override; bool Write(const std::string& output) override; + bool Write(absl::string_view output) override; private: RtcEventLogOutputFile(FileWrapper file, size_t max_size_bytes); diff --git a/api/rtc_event_log_output_file_unittest.cc b/api/rtc_event_log_output_file_unittest.cc index 4274215491..ef5c3e6c21 100644 --- a/api/rtc_event_log_output_file_unittest.cc +++ b/api/rtc_event_log_output_file_unittest.cc @@ -72,15 +72,15 @@ TEST_F(RtcEventLogOutputFileTest, UnlimitedOutputFile) { EXPECT_EQ(GetOutputFileContents(), output_str); } -// Do not allow writing more bytes to the file than +// Do not allow writing more bytes to the file than max file size. TEST_F(RtcEventLogOutputFileTest, LimitedOutputFileCappedToCapacity) { // Fit two bytes, then the third should be rejected. auto output_file = std::make_unique(output_file_name_, 2); - output_file->Write("1"); - output_file->Write("2"); - output_file->Write("3"); + output_file->Write(absl::string_view("1")); + output_file->Write(absl::string_view("2")); + output_file->Write(absl::string_view("3")); // Unsuccessful writes close the file; no need to delete the output to flush. EXPECT_EQ(GetOutputFileContents(), "12"); @@ -108,20 +108,20 @@ TEST_F(RtcEventLogOutputFileTest, DoNotWritePartialLines) { TEST_F(RtcEventLogOutputFileTest, UnsuccessfulWriteReturnsFalse) { auto output_file = std::make_unique(output_file_name_, 2); - EXPECT_FALSE(output_file->Write("abc")); + EXPECT_FALSE(output_file->Write(absl::string_view("abc"))); } TEST_F(RtcEventLogOutputFileTest, SuccessfulWriteReturnsTrue) { auto output_file = std::make_unique(output_file_name_, 3); - EXPECT_TRUE(output_file->Write("abc")); + EXPECT_TRUE(output_file->Write(absl::string_view("abc"))); } // Even if capacity is reached, a successful write leaves the output active. TEST_F(RtcEventLogOutputFileTest, FileStillActiveAfterSuccessfulWrite) { auto output_file = std::make_unique(output_file_name_, 3); - ASSERT_TRUE(output_file->Write("abc")); + ASSERT_TRUE(output_file->Write(absl::string_view("abc"))); EXPECT_TRUE(output_file->IsActive()); } @@ -130,7 +130,7 @@ TEST_F(RtcEventLogOutputFileTest, FileStillActiveAfterSuccessfulWrite) { TEST_F(RtcEventLogOutputFileTest, FileInactiveAfterUnsuccessfulWrite) { auto output_file = std::make_unique(output_file_name_, 2); - ASSERT_FALSE(output_file->Write("abc")); + ASSERT_FALSE(output_file->Write(absl::string_view("abc"))); EXPECT_FALSE(output_file->IsActive()); } @@ -145,9 +145,9 @@ class RtcEventLogOutputFileDeathTest : public RtcEventLogOutputFileTest {}; TEST_F(RtcEventLogOutputFileDeathTest, WritingToInactiveFileForbidden) { RtcEventLogOutputFile output_file(output_file_name_, 2); - ASSERT_FALSE(output_file.Write("abc")); + ASSERT_FALSE(output_file.Write(absl::string_view("abc"))); ASSERT_FALSE(output_file.IsActive()); - EXPECT_DEATH(output_file.Write("abc"), ""); + EXPECT_DEATH(output_file.Write(absl::string_view("abc")), ""); } TEST_F(RtcEventLogOutputFileDeathTest, DisallowUnreasonableFileSizeLimits) { diff --git a/modules/congestion_controller/goog_cc/test/goog_cc_printer.cc b/modules/congestion_controller/goog_cc/test/goog_cc_printer.cc index 6dadf8b9c4..bcf3aca4cd 100644 --- a/modules/congestion_controller/goog_cc/test/goog_cc_printer.cc +++ b/modules/congestion_controller/goog_cc/test/goog_cc_printer.cc @@ -136,10 +136,10 @@ void GoogCcStatePrinter::PrintHeaders(RtcEventLogOutput* log) { int ix = 0; for (const auto& logger : loggers_) { if (ix++) - log->Write(" "); + log->Write(absl::string_view(" ")); log->Write(logger->name()); } - log->Write("\n"); + log->Write(absl::string_view("\n")); log->Flush(); } @@ -160,11 +160,11 @@ void GoogCcStatePrinter::PrintState(RtcEventLogOutput* log, int ix = 0; for (const auto& logger : loggers_) { if (ix++) - log->Write(" "); + log->Write(absl::string_view(" ")); logger->WriteValue(log); } - log->Write("\n"); + log->Write(absl::string_view("\n")); log->Flush(); } diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 19db051629..da9244cd53 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -2810,8 +2810,10 @@ TEST_P(PeerConnectionIntegrationTest, RtcEventLogOutputWriteCalled) { auto output = std::make_unique>(); ON_CALL(*output, IsActive()).WillByDefault(::testing::Return(true)); - ON_CALL(*output, Write(::testing::_)).WillByDefault(::testing::Return(true)); - EXPECT_CALL(*output, Write(::testing::_)).Times(::testing::AtLeast(1)); + ON_CALL(*output, Write(::testing::A())) + .WillByDefault(::testing::Return(true)); + EXPECT_CALL(*output, Write(::testing::A())) + .Times(::testing::AtLeast(1)); EXPECT_TRUE(caller()->pc()->StartRtcEventLog( std::move(output), webrtc::RtcEventLog::kImmediateOutput)); diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc index f195b784c9..2991fac588 100644 --- a/pc/peer_connection_interface_unittest.cc +++ b/pc/peer_connection_interface_unittest.cc @@ -437,7 +437,8 @@ static const char kDtlsSdesFallbackSdp[] = class RtcEventLogOutputNull final : public RtcEventLogOutput { public: bool IsActive() const override { return true; } - bool Write(const std::string& output) override { return true; } + bool Write(const std::string& /*output*/) override { return true; } + bool Write(const absl::string_view /*output*/) override { return true; } }; using ::cricket::StreamParams; diff --git a/pc/test/integration_test_helpers.h b/pc/test/integration_test_helpers.h index 460148c559..3b53ece9b3 100644 --- a/pc/test/integration_test_helpers.h +++ b/pc/test/integration_test_helpers.h @@ -1251,6 +1251,7 @@ class MockRtcEventLogOutput : public webrtc::RtcEventLogOutput { virtual ~MockRtcEventLogOutput() = default; MOCK_METHOD(bool, IsActive, (), (const, override)); MOCK_METHOD(bool, Write, (const std::string&), (override)); + MOCK_METHOD(bool, Write, (absl::string_view), (override)); }; // This helper object is used for both specifying how many audio/video frames diff --git a/test/logging/file_log_writer.cc b/test/logging/file_log_writer.cc index 150f17344d..106143a3fb 100644 --- a/test/logging/file_log_writer.cc +++ b/test/logging/file_log_writer.cc @@ -33,6 +33,10 @@ bool FileLogWriter::IsActive() const { } bool FileLogWriter::Write(const std::string& value) { + return Write(absl::string_view(value)); +} + +bool FileLogWriter::Write(absl::string_view value) { // We don't expect the write to fail. If it does, we don't want to risk // silently ignoring it. RTC_CHECK_EQ(std::fwrite(value.data(), 1, value.size(), out_), value.size()) diff --git a/test/logging/file_log_writer.h b/test/logging/file_log_writer.h index e3c0cf6713..b0dbf03b15 100644 --- a/test/logging/file_log_writer.h +++ b/test/logging/file_log_writer.h @@ -25,6 +25,7 @@ class FileLogWriter final : public RtcEventLogOutput { ~FileLogWriter() final; bool IsActive() const override; bool Write(const std::string& value) override; + bool Write(absl::string_view value) override; void Flush() override; private: diff --git a/test/logging/memory_log_writer.cc b/test/logging/memory_log_writer.cc index 56e8e3e0db..4a5e536059 100644 --- a/test/logging/memory_log_writer.cc +++ b/test/logging/memory_log_writer.cc @@ -27,6 +27,10 @@ class MemoryLogWriter final : public RtcEventLogOutput { buffer_.append(value); return true; } + bool Write(absl::string_view value) override { + buffer_.append(value.data(), value.size()); + return true; + } void Flush() override {} private: diff --git a/test/scenario/column_printer.cc b/test/scenario/column_printer.cc index 661c83bd0d..87dbe0b89c 100644 --- a/test/scenario/column_printer.cc +++ b/test/scenario/column_printer.cc @@ -48,12 +48,12 @@ StatesPrinter::~StatesPrinter() = default; void StatesPrinter::PrintHeaders() { if (!writer_) return; - writer_->Write(printers_[0].headers_); + writer_->Write(absl::string_view(printers_[0].headers_)); for (size_t i = 1; i < printers_.size(); ++i) { - writer_->Write(" "); - writer_->Write(printers_[i].headers_); + writer_->Write(absl::string_view(" ")); + writer_->Write(absl::string_view(printers_[i].headers_)); } - writer_->Write("\n"); + writer_->Write(absl::string_view("\n")); } void StatesPrinter::PrintRow() { diff --git a/test/scenario/stats_collection.cc b/test/scenario/stats_collection.cc index e32696de71..5c81e3dd12 100644 --- a/test/scenario/stats_collection.cc +++ b/test/scenario/stats_collection.cc @@ -29,9 +29,9 @@ VideoQualityAnalyzer::VideoQualityAnalyzer( VideoQualityAnalyzer::~VideoQualityAnalyzer() = default; void VideoQualityAnalyzer::PrintHeaders() { - writer_->Write( + writer_->Write(absl::string_view( "capture_time render_time capture_width capture_height render_width " - "render_height psnr\n"); + "render_height psnr\n")); } std::function VideoQualityAnalyzer::Handler() {