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

ppapi/shared_impl/io_stream_shared.h

+// 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.
+
+#ifndef PPAPI_SHARED_IMPL_IO_STREAM_SHARED_H_
+#define PPAPI_SHARED_IMPL_IO_STREAM_SHARED_H_
+
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/shared_memory.h"
+#include "ppapi/shared_impl/circular_buffer.h"
+#include "ppapi/shared_impl/ppapi_shared_export.h"
+
+namespace ppapi {
+
+class CircularBuffer;
+
+class PPAPI_SHARED_EXPORT IOStreamShared {

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

Would it make sense at all to not have this as a base class and instead the
resource and host could just own one (IOStreamBuffer io_stream_buffer_)?

We overuse implementation inheritance, and it tends to get more confusing I
think.

If you're worried about exposing all the functions as thin wrappers (e.g.,
IOStreamResource::Write simply calling io_stream_buffer_.Write()), you could
instead provide an accessor for the buffer:

IOStreamBuffer& IOStreamResource::buffer() { return io_stream_buffer_; }

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

I like the idea of making it "part of" a resource.
I did that for device enumeration, please see device_enumeration_resource_helper
and pepper_device_enumeration_host_helper.

And I think it is better to have two separate interfaces for in/out.
The fact that Read() / Write() shouldn't be used on the same instance seems
confusing to me.
And having two interfaces also helps to have more descriptive names, for
example, we can change MovePeerLimit() to
NotifyDataProduced and NotifyDataConsumed.

+ public:
+  int32_t Write(const void* buffer, uint32_t size);

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

The int32_t and uint32_t mismatch is a little bit weird and it is possible that
|size| is too big to be represent by int32_t.

I understand that you did it that way so that you can return pp_errors. Is that
necessary? If yes, maybe we should change uint32_t to int32_t?

+
+  int32_t WriteAll(const void* buffer, uint32_t size);
+
+  int32_t Read(void* buffer, uint32_t size);
+
+  int32_t ReadAll(void* buffer, uint32_t size);
+
+  int32_t Lock(void** buffer, uint32_t size);
+
+  int32_t Relock(void* buffer, uint32_t size);
+
+  int32_t Unlock(void* buffer);
+
+  uint32_t remaining() const;
+
+  bool is_input() const { return is_input_; }
+
+  const base::SharedMemory* shared_memory() const {
+    return shm_.get();
+  }
+
+ protected:
+  IOStreamShared(bool is_input);

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

nit: explicit

+
+  virtual ~IOStreamShared();
+
+  // Set the underlying shared memory buffer. If IOStreamShared has a buffer

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

Set*s*

+  // set by |SetBUffer()|, the old buffer will be replaced.

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

SetB*u*ffer

+  int32_t SetBuffer(scoped_ptr<base::SharedMemory> shm, uint32_t size);
+
+  // Subclass uses this function to move the limit position of the
+  // |circular_buffer_|.
+  void MoveLimit(uint32_t offset);
+
+  // Subclass may overridden this function to receive notification when

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

overridden->override?

+  // more buffer is available.
+  virtual void OnMoreBufferAvailable();
+
+ private:
+  // Move limit position at the another peer of an IOStream pair.

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

I can't parse this sentence. Maybe you meant "at the peer", without "another"?

+  // Subclass should send IPC message to the another peer to move the limit

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

^^ here, too

+  // position.
+  virtual void MovePeerLimit(uint32_t offset) = 0;
+
+  // True if it is an input peer of an IO stream, otherwise it is an output peer
+  // of an IO stream.

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

What does input mean here? For reading?
Also, the word "this" might be clearer than "it". Maybe, e.g.:
"True if this is the input side of an IO stream, to be used for reading data
from the stream. Otherwise, it is the output side of an IO stream."

+  bool is_input_;
+
+  // Shared memory.
+  scoped_ptr<base::SharedMemory> shm_;
+
+  // A new shared memory will be used to replace the existing |shm_|.
+  scoped_ptr<base::SharedMemory> shm_new_;
+
+  // Circular buffer
+  scoped_ptr<CircularBuffer> circular_buffer_;
+
+  scoped_ptr<CircularBuffer> circular_buffer_new_;

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

This feels awkward to have the real shmem/circular buffer plus a "new" one. Can
we eliminate that and just not allow replacing the buffer?

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

+1!

I understand you did this for changing buffer size in the middle of a media
stream. But if you make the IOStream "part of" a resource, maybe we can create a
new IOStream instance when buffer size needs to change?

On 2014/01/03 18:05:29, dmichael wrote:

+
+  DISALLOW_COPY_AND_ASSIGN(IOStreamShared);
+};
+
+}  // namespace ppapi
+
+#endif  // PPAPI_SHARED_IMPL_IO_STREAM_SHARED_H_