Make RtcEventLogImpl to use a TaskQueue instead of a helper-thread

webrtc/logging/rtc_event_log/rtc_event_log.cc

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/logging/rtc_event_log/rtc_event_log.h"
 
+#include <atomic>
+#include <deque>
+#include <functional>
 #include <limits>
+#include <memory>
 #include <utility>
 #include <vector>
 
-#include "webrtc/logging/rtc_event_log/rtc_event_log_helper_thread.h"
 #include "webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h"
 #include "webrtc/modules/remote_bitrate_estimator/include/bwe_defines.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "webrtc/modules/rtp_rtcp/source/byte_io.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/app.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/common_header.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_jitter_report.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/psfb.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/rtpfb.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.h"
-#include "webrtc/rtc_base/atomicops.h"
 #include "webrtc/rtc_base/checks.h"
 #include "webrtc/rtc_base/constructormagic.h"
 #include "webrtc/rtc_base/event.h"
+#include "webrtc/rtc_base/ignore_wundef.h"
 #include "webrtc/rtc_base/logging.h"
 #include "webrtc/rtc_base/protobuf_utils.h"
-#include "webrtc/rtc_base/swap_queue.h"
-#include "webrtc/rtc_base/thread_checker.h"
+#include "webrtc/rtc_base/ptr_util.h"
+#include "webrtc/rtc_base/sequenced_task_checker.h"
+#include "webrtc/rtc_base/task_queue.h"
+#include "webrtc/rtc_base/thread_annotations.h"
 #include "webrtc/rtc_base/timeutils.h"
 #include "webrtc/system_wrappers/include/file_wrapper.h"
+#include "webrtc/typedefs.h"
 
 #ifdef ENABLE_RTC_EVENT_LOG
 // *.pb.h files are generated at build-time by the protobuf compiler.
+RTC_PUSH_IGNORING_WUNDEF()
 #ifdef WEBRTC_ANDROID_PLATFORM_BUILD
 #include "external/webrtc/webrtc/logging/rtc_event_log/rtc_event_log.pb.h"
 #else
 #include "webrtc/logging/rtc_event_log/rtc_event_log.pb.h"
 #endif
