Implements RTP header dumping.

chrome/browser/media/webrtc_rtp_dump_handler.cc

Index: chrome/browser/media/webrtc_rtp_dump_handler.cc
diff --git a/chrome/browser/media/webrtc_rtp_dump_handler.cc b/chrome/browser/media/webrtc_rtp_dump_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ee5f5308a1f6b731219beaf7127c03b57e886b00
--- /dev/null
+++ b/chrome/browser/media/webrtc_rtp_dump_handler.cc
@@ -0,0 +1,289 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/media/webrtc_rtp_dump_handler.h"
+
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/time/time.h"
+#include "chrome/browser/media/webrtc_rtp_dump_writer.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
+
+namespace {
+
+static const size_t kMaxOngoingRtpDumpsAllowed = 5;
+
+// The browser process wide total number of ongoing (i.e. started and not
+// released) RTP dumps. Incoming and outgoing in one WebRtcDumpHandler are
+// counted as one dump.
+// Must be accessed on the browser IO thread.
+static size_t g_ongoing_rtp_dumps = 0;
+
+void FireGenericDoneCallback(
+    const WebRtcRtpDumpHandler::GenericDoneCallback& callback,
+    bool success,
+    const std::string& error_message) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK(!callback.is_null());
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(callback, success, error_message));
+}
+
+bool DumpTypeContainsIncoming(RtpDumpType type) {
+  return type == RTP_DUMP_INCOMING || type == RTP_DUMP_BOTH;
+}
+
+bool DumpTypeContainsOutgoing(RtpDumpType type) {
+  return type == RTP_DUMP_OUTGOING || type == RTP_DUMP_BOTH;
+}
+
+}  // namespace
+
+WebRtcRtpDumpHandler::WebRtcRtpDumpHandler(const base::FilePath& dump_dir)
+    : dump_dir_(dump_dir),
+      incoming_state_(STATE_NONE),
+      outgoing_state_(STATE_NONE) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+}
+
+WebRtcRtpDumpHandler::~WebRtcRtpDumpHandler() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  // Reset dump writer first to stop writing.
+  if (dump_writer_) {
+    --g_ongoing_rtp_dumps;
+    dump_writer_.reset();
+  }
+
+  if (incoming_state_ != STATE_NONE && !incoming_dump_path_.empty()) {
+    BrowserThread::PostTask(
+        BrowserThread::FILE,
+        FROM_HERE,
+        base::Bind(
+            base::IgnoreResult(&base::DeleteFile), incoming_dump_path_, false));
+  }
+
+  if (outgoing_state_ != STATE_NONE && !outgoing_dump_path_.empty()) {
+    BrowserThread::PostTask(
+        BrowserThread::FILE,
+        FROM_HERE,
+        base::Bind(
+            base::IgnoreResult(&base::DeleteFile), outgoing_dump_path_, false));
+  }
+}
+
+bool WebRtcRtpDumpHandler::StartDump(RtpDumpType type,
+                                     std::string* error_message) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  if (!dump_writer_ && g_ongoing_rtp_dumps >= kMaxOngoingRtpDumpsAllowed) {
+    *error_message = "Max RTP dump limit reached.";
+    DVLOG(2) << *error_message;
+    return false;
+  }
+
+  // Returns an error if any type of dump specified by the caller cannot be
+  // started.
+  if ((DumpTypeContainsIncoming(type) && incoming_state_ != STATE_NONE) ||
+      (DumpTypeContainsOutgoing(type) && outgoing_state_ != STATE_NONE)) {
+    *error_message = "RTP dump already started.";
+    return false;
+  }
+
+  if (DumpTypeContainsIncoming(type))
+    incoming_state_ = STATE_STARTED;
+
+  if (DumpTypeContainsOutgoing(type))
+    outgoing_state_ = STATE_STARTED;
+
+  DVLOG(2) << "Start RTP dumping: type = " << type;
+
+  if (!dump_writer_) {
+    ++g_ongoing_rtp_dumps;
+
+    static const char kRecvDumpFilePrefix[] = "rtpdump_recv_";
+    static const char kSendDumpFilePrefix[] = "rtpdump_send_";
+    static const size_t kMaxDumpSize = 5 * 1024 * 1024;  // 5MB
+
+    std::string dump_id = base::DoubleToString(base::Time::Now().ToDoubleT());
+    incoming_dump_path_ =
+        dump_dir_.AppendASCII(std::string(kRecvDumpFilePrefix) + dump_id)
+            .AddExtension(FILE_PATH_LITERAL(".gz"));
+
+    outgoing_dump_path_ =
+        dump_dir_.AppendASCII(std::string(kSendDumpFilePrefix) + dump_id)
+            .AddExtension(FILE_PATH_LITERAL(".gz"));
+
+    // WebRtcRtpDumpWriter does not support changing the dump path after it's

[Henrik Grunell, 2014/05/23 12:17:51]

That can be a source of confusion in the future. I'd like the filename to
reflect the actual start timestamp. That should be easily supported in the
writer, shouldn't it?

[jiayl, 2014/05/23 17:48:36]

The timestamp is not intended to be human friendly and only added to make each
dump name unique. 
One should use the file creation time or last modified time to find a dump,
given the time of a call. 

+    // created. So we assign both incoming and outgoing dump path even if only
+    // one type of dumping has been started.
+    // For "Unretained(this)", see comments StopDump.
+    dump_writer_.reset(new WebRtcRtpDumpWriter(
+        incoming_dump_path_,
+        outgoing_dump_path_,
+        kMaxDumpSize,
+        base::Bind(&WebRtcRtpDumpHandler::OnMaxDumpSizeReached,
+                   base::Unretained(this))));
+  }
+
+  return true;
+}
+
+void WebRtcRtpDumpHandler::StopDump(RtpDumpType type,
+                                    const GenericDoneCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  // Returns an error if any type of dump specified by the caller cannot be
+  // stopped.
+  if ((DumpTypeContainsIncoming(type) && incoming_state_ != STATE_STARTED) ||
+      (DumpTypeContainsOutgoing(type) && outgoing_state_ != STATE_STARTED)) {
+    if (!callback.is_null()) {
+      FireGenericDoneCallback(
+          callback, false, "RTP dump not started or already stopped.");
+    }
+    return;
+  }
+
+  DVLOG(2) << "Stopping RTP dumping: type = " << type;
+
+  if (DumpTypeContainsIncoming(type))
+    incoming_state_ = STATE_STOPPING;
+
+  if (DumpTypeContainsOutgoing(type))
+    outgoing_state_ = STATE_STOPPING;
+
+  // Using "Unretained(this)" because the this object owns the writer and the
+  // writer is guaranteed to cancel the callback before it goes away. Same for
+  // the other posted tasks bound to the writer.
+  dump_writer_->EndDump(type,
+                        base::Bind(&WebRtcRtpDumpHandler::OnDumpEnded,
+                                   base::Unretained(this),
+                                   callback,
+                                   type));
+}
+
+WebRtcRtpDumpHandler::ReleasedDumps WebRtcRtpDumpHandler::ReleaseDumps() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  base::FilePath incoming_dump, outgoing_dump;
+
+  // All types of dumps must have been stopped, or one stopped and the other not
+  // started.
+  if (incoming_state_ == STATE_STARTED || incoming_state_ == STATE_STOPPING ||
+      outgoing_state_ == STATE_STARTED || outgoing_state_ == STATE_STOPPING ||
+      (incoming_state_ == STATE_NONE && outgoing_state_ == STATE_NONE)) {
+    DVLOG(2) << "ReleaseDumps called in invalid state: incoming_state = "

[Henrik Grunell, 2014/05/23 12:17:51]

Should this happen? If not, dcheck. It seems like we just keep on as normal even
if we're in a bad state, which seems to liberal to me.

[jiayl, 2014/05/23 17:48:36]

Changed so the caller can call ReadyToRelease to decide if it's valid to call
ReleaseDumps.

+             << incoming_state_ << ", outgoing_state = " << outgoing_state_;
+    return ReleasedDumps(incoming_dump, outgoing_dump);
+  }
+
+  if (incoming_state_ == STATE_STOPPED) {
+    DVLOG(2) << "Incoming RTP dumps released: " << incoming_dump_path_.value();
+
+    incoming_state_ = STATE_NONE;
+    incoming_dump = incoming_dump_path_;
+  }
+
+  if (outgoing_state_ == STATE_STOPPED) {
+    DVLOG(2) << "Outgoing RTP dumps released: " << outgoing_dump_path_.value();
+
+    outgoing_state_ = STATE_NONE;
+    outgoing_dump = outgoing_dump_path_;
+  }
+  return ReleasedDumps(incoming_dump, outgoing_dump);
+}
+
+void WebRtcRtpDumpHandler::OnRtpPacket(const uint8* packet_header,
+                                       size_t header_length,
+                                       size_t packet_length,
+                                       bool incoming) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  if ((incoming && incoming_state_ != STATE_STARTED) ||
+      (!incoming && outgoing_state_ != STATE_STARTED)) {
+    return;

[Henrik Grunell, 2014/05/23 12:17:51]

IS this expected to happen?

[jiayl, 2014/05/23 17:48:36]

I'd like to be defensive for the input to the public interfaces, to save the
caller from tracking the state of the dump.

[Henrik Grunell, 2014/05/26 09:51:03]

Is there a border case, e.g. there's a delay when starting or stopping the dump,
when this is considered normal?

I want to prevent programming errors where the user thinks the dump is started
but it's actually not. I don't think being defensive in general is strong enough
argument, I'd like to have a specific case when this is OK to happen.

[jiayl, 2014/05/27 20:41:40]

There is a delay in stopping the P2PSocketHost: the call is posted to UI thread
first then to the IO thread to flip the states.
So this may be called after WebRtcRtpDumpHandler is stopped but P2PSocketHost is
not.

+  }
+
+  dump_writer_->WriteRtpPacket(
+      packet_header, header_length, packet_length, incoming);
+}
+
+void WebRtcRtpDumpHandler::SetDumpWriterForTesting(
+    scoped_ptr<WebRtcRtpDumpWriter> writer) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  dump_writer_ = writer.Pass();
+  ++g_ongoing_rtp_dumps;

