Use CopyOnWriteBuffer instead of Buffer to avoid unnecessary copies.

webrtc/api/sctputils.cc

Index: webrtc/api/sctputils.cc
diff --git a/webrtc/api/sctputils.cc b/webrtc/api/sctputils.cc
index 1a2c282ff68103f5ecd16e6cdb6d1be5a3234614..07c1984ba1d9c4969de31899bedca680c2502cc2 100644
--- a/webrtc/api/sctputils.cc
+++ b/webrtc/api/sctputils.cc
@@ -10,8 +10,8 @@
 
 #include "webrtc/api/sctputils.h"
 
-#include "webrtc/base/buffer.h"
 #include "webrtc/base/bytebuffer.h"
+#include "webrtc/base/copyonwritebuffer.h"
 #include "webrtc/base/logging.h"
 
 namespace webrtc {
@@ -31,11 +31,12 @@ enum DataChannelOpenMessageChannelType {
   DCOMCT_UNORDERED_PARTIAL_TIME = 0x82,
 };
 
-bool IsOpenMessage(const rtc::Buffer& payload) {
+bool IsOpenMessage(const rtc::CopyOnWriteBuffer& payload) {
   // Format defined at
   // http://tools.ietf.org/html/draft-jesup-rtcweb-data-protocol-04
 
-  rtc::ByteBuffer buffer(payload);
+  // TODO(jbauch): avoid copying the payload data into the ByteBuffer
+  rtc::ByteBuffer buffer(payload.data<char>(), payload.size());

[tommi, 2016/03/18 10:54:37]

is this really just to check a single uint8?  Can we fix this now by checking
payload.size() >= 1 and checking the value of the first uint8?

[joachim, 2016/03/18 12:18:08]

Done, added bracket operators to CopyOnWriteBuffer for this.

   uint8_t message_type;
   if (!buffer.ReadUInt8(&message_type)) {
     LOG(LS_WARNING) << "Could not read OPEN message type.";
@@ -44,13 +45,14 @@ bool IsOpenMessage(const rtc::Buffer& payload) {
   return message_type == DATA_CHANNEL_OPEN_MESSAGE_TYPE;
 }
 
-bool ParseDataChannelOpenMessage(const rtc::Buffer& payload,
+bool ParseDataChannelOpenMessage(const rtc::CopyOnWriteBuffer& payload,
                                  std::string* label,
                                  DataChannelInit* config) {
   // Format defined at
   // http://tools.ietf.org/html/draft-jesup-rtcweb-data-protocol-04
 
-  rtc::ByteBuffer buffer(payload);
+  // TODO(jbauch): avoid copying the payload data into the ByteBuffer
+  rtc::ByteBuffer buffer(payload.data<char>(), payload.size());

[tommi, 2016/03/18 10:54:37]

This is a more complex case of course, maybe file a bug for tracking?

From looking at the code, I think it could be simplified quite a bit too.  Just
read all the required bytes into a struct on the stack that we can then check -
or, better yet, check the size, cast the buffer to a struct ptr and no need to
copy anything. (I'm guessing you're already thinking this)

[joachim, 2016/03/18 12:18:08]

Added https://bugs.chromium.org/p/webrtc/issues/detail?id=5670
I would defer this to a separate CL. Accessing the data directly might need to
convert from network byte order to host byte order.

   uint8_t message_type;
   if (!buffer.ReadUInt8(&message_type)) {
     LOG(LS_WARNING) << "Could not read OPEN message type.";
@@ -120,8 +122,9 @@ bool ParseDataChannelOpenMessage(const rtc::Buffer& payload,
   return true;
 }
 
-bool ParseDataChannelOpenAckMessage(const rtc::Buffer& payload) {
-  rtc::ByteBuffer buffer(payload);
+bool ParseDataChannelOpenAckMessage(const rtc::CopyOnWriteBuffer& payload) {
+  // TODO(jbauch): avoid copying the payload data into the ByteBuffer
+  rtc::ByteBuffer buffer(payload.data<char>(), payload.size());

[tommi, 2016/03/18 10:54:37]

so much copy :(

if we can fix this case now (since it seems very simple), that would be good.

[joachim, 2016/03/18 12:18:08]

Done.

   uint8_t message_type;
   if (!buffer.ReadUInt8(&message_type)) {
     LOG(LS_WARNING) << "Could not read OPEN_ACK message type.";
@@ -137,7 +140,7 @@ bool ParseDataChannelOpenAckMessage(const rtc::Buffer& payload) {
 
 bool WriteDataChannelOpenMessage(const std::string& label,
                                  const DataChannelInit& config,
-                                 rtc::Buffer* payload) {
+                                 rtc::CopyOnWriteBuffer* payload) {
   // Format defined at
   // http://tools.ietf.org/html/draft-ietf-rtcweb-data-protocol-00#section-6.1
   uint8_t channel_type = 0;
@@ -180,7 +183,7 @@ bool WriteDataChannelOpenMessage(const std::string& label,
   return true;
 }
 
-void WriteDataChannelOpenAckMessage(rtc::Buffer* payload) {
+void WriteDataChannelOpenAckMessage(rtc::CopyOnWriteBuffer* payload) {
   rtc::ByteBuffer buffer(rtc::ByteBuffer::ORDER_NETWORK);

[tommi, 2016/03/18 10:54:37]

I'm curious - do you know what's going on here?
a single byte buffer in network byte order?

I'm wondering what's the difference is between this function and doing:

uint8_t data = DATA_CHANNEL_OPEN_ACK_MESSAGE_TYPE;
payload->SetData(&data, sizeof(data));

[joachim, 2016/03/18 12:18:08]

Yeah, that looks overly complicated - changed to your suggestion.

   buffer.WriteUInt8(DATA_CHANNEL_OPEN_ACK_MESSAGE_TYPE);
   payload->SetData(buffer.Data(), buffer.Length());