+RTC_POP_IGNORING_WUNDEF()
 #endif
 
 namespace webrtc {
 
 #ifdef ENABLE_RTC_EVENT_LOG
 
+namespace {
+const int kEventsInHistory = 10000;
+
+bool IsConfigEvent(const rtclog::Event& event) {
+  rtclog::Event_EventType event_type = event.type();
+  return event_type == rtclog::Event::VIDEO_RECEIVER_CONFIG_EVENT ||
+         event_type == rtclog::Event::VIDEO_SENDER_CONFIG_EVENT ||
+         event_type == rtclog::Event::AUDIO_RECEIVER_CONFIG_EVENT ||
+         event_type == rtclog::Event::AUDIO_SENDER_CONFIG_EVENT;
+}
+
+// This class exists because C++11 doesn't allow transferring a unique_ptr to
+// a lambda (a copy constructor is required).
+class RtcEventLogTask final : public rtc::QueuedTask {

[nisse-webrtc, 2017/08/30 12:18:35]

You could templateize this and the below class, on the type the unique_ptr
refers to.

Not sure if it would improve readability, but in case the type can be inferred
automatically when it is used, it might be worth a try.

[eladalon, 2017/08/30 13:01:18]

Good suggestion - I think it would actually, oddly enough for a template,
improve readability, because it would disconnect the actual type (file/event)
from the fact that this class just exists to manage its lifetime.
Done.

+ public:
+  RtcEventLogTask(std::unique_ptr<rtclog::Event> event,
+                  std::function<void(std::unique_ptr<rtclog::Event>)> handler)
+      : event_(std::move(event)), handler_(handler) {}
+
+  bool Run() override {
+    handler_(std::move(event_));
+    return true;
+  }
+
+ private:
+  std::unique_ptr<rtclog::Event> event_;
+  std::function<void(std::unique_ptr<rtclog::Event>)> handler_;
+};
+
+// This class exists because C++11 doesn't allow transferring a unique_ptr to
+// a lambda (a copy constructor is required).
+class RtcEventLogFileTask final : public rtc::QueuedTask {
+ public:
+  RtcEventLogFileTask(std::unique_ptr<FileWrapper> file,
+                      std::function<void(std::unique_ptr<FileWrapper>)> handler)
+      : file_(std::move(file)),
+        handler_(handler) {}
+
+  bool Run() override {
+    handler_(std::move(file_));
+    return true;
+  }
+
+ private:
+  std::unique_ptr<FileWrapper> file_;
+  std::function<void(std::unique_ptr<FileWrapper>)> handler_;
+};
+}  // namespace
+
 class RtcEventLogImpl final : public RtcEventLog {
   friend std::unique_ptr<RtcEventLog> RtcEventLog::Create();
 
  public:
   ~RtcEventLogImpl() override;
 
   bool StartLogging(const std::string& file_name,
                     int64_t max_size_bytes) override;
   bool StartLogging(rtc::PlatformFile platform_file,
                     int64_t max_size_bytes) override;
@@ skipping 22 matching lines @@
       const AudioEncoderRuntimeConfig& config) override;
   void LogProbeClusterCreated(int id,
                               int bitrate_bps,
                               int min_probes,
                               int min_bytes) override;
   void LogProbeResultSuccess(int id, int bitrate_bps) override;
   void LogProbeResultFailure(int id,
                              ProbeFailureReason failure_reason) override;
 
  private:
-  // Private constructor to ensure that creation is done by RtcEventLog::Create.
-  RtcEventLogImpl();
+  void StartLoggingInternal(std::unique_ptr<FileWrapper> file,
+                            int64_t max_size_bytes);
+
+  RtcEventLogImpl();  // Creation is done by RtcEventLog::Create.
 
   void StoreEvent(std::unique_ptr<rtclog::Event> event);
   void LogProbeResult(int id,
                       rtclog::BweProbeResult::ResultType result,
                       int bitrate_bps);
 
-  static volatile int log_count_;
+  // Attempts to append an event to the output protobuf string, while observing
+  // the (potential) limitation on file size. (False is returned when observing
+  // the limitation disallows appending the event.)

[nisse-webrtc, 2017/08/30 12:18:35]

Suggested rephrasing of this comment:

Appends an event to the output protobuf string, returning true on
success. Fails and returns false in case the limit out output size
prevents the event from being added; in this case, the output string
is left unchanged.

[eladalon, 2017/08/30 13:01:18]

Done.

+  bool AppendEventToString(rtclog::Event* event,
+                           ProtoString* output_string) RTC_WARN_UNUSED_RESULT;

[nisse-webrtc, 2017/08/30 12:18:36]

How do you expect callers to handle errors? 

If there's nothing useful they can do, you shouldn't use RTC_WARN_UNUSED_RESULT.

[eladalon, 2017/08/30 13:01:18]

This is not an error, this is an indication that an event was not written due to
lack of space. Before, some call-sites of this function checked the return value
and stopped on the first failure, but others failed to check the return-code,
which could result in skipped events in the log. That is, given [event0, event1,
event2], the log-file could end up containing only event0 and event2, because
event1 happened to not fit, but event2 did. I've fixed that, and added this
annotation, so as to reduce this likelihood of such bugs re-appearing.

[nisse-webrtc, 2017/08/30 13:13:23]

You could move that responsibility to this class, with a flag

  bool log_is_full_go_away_;

to ensure that if AppendEventToString ever returns false, it will continue to
return false until the end of time.

[eladalon, 2017/08/30 14:15:44]

It would indeed be better, because now we still have the problem that future
calls to StoreEvent() would not remember that a previous call has failed. But
this is a bit more of a departure from the original CL, and I've already
piggybacked too many changes on this CL, which was originally only intended to
migrate from a helper-thread to a task queue. So I would prefer to handle this
in a later CL. Wdyt?

[nisse-webrtc, 2017/08/30 14:27:01]

It's just that I don't think RTC_WARN_UNUSED_RESULT should be used lightly. But
I'll defer to Björn about division of work into cls.

[terelius, 2017/08/30 15:06:04]

Nisse, I think it is fine to land this CL as-is. There are several
implementation changes planned for the future and we'll keep your objection in
mind for the coming CLs.

Elad, I don't understand your comment about StoreEvent. This function is not
used by StoreEvent. More importantly, the log stops if AppendEventToString ever
returns false.

[eladalon, 2017/08/30 15:19:20]

1. StoreEvent -> LogToFile -> AppendEventToString
2. Yes, you're right - the file is closed when this fails.

 
-  // Message queue for passing control messages to the logging thread.
-  SwapQueue<RtcEventLogHelperThread::ControlMessage> message_queue_;
+  void LogToMemory(std::unique_ptr<rtclog::Event> event);
 
-  // Message queue for passing events to the logging thread.
-  SwapQueue<std::unique_ptr<rtclog::Event> > event_queue_;
+  void StartLogFile();
+  void LogToFile(std::unique_ptr<rtclog::Event> event);
+  void StopLogFile(int64_t stop_time);
 
-  RtcEventLogHelperThread helper_thread_;
-  rtc::ThreadChecker thread_checker_;
+  // TODO(eladalon): Known issue - there's a race over |log_count_|.
+  static std::atomic<int> log_count_;

[nisse-webrtc, 2017/08/30 12:18:35]

I'm not sure use of std::atomic is allowed yet. 

And I think log_count_ deserves a comment on its purpose.

[eladalon, 2017/08/30 13:01:18]

1. I think it's allowed. I'll send you a relevant link OOB.
2. I'll add the comment.

[nisse-webrtc, 2017/08/30 13:13:23]

I've checked the Chromium style guide, it's allowed according to
https://chromium-cpp.appspot.com/. So this is fine.

+
+  rtc::SequencedTaskChecker worker_sequence_checker_;
+  rtc::SequencedTaskChecker owner_sequence_checker_;
+
+  // History containing all past configuration events.
+  std::vector<std::unique_ptr<rtclog::Event>> config_history_
+      GUARDED_BY(worker_sequence_checker_);
+
+  // History containing the most recent (non-configuration) events (~ 10 s).
+  std::deque<std::unique_ptr<rtclog::Event>> history_
+      GUARDED_BY(worker_sequence_checker_);
+
+  std::unique_ptr<FileWrapper> file_ GUARDED_BY(worker_sequence_checker_);
+
+  size_t max_size_bytes_ GUARDED_BY(worker_sequence_checker_);
+  size_t written_bytes_ GUARDED_BY(worker_sequence_checker_);
+
+  // Keep this last to ensure it destructs first, or else tasks living on the
+  // queue might access other members after they've been torn down.
+  rtc::TaskQueue task_queue_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(RtcEventLogImpl);
 };
 
 namespace {
 // The functions in this namespace convert enums from the runtime format
 // that the rest of the WebRtc project can use, to the corresponding
 // serialized enum which is defined by the protobuf.
 
 rtclog::VideoReceiveConfig_RtcpMode ConvertRtcpMode(RtcpMode rtcp_mode) {
@@ skipping 31 matching lines @@
       return rtclog::BweProbeResult::INVALID_SEND_RECEIVE_INTERVAL;
     case kInvalidSendReceiveRatio:
       return rtclog::BweProbeResult::INVALID_SEND_RECEIVE_RATIO;
     case kTimeout:
       return rtclog::BweProbeResult::TIMEOUT;
   }
   RTC_NOTREACHED();
   return rtclog::BweProbeResult::SUCCESS;
 }
 