[Henrik Grunell, 2014/05/23 12:17:51]

Why increase the count when assigning a writer?

[jiayl, 2014/05/23 17:48:36]

Because the goal is to limit the total disk usage and the max dump size is
applied on a writer, so it's essentially to limit the number of writers.
Increasing the count here so we can test the max writer limit in unit tests.

[Henrik Grunell, 2014/05/26 09:51:03]

So, isn't start called after this? And increased again?

[jiayl, 2014/05/27 20:41:40]

It's not increased again because Start only increase it when a new writer is
created.

+
+  incoming_dump_path_ = dump_dir_.AppendASCII("recv");
+  outgoing_dump_path_ = dump_dir_.AppendASCII("send");
+}
+
+void WebRtcRtpDumpHandler::OnMaxDumpSizeReached() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  RtpDumpType type =
+      (incoming_state_ == STATE_STARTED)
+          ? (outgoing_state_ == STATE_STARTED ? RTP_DUMP_BOTH
+                                              : RTP_DUMP_INCOMING)
+          : RTP_DUMP_OUTGOING;
+  StopDump(type, GenericDoneCallback());

[Henrik Grunell, 2014/05/23 12:17:51]

The writer calls this object, which calls back into the writer. Will that be a
problem?

[jiayl, 2014/05/23 17:48:36]

