[PPAPI] Implement an IOStreamResource for data transmission between plugin and renderer.

ppapi/shared_impl/circular_buffer.cc

+// Copyright (c) 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 "ppapi/shared_impl/circular_buffer.h"
+
+#include <algorithm>
+
+#include "base/logging.h"
+#include "ppapi/c/pp_errors.h"
+
+namespace ppapi {
+
+CircularBuffer::CircularBuffer(void* buffer, uint32_t size)
+    : buffer_(static_cast<uint8_t*>(buffer)),
+      buffer_size_(size),
+      position_(0),
+      limit_(0),
+      remaining_(0),
+      locked_buffer_(NULL),
+      locked_buffer_size_(0) {
+}
+
+CircularBuffer::~CircularBuffer() {
+}
+
+void CircularBuffer::MovePosition(uint32_t offset) {
+  CHECK(offset);

[yzshen1, 2014/01/03 21:51:41]

nit, optional: it seems harmless to allow offset==0 which is a no-op, right?
That may simplifies some callers' logic, I think.

+  CHECK(offset <= remaining_);

[yzshen1, 2014/01/03 21:51:41]

Please use CHECK_LE, or maybe we can just return failure?

+
+  position_ += offset;
+  remaining_ -= offset;
+  if (position_ >= buffer_size_)
+    position_ -= buffer_size_;
+}
+
+void CircularBuffer::MoveLimit(uint32_t offset) {
+  CHECK(offset);

[yzshen1, 2014/01/03 21:51:41]

please see my comment of the previous method.

+  CHECK(offset <= (buffer_size_ - remaining_));

[yzshen1, 2014/01/03 21:51:41]

Please use CHECK_LE, or maybe we can just return failure?

+
+  limit_ += offset;
+  remaining_ += offset;
+  if (limit_ >= buffer_size_)
+    limit_ -= buffer_size_;
+}
+
+int32_t CircularBuffer::Read(void* buffer, uint32_t size) {
+  // Buffer has been locked.
+  if (locked_buffer_)
+    return PP_ERROR_INPROGRESS;
+
+  size = std::min(size, remaining_);
+
+  if (!size)
+    return 0;
+
+  return ReadInternal(buffer, size);
+}
+
+int32_t CircularBuffer::ReadAll(void* buffer, uint32_t size) {
+  // Buffer has been locked.
+  if (locked_buffer_)
+    return PP_ERROR_INPROGRESS;
+
+  if (size > buffer_size_)
+    return PP_ERROR_BADARGUMENT;
+
+  if (size > remaining_)
+    return PP_ERROR_NOSPACE;
+
+  return ReadInternal(buffer, size);
+
+}
+
+int32_t CircularBuffer::ReadInternal(void* buffer, uint32_t size) {

[yzshen1, 2014/01/03 21:51:41]

Order definitions in the same order as declarations, please.

+  uint32_t n = std::min(buffer_size_ - position_, size);
+  memcpy(buffer, buffer_ + position_,  n);

[dmichael (off chromium), 2014/01/03 18:05:29]

Maybe it would be good to call the parameter something else; "buffer" and
"buffer_" are close and easy to mix up. Maybe "client_buffer" or something.

+  if (n != size) {

[dmichael (off chromium), 2014/01/03 18:05:29]

nit: It might be clearer to say:
if (n < size)

(the uses of |n| below are just more obviously correct that way, and it should
always be the case if n != size)

+    memcpy(static_cast<uint8_t*>(buffer) + n, buffer_,  size - n);
+  }
+  MovePosition(size);
+  return size;
+}
+
+int32_t CircularBuffer::Write(const void* buffer, uint32_t size) {
+  // Buffer has been locked.
+  if (locked_buffer_)
+    return PP_ERROR_INPROGRESS;
+
+  size = std::min(size, remaining_);

[yzshen1, 2014/01/03 21:51:41]

Ah, now I realize that remaining_ has a different meaning for read / write. And
why you only have one Lock() instead of LockForRead()/LockForWrite().

This is quite confusing. Moreover, read/write on the same instance will be
catastrophic. The interface of this class doesn't prevent that.

+  if (!size)
+    return 0;
+
+  return WriteInternal(buffer, size);
+}
+
+int32_t CircularBuffer::WriteAll(const void* buffer, uint32_t size) {
+  // Buffer has been locked.
+  if (locked_buffer_)
+    return PP_ERROR_INPROGRESS;
+
+  if (size > buffer_size_)
+    return PP_ERROR_BADARGUMENT;
+
+  if (size > remaining_)
+    return PP_ERROR_NOSPACE;
+
+  return WriteInternal(buffer, size);
+}
+
+int32_t CircularBuffer::WriteInternal(const void* buffer, uint32_t size) {
+  uint32_t n = std::min(buffer_size_ - position_, size);
+  memcpy(buffer_ + position_, buffer, n);
+  if (n != size) {
+    memcpy(buffer_, static_cast<const uint8_t*>(buffer) + n,  size - n);
+  }
+  MovePosition(size);
+  return size;
+}
+
+int32_t CircularBuffer::Lock(void** buffer, uint32_t size) {
+  CHECK(buffer);

[yzshen1, 2014/01/03 21:51:41]

[for discussion] My understanding:
- if you assume that the callers will always pass in the right thing, then this
could be DCHECK().
- if you assume that the callers may pass in wrong values and we should handle
that, then it should just return a failure.

It seems a little unusual to use CHECK (here and elsewhere in this class)

+  CHECK(size);
+
+  if (locked_buffer_)
+    return PP_ERROR_INPROGRESS;
+
+  // Request memory is not continuous.

[dmichael (off chromium), 2014/01/03 18:05:29]

continuous->contiguous?

+  if (position_ + size > buffer_size_)
+    return PP_ERROR_FAILED;
+
+  // Do not have enough space for locking.
+  if (size > remaining_)
+    return PP_ERROR_NOSPACE;
+
+  locked_buffer_ = buffer_ + position_;
+  *buffer = locked_buffer_;
+  locked_buffer_size_ = size;
+  return PP_OK;
+}
+
+int32_t CircularBuffer::Relock(void* buffer, uint32_t size) {
+  if (locked_buffer_ != buffer)
+    return PP_ERROR_BADARGUMENT;

[dmichael (off chromium), 2014/01/03 18:05:29]

Why do you not have the same checks here for enough space and for the space to
be contiguous? It seems like this could easily lead to the client reading or
writing past the end of the buffer.

+  locked_buffer_size_ = size;
+  return PP_OK;
+}
+
+int32_t CircularBuffer::Unlock(const void* buffer) {
+  if (!buffer)
+    return PP_ERROR_BADARGUMENT;
+  if (locked_buffer_ != buffer)
+    return PP_ERROR_BADARGUMENT;
+
+  int32_t size = locked_buffer_size_;
+
+  locked_buffer_ = NULL;
+  locked_buffer_size_ = 0;
+
+  MovePosition(size);
+  return size;
+}
+
+}  // namespace ppapi