-// The RTP and RTCP buffers reserve space for twice the expected number of
-// sent packets because they also contain received packets.
-static const int kEventsPerSecond = 1000;
-static const int kControlMessagesPerSecond = 10;
 }  // namespace
 
-volatile int RtcEventLogImpl::log_count_ = 0;
+std::atomic<int> RtcEventLogImpl::log_count_(0);
 
-// RtcEventLogImpl member functions.
 RtcEventLogImpl::RtcEventLogImpl()
-    // Allocate buffers for roughly one second of history.
-    : message_queue_(kControlMessagesPerSecond),
-      event_queue_(kEventsPerSecond),
-      helper_thread_(&message_queue_, &event_queue_),
-      thread_checker_() {
-  thread_checker_.DetachFromThread();
+    : file_(FileWrapper::Create()),
+      max_size_bytes_(std::numeric_limits<decltype(max_size_bytes_)>::max()),
+      written_bytes_(0),
+      task_queue_("rtc_event_log") {
+  worker_sequence_checker_.Detach();
 }
 
 RtcEventLogImpl::~RtcEventLogImpl() {
-  // The RtcEventLogHelperThread destructor closes the file
-  // and waits for the thread to terminate.
-  int count = rtc::AtomicOps::Decrement(&RtcEventLogImpl::log_count_);
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&owner_sequence_checker_);
+
+  // If we're logging to the file, this will stop that. Blocking function.
+  StopLogging();
+
+  int count = std::atomic_fetch_sub(&RtcEventLogImpl::log_count_, 1) - 1;
   RTC_DCHECK_GE(count, 0);
 }
 
 bool RtcEventLogImpl::StartLogging(const std::string& file_name,
                                    int64_t max_size_bytes) {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
-  RtcEventLogHelperThread::ControlMessage message;
-  message.message_type = RtcEventLogHelperThread::ControlMessage::START_FILE;
-  message.max_size_bytes = max_size_bytes <= 0
-                               ? std::numeric_limits<int64_t>::max()
-                               : max_size_bytes;
-  message.start_time = rtc::TimeMicros();
-  message.stop_time = std::numeric_limits<int64_t>::max();
-  message.file.reset(FileWrapper::Create());
-  if (!message.file->OpenFile(file_name.c_str(), false)) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&owner_sequence_checker_);
+
+  auto file = rtc::WrapUnique<FileWrapper>(FileWrapper::Create());
+  if (!file->OpenFile(file_name.c_str(), false)) {
     LOG(LS_ERROR) << "Can't open file. WebRTC event log not started.";
     return false;
   }
