Use CopyOnWriteBuffer instead of Buffer to avoid unnecessary copies.

webrtc/api/sctputils.cc

 /*
  *  Copyright 2013 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/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 {
 
 // Format defined at
 // http://tools.ietf.org/html/draft-ietf-rtcweb-data-protocol-01#section
 
 static const uint8_t DATA_CHANNEL_OPEN_MESSAGE_TYPE = 0x03;
 static const uint8_t DATA_CHANNEL_OPEN_ACK_MESSAGE_TYPE = 0x02;
 
 enum DataChannelOpenMessageChannelType {
   DCOMCT_ORDERED_RELIABLE = 0x00,
   DCOMCT_ORDERED_PARTIAL_RTXS = 0x01,
   DCOMCT_ORDERED_PARTIAL_TIME = 0x02,
   DCOMCT_UNORDERED_RELIABLE = 0x80,
   DCOMCT_UNORDERED_PARTIAL_RTXS = 0x81,
   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.";
     return false;
   }
   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.";
     return false;
   }
   if (message_type != DATA_CHANNEL_OPEN_MESSAGE_TYPE) {
     LOG(LS_WARNING) << "Data Channel OPEN message of unexpected type: "
                     << message_type;
     return false;
   }
@@ skipping 49 matching lines @@
       config->maxRetransmits = reliability_param;
       break;
     case DCOMCT_ORDERED_PARTIAL_TIME:
     case DCOMCT_UNORDERED_PARTIAL_TIME:
       config->maxRetransmitTime = reliability_param;
       break;
   }
   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.";
     return false;
   }
   if (message_type != DATA_CHANNEL_OPEN_ACK_MESSAGE_TYPE) {
     LOG(LS_WARNING) << "Data Channel OPEN_ACK message of unexpected type: "
                     << message_type;
     return false;
   }
   return true;
 }
 
 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;
   uint32_t reliability_param = 0;
   uint16_t priority = 0;
   if (config.ordered) {
     if (config.maxRetransmits > -1) {
       channel_type = DCOMCT_ORDERED_PARTIAL_RTXS;
       reliability_param = config.maxRetransmits;
     } else if (config.maxRetransmitTime > -1) {
@@ skipping 18 matching lines @@
       NULL, 20 + label.length() + config.protocol.length(),
       rtc::ByteBuffer::ORDER_NETWORK);
   buffer.WriteUInt8(DATA_CHANNEL_OPEN_MESSAGE_TYPE);
   buffer.WriteUInt8(channel_type);
   buffer.WriteUInt16(priority);
   buffer.WriteUInt32(reliability_param);
   buffer.WriteUInt16(static_cast<uint16_t>(label.length()));
   buffer.WriteUInt16(static_cast<uint16_t>(config.protocol.length()));
   buffer.WriteString(label);
   buffer.WriteString(config.protocol);
   payload->SetData(buffer.Data(), buffer.Length());

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

Hmm... there's no error checking for any of the preceding write operations and
no checking if Length() is the expected value.  I think there's room for
improvement.  Can you put down a TODO for me to take a look at this?

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

Done.

   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());
 }
 }  // namespace webrtc