I don't think so. Writer::EndDump can be called at any time.

+}
+
+void WebRtcRtpDumpHandler::OnDumpEnded(const GenericDoneCallback& callback,
+                                       RtpDumpType ended_type,
+                                       bool incoming_success,
+                                       bool outgoing_success) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  if (DumpTypeContainsIncoming(ended_type)) {
+    DCHECK_EQ(STATE_STOPPING, incoming_state_);
+    incoming_state_ = STATE_STOPPED;
+
+    if (!incoming_success) {
+      BrowserThread::PostTask(BrowserThread::FILE,
+                              FROM_HERE,
+                              base::Bind(base::IgnoreResult(&base::DeleteFile),
+                                         incoming_dump_path_,
+                                         false));
+
+      DVLOG(2) << "Deleted invalid incoming dump "
+               << incoming_dump_path_.value();
+      incoming_dump_path_.clear();
+    }
+  }
+
+  if (DumpTypeContainsOutgoing(ended_type)) {

[Henrik Grunell, 2014/05/23 12:17:51]

I really, really would like to get rid of the incoming/outgoing duplication in
the handler and writer. (I.e. use two handlers instead.) I can't dig up the
comment with the reason not to, can you refresh me?

[jiayl, 2014/05/23 17:48:36]

Here is the previous discussion:

https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we...
chrome/browser/media/webrtc_rtp_dump_handler.h:120:
scoped_ptr<WebRtcRtpDumpWriter> dump_writer_;
On 2014/05/14 16:07:58, jiayl wrote:
be
all
in
I see. And the incoming and outgoing can't be expected to be ~equal. OK, let's
keep it this way.

[Henrik Grunell, 2014/05/26 09:51:03]

We're just storing packet headers, right? So in what span can the header size
be? Could we simply have two writers & handlers and limit the size in each to
half of the current limit.

[jiayl, 2014/05/27 20:41:40]

Incoming and outgoing can be very asymmetric. E.g. a call may have outgoing
traffic only; or one direction has only video or audio, which impacts the size a
lot; there may be far more incoming streams than outgoing streams; JS may only
start one type of dump.

Limiting the size to half the limit is sub-optimal in these cases.

+    DCHECK_EQ(STATE_STOPPING, outgoing_state_);
+    outgoing_state_ = STATE_STOPPED;
+
+    if (!outgoing_success) {
+      BrowserThread::PostTask(BrowserThread::FILE,
+                              FROM_HERE,
+                              base::Bind(base::IgnoreResult(&base::DeleteFile),
+                                         outgoing_dump_path_,
+                                         false));
+
+      DVLOG(2) << "Deleted invalid outgoing dump "
+               << outgoing_dump_path_.value();
+      outgoing_dump_path_.clear();
+    }
+  }
+
+  if (!callback.is_null())
+    FireGenericDoneCallback(callback, true, "");
+
+  // Release the writer when it's no longer needed.
+  if (incoming_state_ != STATE_STOPPING && outgoing_state_ != STATE_STOPPING &&
+      incoming_state_ != STATE_STARTED && outgoing_state_ != STATE_STARTED) {
+    dump_writer_.reset();
+    --g_ongoing_rtp_dumps;
+  }
+}