-  if (!message_queue_.Insert(&message)) {
-    LOG(LS_ERROR) << "Message queue full. Can't start logging.";
-    return false;
-  }
-  helper_thread_.SignalNewEvent();
-  LOG(LS_INFO) << "Starting WebRTC event log.";
+
+  StartLoggingInternal(std::move(file), max_size_bytes);
+
   return true;
 }
 
 bool RtcEventLogImpl::StartLogging(rtc::PlatformFile platform_file,
                                    int64_t max_size_bytes) {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
-  RtcEventLogHelperThread::ControlMessage message;
-  message.message_type = RtcEventLogHelperThread::ControlMessage::START_FILE;
-  message.max_size_bytes = max_size_bytes <= 0
-                               ? std::numeric_limits<int64_t>::max()
-                               : max_size_bytes;
-  message.start_time = rtc::TimeMicros();
-  message.stop_time = std::numeric_limits<int64_t>::max();
-  message.file.reset(FileWrapper::Create());
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&owner_sequence_checker_);
+
+  auto file = rtc::WrapUnique<FileWrapper>(FileWrapper::Create());
   FILE* file_handle = rtc::FdopenPlatformFileForWriting(platform_file);
   if (!file_handle) {
     LOG(LS_ERROR) << "Can't open file. WebRTC event log not started.";
     // Even though we failed to open a FILE*, the platform_file is still open
     // and needs to be closed.
     if (!rtc::ClosePlatformFile(platform_file)) {
       LOG(LS_ERROR) << "Can't close file.";
     }
     return false;
   }
