Added CompoundBuffer that will be used to store data in the encoding/decoding

remoting/base/compound_buffer.h

+// Copyright (c) 2010 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.
+
+// CompoundBuffer implements a data buffer that is composed of several pieces,
+// each stored in a refcounted IOBuffer. It is needed for encoding/decoding
+// video pipeline to represent data packet and minimize data copying.
+// It is particularly useful for splitting data between multiple RTP packets
+// and assembling them into one buffer on the receiving side.

[awong, 2010/11/12 02:40:06]

Hmm...it feels like we just reimplemented rope. :P

http://www.sgi.com/tech/stl/Rope.html

[Sergey Ulanov, 2010/11/13 04:43:39]

hmm, yeah. Except rope doesn't use IOBuffer, so you need memcopy if you need to
put IOBuffer into it.

[awong, 2010/11/16 00:04:57]

Yeah...that sucks. :-/

+//
+// CompoundBuffer also implements ZeroCopyInputStream interface to be used by
+// protobuf to decode bytes into a protocol buffer message.
+
+#ifndef REMOTING_BASE_COMPOUND_BUFFER_H_
+#define REMOTING_BASE_COMPOUND_BUFFER_H_
+
+#include <deque>
+
+#include "base/basictypes.h"
+#include "base/ref_counted.h"
+#include "google/protobuf/io/zero_copy_stream.h"
+
+namespace net {
+class IOBuffer;
+class IOBufferWithSize;
+}  // namespace net
+
+namespace remoting {
+
+class CompoundBuffer :
+      public google::protobuf::io::ZeroCopyInputStream {

[awong, 2010/11/12 02:40:06]

Isn't this just supposed to go in 4-spaces?

[Sergey Ulanov, 2010/11/13 04:43:39]

":" must be on this line. Done.

+ public:
+  CompoundBuffer();
+  virtual ~CompoundBuffer();
+
+  void Clear();
+
+  // Adds new chunk to the buffer. |data| must point to start of the data

[awong, 2010/11/12 02:40:06]

How about rename |data| to |start|?

Also, specify if data_size is the size of the whole buffer, or the size of the
start.

Lastly, why pass in a pointer, and not an int that is the start position?

[Sergey Ulanov, 2010/11/13 04:43:39]

Done.
Done
The reason is that IOBuffer()->data() doesn't always point to the same location.
For example DrainableIOBuffer changes data() when you call DidConsume(). This
causes very nasty issues when this method is called with DrainableIOBuffer.

+  // within |buffer|.
+  void Append(net::IOBuffer* buffer, int data_size);
+  void Append(net::IOBuffer* buffer, const char* data, int data_size);

[awong, 2010/11/12 02:40:06]

overloading is frowned upon by the style guide.  Rename to

Append() and AppendWithStart()?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

[Sergey Ulanov, 2010/11/13 04:43:39]

I think this is a legal use of overloading, as both Append methods do the same
thing. No?
CopyAndAppend() does a different, hence a different name.

[awong, 2010/11/16 00:04:57]

Yeah...I don't feel fully comfortable with it, but you're right. It's not a
style guide issue.  Let's leave as is for now.

+  void Prepend(net::IOBuffer* buffer, int data_size);
+  void Prepend(net::IOBuffer* buffer, const char* data, int data_size);
+
+  // Same as above, but creates new IOBuffer and copies the data.
+  void CopyAndAppend(const char* data, int data_size);

[awong, 2010/11/12 02:40:06]

CopyFrom() below implies a clear.  These do not.

How about:

  AppendCopyOf()
  PrependCopyOf()

?

[Sergey Ulanov, 2010/11/13 04:43:39]

Done.

+  void CopyAndPrepend(const char* data, int data_size);
+
+  // Current size of the buffer.
+  int total_bytes() const { return total_bytes_; }
+
+  // Assemble all chunks in one buffer.

[awong, 2010/11/12 02:40:06]

The word "buffer" has too many meanings in this file.  There are IOBuffers,
CompoundBuffers, the "current buffer."  I'm not sure what this comment means. 
Clarify?

Also, you should state the memory ownership semantics.

[Sergey Ulanov, 2010/11/13 04:43:39]

Done.

+  net::IOBufferWithSize* Assemble() const;
+  void Assemble(char* data, int data_size) const;

[awong, 2010/11/12 02:40:06]

How about the names:

OutputAsIOBufferWithSize()
ToByteArray()

?

[Sergey Ulanov, 2010/11/13 04:43:39]

Renamed to ToIOBufferWithSize().
Renamed to CopyTo().

+
+  // Drops current content of the buffer, and initializes it with the interval

[awong, 2010/11/12 02:40:06]

How about "Calls Clear(), and then initializes..."

[Sergey Ulanov, 2010/11/13 04:43:39]

The fact that it calls Clear() is an implementation detail. Changed this to
"Clears the buffer ..."

+  // from |buffer| starting at |start| and ending at |end|. The data itself
+  // isn't copied.
+  void CopyFrom(const CompoundBuffer& source, int start, int end);
+
+  // google::protobuf::io::ZeroCopyInputStream interface.

[awong, 2010/11/12 02:40:06]

There seems to be some funky API interactions. Namely, after you start reading
via Next(), etc., some of the functions are no longer allowed to be called (eg.,
Append().

I wonder if it'd be better to create a thin wrapper that adapted the interfaces?

Maybe CompoundBufferInputStream?

[Sergey Ulanov, 2010/11/13 04:43:39]

Done. Also added Lock() method to ensure that the content doesn't change after
an CompoundBufferInputStream has been created.

+  virtual bool Next(const void** data, int* size);
+  virtual void BackUp(int count);
+  virtual bool Skip(int count);
+  virtual int64 ByteCount() const;
+
+ private:
+  struct DataChunk {
+    DataChunk(net::IOBuffer* buffer, const char* data_start, int data_size);
+
+    scoped_refptr<net::IOBuffer> buffer;
+    const char* data_start;
+    int data_size;
+  };
+  typedef std::deque<DataChunk> DataChunkList;
+
+  DataChunkList buffers_;
+  int total_bytes_;
+
+  // Current position for ZeroCopyInputStream.

[awong, 2010/11/12 02:40:06]

What happens to all this stuff if you start using things like Assemble, etc.?

This should be documented in the file header.

Maybe also give some sample usage?

[Sergey Ulanov, 2010/11/13 04:43:39]

Moved these to CompoundBufferInputStream

+  size_t current_buffer_;
+  int current_buffer_position_;
+  int position_;
+  int last_returned_size_;
+
+  DISALLOW_COPY_AND_ASSIGN(CompoundBuffer);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_BASE_COMPOUND_BUFFER_H_