-  if (!message.file->OpenFromFileHandle(file_handle)) {
+  if (!file->OpenFromFileHandle(file_handle)) {
     LOG(LS_ERROR) << "Can't open file. WebRTC event log not started.";
     return false;
   }
-  if (!message_queue_.Insert(&message)) {
-    LOG(LS_ERROR) << "Message queue full. Can't start logging.";
-    return false;
-  }
-  helper_thread_.SignalNewEvent();
-  LOG(LS_INFO) << "Starting WebRTC event log.";
+
+  StartLoggingInternal(std::move(file), max_size_bytes);
+
   return true;
 }
 
 void RtcEventLogImpl::StopLogging() {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
-  RtcEventLogHelperThread::ControlMessage message;
-  message.message_type = RtcEventLogHelperThread::ControlMessage::STOP_FILE;
-  message.stop_time = rtc::TimeMicros();
-  while (!message_queue_.Insert(&message)) {
-    // TODO(terelius): We would like to have a blocking Insert function in the
-    // SwapQueue, but for the time being we will just clear any previous
-    // messages.
-    // Since StopLogging waits for the thread, it is essential that we don't
-    // clear any STOP_FILE messages. To ensure that there is only one call at a
-    // time, we require that all calls to StopLogging are made on the same
-    // thread.
-    LOG(LS_ERROR) << "Message queue full. Clearing queue to stop logging.";
-    message_queue_.Clear();
-  }
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&owner_sequence_checker_);
+
   LOG(LS_INFO) << "Stopping WebRTC event log.";
-  helper_thread_.WaitForFileFinished();
+
+  const int64_t stop_time = rtc::TimeMicros();
+
+  task_queue_.PostTask([this, stop_time]() {
+    RTC_DCHECK_CALLED_SEQUENTIALLY(&worker_sequence_checker_);
+    if (file_->is_open()) {
+      StopLogFile(stop_time);
+    }
+  });
+
+  // Wait for an indication that the previous task, which stops the log file,
+  // has been executed to completion.
+  rtc::Event file_finished(true, false);
+  task_queue_.PostTask([this, &file_finished]() {
+    RTC_DCHECK_CALLED_SEQUENTIALLY(&worker_sequence_checker_);
+    file_finished.Set();
+  });
+  file_finished.Wait(rtc::Event::kForever);
+
+  LOG(LS_INFO) << "WebRTC event log successfully stopped.";
 }
 
 void RtcEventLogImpl::LogVideoReceiveStreamConfig(
     const rtclog::StreamConfig& config) {
   std::unique_ptr<rtclog::Event> event(new rtclog::Event());
   event->set_timestamp_us(rtc::TimeMicros());
   event->set_type(rtclog::Event::VIDEO_RECEIVER_CONFIG_EVENT);
 
   rtclog::VideoReceiveConfig* receiver_config =
       event->mutable_video_receiver_config();
@@ skipping 280 matching lines @@
   event->set_type(rtclog::Event::BWE_PROBE_RESULT_EVENT);
 
   auto probe_result = event->mutable_probe_result();
   probe_result->set_id(id);
   probe_result->set_result(result);
   if (result == rtclog::BweProbeResult::SUCCESS)
     probe_result->set_bitrate_bps(bitrate_bps);
   StoreEvent(std::move(event));
 }
 
+void RtcEventLogImpl::StartLoggingInternal(std::unique_ptr<FileWrapper> file,
+                                           int64_t max_size_bytes) {
+  LOG(LS_INFO) << "Starting WebRTC event log.";
+
+  max_size_bytes = (max_size_bytes <= 0) ? std::numeric_limits<int64_t>::max()
+                                         : max_size_bytes;
+  auto file_handler = [this, max_size_bytes](
+      std::unique_ptr<FileWrapper> file) {
+    RTC_DCHECK_CALLED_SEQUENTIALLY(&worker_sequence_checker_);
+    if (!file_->is_open()) {
+      max_size_bytes_ = max_size_bytes;
+      file_= std::move(file);
+      StartLogFile();
+    } else {
+      // Already started. Ignore message and close file handle.
+      file->CloseFile();
+    }
+  };
+  auto task = rtc::WrapUnique<rtc::QueuedTask>(
+      new RtcEventLogFileTask(std::move(file), file_handler));
+  task_queue_.PostTask(std::move(task));
+}
+
 void RtcEventLogImpl::StoreEvent(std::unique_ptr<rtclog::Event> event) {
-  RTC_DCHECK(event.get() != nullptr);
-  if (!event_queue_.Insert(&event)) {
-    LOG(LS_ERROR) << "WebRTC event log queue full. Dropping event.";
+  RTC_DCHECK(event);
+
+  auto event_handler = [this](std::unique_ptr<rtclog::Event> rtclog_event) {
+    RTC_DCHECK_CALLED_SEQUENTIALLY(&worker_sequence_checker_);
+    if (file_->is_open()) {
+      LogToFile(std::move(rtclog_event));
+    } else {
+      LogToMemory(std::move(rtclog_event));
+    }
+  };
+
+  auto task = rtc::WrapUnique<rtc::QueuedTask>(
+      new RtcEventLogTask(std::move(event), event_handler));
+
+  task_queue_.PostTask(std::move(task));

[nisse-webrtc, 2017/08/30 12:18:35]

Now you set this up in three steps:

1. named lambda event_handler (btw, named lambda is an odd construction; C++
ought to let us declare a local function instead...)

2. Wrap the lambda in a RtcEventLogTask, with yet another local variable.

3. Pass it to PostTask.

I think it will be clearer to merge the second two, something like

task_queue_.PostTask(rtc::MakeUnique<RtcEventLogTask>(
   std::move(event), event_handler));

And I'd consider merging all three, inlining the definition of the lambda where
that is used. But I guess that won't suite everyone's taste.

[eladalon, 2017/08/30 13:01:19]

We have to have RtcEventLogTask (now named ResourceOwningTask), because a lambda
cannot capture a unique_ptr. We can't use a raw pointer, because if the task is
ever destructed without being executed, that would be a memory leak (but the
solution I have in place would not leak memory, because the destructor would
still be called and properly release memory).

As for naming lambdas, I personally like this, and use it often. I'd like to
keep doing that, at least unless there's a reason to suspect it might end up
with less efficient machine-code.

But speaking of efficiency - I used ResourceOwningTask::handler_ because I did
not want to make ResourceOwningTask a friend of RtcEventLogImpl. But maybe
that's too inefficient. I'll push the CL as it is, with these comments. We can
then think about modifying this more.

[nisse-webrtc, 2017/08/30 13:13:23]

ResourceOwningTask is fine, I was merely suggesting folding the expression to
construct it into the expression calling to PostTask.

[eladalon, 2017/08/30 14:15:44]

I see two issues:
1. The nit - that the lambda is named. I prefer this style.
2. That actual issue - that the lambda is another layer of indirection, which
could be avoided. I think that's preferable to the alternative of defining two
classes inside of RtcEventLogImpl and having both access RtcEventLogImpl's
internals.

[nisse-webrtc, 2017/08/30 14:27:01]

I'm not talking of either of those... I would like:

1. WrapUnique replaced by MakeUnique.

2. The local variable |task| eliminated, and it's value substituted where it is
used. 

[eladalon, 2017/08/30 15:19:20]

1. We can do that, because unique_ptr<Derived> is not implicitly convertible to
unique_ptr<Base>.
2. I think readability suffers if we do this, with no gain in efficiency to
justify this.

[nisse-webrtc, 2017/08/31 06:53:15]

I'd expect the unique_ptr to  support implicit conversion whenever the
underlying raw pointers do. Otherwise it's not a smart pointer, but a stupid
pointer...

From http://en.cppreference.com/w/cpp/memory/unique_ptr: "If T is a derived
class of some base B, then std::unique_ptr<T> is implicitly convertible to
std::unique_ptr<B>. "

WrapUnique is intended only for rare cases, like working with old raw-pointer
apis, or with private constructors. I think it's fairly important to not
introduce needlessly.
I disagree on readability, but I won't insist on this one.

[eladalon, 2017/08/31 09:05:49]

You're right, I skimmed through the compiler's error message too quickly -
std::unique_ptr converts implicitly from derived-class to base-class. The
problem was that PostTask(const Closure&) was a better match than
PostTask(std::unique_ptr<QueuedTask>) when posting a sub-class of QueuedTask.

+}
+
+bool RtcEventLogImpl::AppendEventToString(rtclog::Event* event,
+                                          ProtoString* output_string) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&worker_sequence_checker_);
+
+  // Even though we're only serializing a single event during this call, what
+  // we intend to get is a list of events, with a tag and length preceding
+  // each actual event. To produce that, we serialize a list of a single event.
+  // If we later serialize additional events, the resulting ProtoString will
+  // be a proper concatenation of all those events.
+
+  rtclog::EventStream event_stream;
+  event_stream.add_stream();
+
+  // As a tweak, we swap the new event into the event-stream, write that to
+  // file, then swap back. This saves on some copying.
+  rtclog::Event* output_event = event_stream.mutable_stream(0);
+  output_event->Swap(event);
+
+  bool appended;
+  size_t potential_new_size =
+      written_bytes_ + output_string->size() + event_stream.ByteSize();
+  if (potential_new_size <= max_size_bytes_) {
+    event_stream.AppendToString(output_string);
+    appended = true;
+  } else {
+    appended = false;
   }
-  helper_thread_.SignalNewEvent();
+
+  // When the function returns, the original Event will be unchanged.
+  output_event->Swap(event);
+
+  return appended;
+}
+
+void RtcEventLogImpl::LogToMemory(std::unique_ptr<rtclog::Event> event) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&worker_sequence_checker_);
+  RTC_DCHECK(!file_->is_open());
+
+  if (IsConfigEvent(*event.get())) {
+    config_history_.push_back(std::move(event));
+  } else {
+    history_.push_back(std::move(event));
+    if (history_.size() > kEventsInHistory) {
+      history_.pop_front();
+    }
+  }
+}
+
+void RtcEventLogImpl::StartLogFile() {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&worker_sequence_checker_);
+  RTC_DCHECK(file_->is_open());
+
+  ProtoString output_string;
+
+  // Create and serialize the LOG_START event.
+  // The timestamp used will correspond to when logging has started. The log
+  // may contain events earlier than the LOG_START event. (In general, the
+  // timestamps in the log are not monotonic.)
+  rtclog::Event start_event;
+  start_event.set_timestamp_us(rtc::TimeMicros());
+  start_event.set_type(rtclog::Event::LOG_START);
+  bool appended = AppendEventToString(&start_event, &output_string);
+
+  // Serialize the config information for all old streams, including streams
+  // which were already logged to previous files.
+  for (auto& event : config_history_) {
+    if (!appended) {
+      break;
+    }
+    appended = AppendEventToString(event.get(), &output_string);
+  }
+
+  // Serialize the events in the event queue.
+  while (appended && !history_.empty()) {
+    appended = AppendEventToString(history_.front().get(), &output_string);
+    if (appended) {
+      // Known issue - if writing to the file fails, these events will have
+      // been lost. If we try to open a new file, these events will be missing
+      // from it.
+      history_.pop_front();
+    }
+  }
+
+  // Write to file.
+  if (!file_->Write(output_string.data(), output_string.size())) {
+    LOG(LS_ERROR) << "FileWrapper failed to write WebRtcEventLog file.";
+    // The current FileWrapper implementation closes the file on error.
+    RTC_DCHECK(!file_->is_open());
+    return;
+  }
+  written_bytes_ += output_string.size();
+
+  if (!appended) {
+    RTC_DCHECK(file_->is_open());
+    StopLogFile(rtc::TimeMicros());
+  }
+}
+
+void RtcEventLogImpl::LogToFile(std::unique_ptr<rtclog::Event> event) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&worker_sequence_checker_);
+  RTC_DCHECK(file_->is_open());
+
+  ProtoString output_string;
+
+  bool appended = AppendEventToString(event.get(), &output_string);
+
+  if (IsConfigEvent(*event.get())) {
+    config_history_.push_back(std::move(event));
+  }
+
+  if (!appended) {

[terelius, 2017/08/30 15:06:04]

This is nitpicking, but you are losing the event for future logs if it doesn't
fit in the file.

[eladalon, 2017/08/30 15:19:20]

Done.

+    RTC_DCHECK(file_->is_open());
+    StopLogFile(rtc::TimeMicros());
+    return;
+  }
+
+  // Write string to file.
+  if (file_->Write(output_string.data(), output_string.size())) {
+    written_bytes_ += output_string.size();
+  } else {
+    LOG(LS_ERROR) << "FileWrapper failed to write WebRtcEventLog file.";
+    // The current FileWrapper implementation closes the file on error.
+    RTC_DCHECK(!file_->is_open());
+  }
+}
+
+void RtcEventLogImpl::StopLogFile(int64_t stop_time) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&worker_sequence_checker_);
+  RTC_DCHECK(file_->is_open());
+
+  ProtoString output_string;
+
+  rtclog::Event end_event;
+  end_event.set_timestamp_us(stop_time);
+  end_event.set_type(rtclog::Event::LOG_END);
+  bool appended = AppendEventToString(&end_event, &output_string);
+
+  if (appended) {
+    if (!file_->Write(output_string.data(), output_string.size())) {
+      LOG(LS_ERROR) << "FileWrapper failed to write WebRtcEventLog file.";
+      // The current FileWrapper implementation closes the file on error.
+      RTC_DCHECK(!file_->is_open());
+    }
+    written_bytes_ += output_string.size();
+  }
+
+  max_size_bytes_ = std::numeric_limits<int64_t>::max();
+  written_bytes_ = 0;
+
+  file_->CloseFile();
+  RTC_DCHECK(!file_->is_open());
 }
 
 bool RtcEventLog::ParseRtcEventLog(const std::string& file_name,
                                    rtclog::EventStream* result) {
   char tmp_buffer[1024];
   int bytes_read = 0;
   std::unique_ptr<FileWrapper> dump_file(FileWrapper::Create());
   if (!dump_file->OpenFile(file_name.c_str(), true)) {
     return false;
   }
   ProtoString dump_buffer;
   while ((bytes_read = dump_file->Read(tmp_buffer, sizeof(tmp_buffer))) > 0) {
     dump_buffer.append(tmp_buffer, bytes_read);
   }
   dump_file->CloseFile();
   return result->ParseFromString(dump_buffer);
 }
 
 #endif  // ENABLE_RTC_EVENT_LOG
 
 // RtcEventLog member functions.
 std::unique_ptr<RtcEventLog> RtcEventLog::Create() {
 #ifdef ENABLE_RTC_EVENT_LOG
+  // TODO(eladalon): Known issue - there's a race over |log_count_| here.
   constexpr int kMaxLogCount = 5;
-  int count = rtc::AtomicOps::Increment(&RtcEventLogImpl::log_count_);
+  int count = 1 + std::atomic_fetch_add(&RtcEventLogImpl::log_count_, 1);
   if (count > kMaxLogCount) {
     LOG(LS_WARNING) << "Denied creation of additional WebRTC event logs. "
                     << count - 1 << " logs open already.";
-    rtc::AtomicOps::Decrement(&RtcEventLogImpl::log_count_);
+    std::atomic_fetch_sub(&RtcEventLogImpl::log_count_, 1);
     return std::unique_ptr<RtcEventLog>(new RtcEventLogNullImpl());
   }
   return std::unique_ptr<RtcEventLog>(new RtcEventLogImpl());
 #else
-  return std::unique_ptr<RtcEventLog>(new RtcEventLogNullImpl());
+  return CreateNull();
 #endif  // ENABLE_RTC_EVENT_LOG
 }
 
 std::unique_ptr<RtcEventLog> RtcEventLog::CreateNull() {
   return std::unique_ptr<RtcEventLog>(new RtcEventLogNullImpl());
 }
 
 }  // namespace webrtc