Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
119853003:
[PPAPI] Implement an IOStreamResource for data transmission between plugin and renderer. (Closed)
|
Created:
6 years, 12 months ago by Peng Modified:
6 years, 11 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[PPAPI] Implement an IOStreamResource for data transmission between plugin and renderer.
It will be used by MediaStream API - MediaStreamVideo/AudioTrack and MediaStreamVideo/AudioProducer will inherit from IOStreamResource to get the data transmission functionality.
BUG=330851
Patch Set 1 : Update #
Total comments: 45
Patch Set 2 : Update #
Total comments: 12
Patch Set 3 : Update #
Total comments: 67
Messages
Total messages: 15 (0 generated)
Hi,
This CL adds an IOStreamResource which will be used as base class of
MediaStream{Audio,Video}{Track,Producer} for transmission data. PTAL. Thanks.
FYI. https://codereview.chromium.org/122383002/ is VidoTrack implementation
draft CL. It demos how to use the IOStreamResource.
https://codereview.chromium.org/119853003/diff/220001/content/renderer/pepper... File content/renderer/pepper/pepper_io_stream_host.cc (right): https://codereview.chromium.org/119853003/diff/220001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:41: bool input) is_input https://codereview.chromium.org/119853003/diff/220001/content/renderer/pepper... File content/renderer/pepper/pepper_io_stream_host.h (right): https://codereview.chromium.org/119853003/diff/220001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.h:19: : public ppapi::host::ResourceHost, public ppapi::IOStreamShared { One subclass per line. i.e. class PepperIOStreamHost : public ppapi::host::ResourceHost, public ppapi::IOStreamShared { https://codereview.chromium.org/119853003/diff/220001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.h:32: bool InitSharedBufferAndInfoPlugin(uint32_t size); InitSharedBufferAndNotifyPlugin would be clearer. https://codereview.chromium.org/119853003/diff/220001/ppapi/host/ppapi_host.h File ppapi/host/ppapi_host.h (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/host/ppapi_host.h... ppapi/host/ppapi_host.h:65: // Similar to SendUnsolicitedReply, but it support send a file handler. s/support send/supports sending s/file handler/file handle/ https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... File ppapi/proxy/io_stream_resource.cc (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:32: CHECK(size); You should probably return PP_ERROR_BADARGUMENT instead of asserting. https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... File ppapi/proxy/io_stream_resource.h (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:16: // renderer processes. It works as a ring buffer. Its underlayer is shared How about: It is implemented using shared memory. https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:34: // TODO(penghuang): Support initializing asynchronizely. sp asynchronously https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:42: // Handle Plugin MoveLimit message. s/MoveLimit/Init https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/circu... File ppapi/shared_impl/circular_buffer.h (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:27: } This is a bit long and complicated to be an inline using the getter style (lower case). https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:83: bool maybe_empty_; It's a little hard to understand how this works. Why not add a remaining_ field and keep it up to date in MovePosition and MoveLimit? https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... File ppapi/shared_impl/io_stream_shared.cc (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. s/2012/2013 or even 2014! https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:20: uint32_t size) { indent https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:23: CHECK(shm->Map(size)); These will crash the browser if compiled into the host class. You should probably use DCHECK or 'if' statements to handle errors here. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:31: // So we have to do it in |Unlock()|. Maybe this is a little clearer: // If |circular_buffer_| is locked , wait until the next call to |Unlock()|. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:105: // |circular_buffer_| is unlocked. we need replace it now. How about this? // Now that |circular_buffer_| is unlocked, swap in the new buffer. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... File ppapi/shared_impl/io_stream_shared.h (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. s/2012/2013 https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:5: #ifndef PPAPI_SHARED_IMPL_IO_STREAM_SHAERD_H_ s/SHAERD_H/SHARED_H https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:11: #include "base/memory/weak_ptr.h" I don't think this is needed. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:37: bool input() { return input_; } I think it would be clearer to call this is_input(). That probably implies changing 'input' to 'is_input' everywhere in this file. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:39: const scoped_ptr<base::SharedMemory>& shared_memory() const { It seems dangerous to return a ref to our scoped_ptr. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:78: scoped_ptr<CircularBuffer> circular_buffer_new_; I think you could get away with just having one CircularBuffer, and just contain it directly rather than use scoped_ptr. You would have to add a size_new_ field to hold the new size until you can do the swap. You would also need to change CircularBuffer to have a default constructor and an Init or SetBuffer method.
https://codereview.chromium.org/119853003/diff/220001/content/renderer/pepper... File content/renderer/pepper/pepper_io_stream_host.cc (right): https://codereview.chromium.org/119853003/diff/220001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:41: bool input) On 2013/12/31 00:51:53, bbudge1 wrote: > is_input Done. https://codereview.chromium.org/119853003/diff/220001/content/renderer/pepper... File content/renderer/pepper/pepper_io_stream_host.h (right): https://codereview.chromium.org/119853003/diff/220001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.h:19: : public ppapi::host::ResourceHost, public ppapi::IOStreamShared { On 2013/12/31 00:51:53, bbudge1 wrote: > One subclass per line. i.e. > > class PepperIOStreamHost > : public ppapi::host::ResourceHost, > public ppapi::IOStreamShared { Done. https://codereview.chromium.org/119853003/diff/220001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.h:32: bool InitSharedBufferAndInfoPlugin(uint32_t size); On 2013/12/31 00:51:53, bbudge1 wrote: > InitSharedBufferAndNotifyPlugin would be clearer. Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/host/ppapi_host.h File ppapi/host/ppapi_host.h (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/host/ppapi_host.h... ppapi/host/ppapi_host.h:65: // Similar to SendUnsolicitedReply, but it support send a file handler. On 2013/12/31 00:51:53, bbudge1 wrote: > s/support send/supports sending > s/file handler/file handle/ Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... File ppapi/proxy/io_stream_resource.cc (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:32: CHECK(size); On 2013/12/31 00:51:53, bbudge1 wrote: > You should probably return PP_ERROR_BADARGUMENT instead of asserting. Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... File ppapi/proxy/io_stream_resource.h (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/12/31 00:51:53, bbudge1 wrote: > 2013 Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:16: // renderer processes. It works as a ring buffer. Its underlayer is shared On 2013/12/31 00:51:53, bbudge1 wrote: > How about: > It is implemented using shared memory. Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:34: // TODO(penghuang): Support initializing asynchronizely. On 2013/12/31 00:51:53, bbudge1 wrote: > sp asynchronously Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:42: // Handle Plugin MoveLimit message. On 2013/12/31 00:51:53, bbudge1 wrote: > s/MoveLimit/Init Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/circu... File ppapi/shared_impl/circular_buffer.h (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:27: } On 2013/12/31 00:51:53, bbudge1 wrote: > This is a bit long and complicated to be an inline using the getter style (lower > case). Add remaining_ field, and this function returns it directly. Done https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:83: bool maybe_empty_; On 2013/12/31 00:51:53, bbudge1 wrote: > It's a little hard to understand how this works. Why not add a remaining_ field > and keep it up to date in MovePosition and MoveLimit? Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... File ppapi/shared_impl/io_stream_shared.cc (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/12/31 00:51:53, bbudge1 wrote: > s/2012/2013 > > or even 2014! Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:20: uint32_t size) { On 2013/12/31 00:51:53, bbudge1 wrote: > indent Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:23: CHECK(shm->Map(size)); On 2013/12/31 00:51:53, bbudge1 wrote: > These will crash the browser if compiled into the host class. You should > probably use DCHECK or 'if' statements to handle errors here. Done. Fixed in this function. For other functions, probably CHECK() is good. Because those functions are used by renderer or third-party plugin code. For renderer, it should make sure all passed in arguments are valid. For third-party plugin code, make plugin crash could help developers diagnose issues. thought? https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:31: // So we have to do it in |Unlock()|. On 2013/12/31 00:51:53, bbudge1 wrote: > Maybe this is a little clearer: > // If |circular_buffer_| is locked , wait until the next call to |Unlock()|. Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:105: // |circular_buffer_| is unlocked. we need replace it now. On 2013/12/31 00:51:53, bbudge1 wrote: > How about this? > // Now that |circular_buffer_| is unlocked, swap in the new buffer. Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... File ppapi/shared_impl/io_stream_shared.h (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/12/31 00:51:53, bbudge1 wrote: > s/2012/2013 Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:5: #ifndef PPAPI_SHARED_IMPL_IO_STREAM_SHAERD_H_ On 2013/12/31 00:51:53, bbudge1 wrote: > s/SHAERD_H/SHARED_H Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:11: #include "base/memory/weak_ptr.h" On 2013/12/31 00:51:53, bbudge1 wrote: > I don't think this is needed. Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:37: bool input() { return input_; } On 2013/12/31 00:51:53, bbudge1 wrote: > I think it would be clearer to call this is_input(). > That probably implies changing 'input' to 'is_input' everywhere in this file. Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:39: const scoped_ptr<base::SharedMemory>& shared_memory() const { On 2013/12/31 00:51:53, bbudge1 wrote: > It seems dangerous to return a ref to our scoped_ptr. Done. https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:78: scoped_ptr<CircularBuffer> circular_buffer_new_; On 2013/12/31 00:51:53, bbudge1 wrote: > I think you could get away with just having one CircularBuffer, and just contain > it directly rather than use scoped_ptr. > > You would have to add a size_new_ field to hold the new size until you can do > the swap. You would also need to change CircularBuffer to have a default > constructor and an Init or SetBuffer method. I use two circular_buffers because when a new shared memory is set, the |MoveLimit()|, |Relock()| can still be used before |Unlock()|. In this case, |MoveLimit()| will move the limit_ in |circular_buffer_new_| instead of |circular_buffer_|. If we want to use one buffer, probably we have to duplicate some fields in CircularBuffer. So I think using two circular buffers is slight simpler. BTW, currently, we will drop all frames if underlying buffer is changed. probably it is OK. But if we want, we could swap circular_buffer_ when all content in the old buffer is consumed instead of in |Unlock()|. Not sure if it is necessary. thought?
https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... File ppapi/shared_impl/io_stream_shared.h (right): https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:78: scoped_ptr<CircularBuffer> circular_buffer_new_; On 2013/12/31 23:33:54, Peng wrote: > On 2013/12/31 00:51:53, bbudge1 wrote: > > I think you could get away with just having one CircularBuffer, and just > contain > > it directly rather than use scoped_ptr. > > > > You would have to add a size_new_ field to hold the new size until you can do > > the swap. You would also need to change CircularBuffer to have a default > > constructor and an Init or SetBuffer method. > > I use two circular_buffers because when a new shared memory is set, the > |MoveLimit()|, |Relock()| can still be used before |Unlock()|. In this case, > |MoveLimit()| will move the limit_ in |circular_buffer_new_| instead of > |circular_buffer_|. If we want to use one buffer, probably we have to duplicate > some fields in CircularBuffer. So I think using two circular buffers is slight > simpler. > > BTW, currently, we will drop all frames if underlying buffer is changed. > probably it is OK. But if we want, we could swap circular_buffer_ when all > content in the old buffer is consumed instead of in |Unlock()|. Not sure if it > is necessary. > > > thought? > > > > I'm suggesting that when SetBuffer is called, but the CircularBuffer is locked, you simply store the shm in shm_new_ and wait until the buffer is unlocked before creating the new CircularBuffer. Any calls to MoveLimit before Unlock should work on the old CircularBuffer. I don't know much about how this would be used so I'm not sure about your last question. I would expect that SetBuffer would only be called once in the vast majority of usage scenarios so I would go with the approach that is easiest to implement for now.
On 2014/01/02 18:51:42, bbudge1 wrote: > https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... > File ppapi/shared_impl/io_stream_shared.h (right): > > https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... > ppapi/shared_impl/io_stream_shared.h:78: scoped_ptr<CircularBuffer> > circular_buffer_new_; > On 2013/12/31 23:33:54, Peng wrote: > > On 2013/12/31 00:51:53, bbudge1 wrote: > > > I think you could get away with just having one CircularBuffer, and just > > contain > > > it directly rather than use scoped_ptr. > > > > > > You would have to add a size_new_ field to hold the new size until you can > do > > > the swap. You would also need to change CircularBuffer to have a default > > > constructor and an Init or SetBuffer method. > > > > I use two circular_buffers because when a new shared memory is set, the > > |MoveLimit()|, |Relock()| can still be used before |Unlock()|. In this case, > > |MoveLimit()| will move the limit_ in |circular_buffer_new_| instead of > > |circular_buffer_|. If we want to use one buffer, probably we have to > duplicate > > some fields in CircularBuffer. So I think using two circular buffers is slight > > simpler. > > > > BTW, currently, we will drop all frames if underlying buffer is changed. > > probably it is OK. But if we want, we could swap circular_buffer_ when all > > content in the old buffer is consumed instead of in |Unlock()|. Not sure if it > > is necessary. > > > > > > thought? > > > > > > > > > > I'm suggesting that when SetBuffer is called, but the CircularBuffer is locked, > you simply store the shm in shm_new_ and wait until the buffer is unlocked > before creating the new CircularBuffer. Any calls to MoveLimit before Unlock > should work on the old CircularBuffer. Maybe it is little confusing. Let me explain the implementation detail. In current implementation, after |SetBuffer()| is called, the sender will use the new buffer to send all new frames, and the receiver will not report frame consuming on the old buffer. So any |MoveLimit()| after |SetBuffer()| is for the new buffer. So it is easier to have a new circular buffer to handle it. When the old circular buffer is unlocked, we will replace it with the new one. > > I don't know much about how this would be used so I'm not sure about your last > question. I would expect that SetBuffer would only be called once in the vast > majority of usage scenarios so I would go with the approach that is easiest to > implement for now. You are right, currently, it will be only called once. But in the w3c spec, MediaStreamTrack can be reconfigured by using applyConstraints() (changing format, video frame size, etc. but chrome does not support it right now).
On 2014/01/02 20:36:10, Peng wrote: > On 2014/01/02 18:51:42, bbudge1 wrote: > > > https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... > > File ppapi/shared_impl/io_stream_shared.h (right): > > > > > https://codereview.chromium.org/119853003/diff/220001/ppapi/shared_impl/io_st... > > ppapi/shared_impl/io_stream_shared.h:78: scoped_ptr<CircularBuffer> > > circular_buffer_new_; > > On 2013/12/31 23:33:54, Peng wrote: > > > On 2013/12/31 00:51:53, bbudge1 wrote: > > > > I think you could get away with just having one CircularBuffer, and just > > > contain > > > > it directly rather than use scoped_ptr. > > > > > > > > You would have to add a size_new_ field to hold the new size until you can > > do > > > > the swap. You would also need to change CircularBuffer to have a default > > > > constructor and an Init or SetBuffer method. > > > > > > I use two circular_buffers because when a new shared memory is set, the > > > |MoveLimit()|, |Relock()| can still be used before |Unlock()|. In this case, > > > |MoveLimit()| will move the limit_ in |circular_buffer_new_| instead of > > > |circular_buffer_|. If we want to use one buffer, probably we have to > > duplicate > > > some fields in CircularBuffer. So I think using two circular buffers is > slight > > > simpler. > > > > > > BTW, currently, we will drop all frames if underlying buffer is changed. > > > probably it is OK. But if we want, we could swap circular_buffer_ when all > > > content in the old buffer is consumed instead of in |Unlock()|. Not sure if > it > > > is necessary. > > > > > > > > > thought? > > > > > > > > > > > > > > > > I'm suggesting that when SetBuffer is called, but the CircularBuffer is > locked, > > you simply store the shm in shm_new_ and wait until the buffer is unlocked > > before creating the new CircularBuffer. Any calls to MoveLimit before Unlock > > should work on the old CircularBuffer. > > Maybe it is little confusing. Let me explain the implementation detail. > In current implementation, after |SetBuffer()| is called, the sender will use > the new buffer to send all new frames, and the receiver will not report frame > consuming on the old buffer. So any |MoveLimit()| after |SetBuffer()| is for the > new buffer. So it is easier to have a new circular buffer to handle it. When the > old circular buffer is unlocked, we will replace it with the new one. OK, I see now. > > > > > I don't know much about how this would be used so I'm not sure about your last > > question. I would expect that SetBuffer would only be called once in the vast > > majority of usage scenarios so I would go with the approach that is easiest to > > implement for now. > > You are right, currently, it will be only called once. But in the w3c spec, > MediaStreamTrack can be reconfigured by using applyConstraints() (changing > format, video frame size, etc. but chrome does not support it right now).
https://codereview.chromium.org/119853003/diff/480001/content/renderer/pepper... File content/renderer/pepper/pepper_io_stream_host.cc (right): https://codereview.chromium.org/119853003/diff/480001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:50: CHECK(offset); DCHECK? https://codereview.chromium.org/119853003/diff/480001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:74: shm = render_thread->HostAllocateSharedMemoryBuffer(size).Pass(); Why not assign directly (on line above) to avoid default ctor running? i.e. scoped_ptr<base::SharedMemory> shm = ... https://codereview.chromium.org/119853003/diff/480001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:102: // ourselves. This comment won't be needed if you have a reply message (see below). https://codereview.chromium.org/119853003/diff/480001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:110: SendReply(reply_context, IPC::Message()); If a message requires a reply to get back some result or handle, we usually define a reply message. In this case you define: IPC_MESSAGE_CONTROL1(PpapiHostMsg_IOStream_Init, uint32_t /* size */) So, you would define this reply: IPC_MESSAGE_CONTROL0(PpapiPluginMsg_IOStream_InitReply) https://codereview.chromium.org/119853003/diff/480001/ppapi/proxy/io_stream_r... File ppapi/proxy/io_stream_resource.cc (right): https://codereview.chromium.org/119853003/diff/480001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:72: new base::SharedMemory(shm_handle, is_input())), size); indentation https://codereview.chromium.org/119853003/diff/480001/ppapi/proxy/io_stream_r... File ppapi/proxy/io_stream_resource.h (right): https://codereview.chromium.org/119853003/diff/480001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:35: // TODO(penghuang): Support initializing asynchronously. We really encourage asynchronous calls for a bunch of reasons. Can we do it asynchronously now?
https://codereview.chromium.org/119853003/diff/480001/content/renderer/pepper... File content/renderer/pepper/pepper_io_stream_host.cc (right): https://codereview.chromium.org/119853003/diff/480001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:50: CHECK(offset); On 2014/01/02 22:16:22, bbudge1 wrote: > DCHECK? Done. https://codereview.chromium.org/119853003/diff/480001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:74: shm = render_thread->HostAllocateSharedMemoryBuffer(size).Pass(); On 2014/01/02 22:16:22, bbudge1 wrote: > Why not assign directly (on line above) to avoid default ctor running? i.e. > > scoped_ptr<base::SharedMemory> shm = ... Done https://codereview.chromium.org/119853003/diff/480001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:102: // ourselves. On 2014/01/02 22:16:22, bbudge1 wrote: > This comment won't be needed if you have a reply message (see below). Done. https://codereview.chromium.org/119853003/diff/480001/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:110: SendReply(reply_context, IPC::Message()); On 2014/01/02 22:16:22, bbudge1 wrote: > If a message requires a reply to get back some result or handle, we usually > define a reply message. In this case you define: > > IPC_MESSAGE_CONTROL1(PpapiHostMsg_IOStream_Init, > uint32_t /* size */) > > So, you would define this reply: > > IPC_MESSAGE_CONTROL0(PpapiPluginMsg_IOStream_InitReply) Done. https://codereview.chromium.org/119853003/diff/480001/ppapi/proxy/io_stream_r... File ppapi/proxy/io_stream_resource.cc (right): https://codereview.chromium.org/119853003/diff/480001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:72: new base::SharedMemory(shm_handle, is_input())), size); On 2014/01/02 22:16:22, bbudge1 wrote: > indentation The vim editor gives it 6 spaces intent. In this case, 'new base::...' is an argument of scoped_ptr<base::SharedMemory>(). It should be 4 spaces or 8 spaces? I changed it to 8 spaces. If it not correct, please let me know. Thanks. Done https://codereview.chromium.org/119853003/diff/480001/ppapi/proxy/io_stream_r... File ppapi/proxy/io_stream_resource.h (right): https://codereview.chromium.org/119853003/diff/480001/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:35: // TODO(penghuang): Support initializing asynchronously. On 2014/01/02 22:16:22, bbudge1 wrote: > We really encourage asynchronous calls for a bunch of reasons. Can we do it > asynchronously now? Done
Sorry for taking a while to get to this review; I was gone for most of the holidays. https://codereview.chromium.org/119853003/diff/660002/ppapi/host/ppapi_host.cc File ppapi/host/ppapi_host.cc (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/host/ppapi_host.c... ppapi/host/ppapi_host.cc:110: const SerializedHandle& handle) { Maybe this accept a vector of handles? Then SendUnsolicitedReply can pass an empty vector, and you don't need the IsHandleValid check, and it will be able to support multiple handles if that's ever necessary. https://codereview.chromium.org/119853003/diff/660002/ppapi/host/ppapi_host.h File ppapi/host/ppapi_host.h (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/host/ppapi_host.h... ppapi/host/ppapi_host.h:65: // Similar to SendUnsolicitedReply, but it support sending a file handle. support->supports https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... File ppapi/proxy/io_stream_resource.cc (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:66: const ResourceMessageReplyParams& params, uint32_t size) { Why is this "ResourceMessageReplyParams" instead of just "ResourceMessageParams"? Same question for OnPluginMsgMoveLimit. https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:69: SetBuffer(scoped_ptr<base::SharedMemory>( Note I think this is only ever changing from an empty buffer to a real one. I think you can get away with having an unitialized IOStreamBuffer that can get initialized only once, and omit the shm_new_ and circular_buffer_new_ stuff. https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:78: void IOStreamResource::OnPluginMsgInitReply( Am I understanding correctly that there are two distinct ways a client could initialize the IOStreamResource? Either by calling Init, or because the host sent a PpapiPluginMsg_IOStream_Init? Can we cut it down to just 1 way to do it? https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:85: callback->Run(result);; nit: extra semicolon https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... File ppapi/shared_impl/circular_buffer.cc (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.cc:77: memcpy(buffer, buffer_ + position_, n); 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. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.cc:78: if (n != size) { 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) https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.cc:128: // Request memory is not continuous. continuous->contiguous? https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.cc:144: return PP_ERROR_BADARGUMENT; 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. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... File ppapi/shared_impl/circular_buffer.h (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:20: // Remaining buffer size for reading or writing. nit: size in bytes? Same above. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:21: uint32_t remaining() const { I'm worried this might get confusing for users of Lock, since you can't always lock "remaining()" bytes. Maybe it would make sense to just eliminate this function and force them to attempt the Read/Write/Lock and rely on the return value? https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:29: void MoveLimit(uint32_t offset); Should these be private or protected? Doesn't seem like it should be part of the public interface. Should clients instead be using Lock/Unlock? https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:35: // Similar to |Read()|, but it will fail, if the circular buffer does not have nit: no comma after fail (same for WriteAll) https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:45: int32_t WriteAll(const void* buffer, uint32_t size); Should WriteAll and ReadAll return bool instead of int32_t? https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:50: // Relocks underlying buffer. It is used for adjusting locked block size; nit: semicolon should be a period. Please also note what the return value is here and for Lock. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:54: // size will be returned. You should also note that the position is moved by the size of the locked region. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:66: uint8_t* buffer_; You should probably note that this is unowned (it is, right?) https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:81: // When |Unlock()| is called. nit: no comma after NULL, "When" should not be capitalized. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... File ppapi/shared_impl/io_stream_shared.cc (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:117: return circular_buffer_->remaining(); This isn't right if there's a "new" CircularBuffer and we're still waiting on the unlock. (I think the best way to "fix" it is to remove the ability to replace the buffer at all, if that's possible) https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... File ppapi/shared_impl/io_stream_shared.h (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:18: class PPAPI_SHARED_EXPORT IOStreamShared { 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_; } https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:43: IOStreamShared(bool is_input); nit: explicit https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:55: // Subclass may overridden this function to receive notification when overridden->override? https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:60: // Move limit position at the another peer of an IOStream pair. I can't parse this sentence. Maybe you meant "at the peer", without "another"? https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:61: // Subclass should send IPC message to the another peer to move the limit ^^ here, too https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:66: // of an IO stream. 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." https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:78: scoped_ptr<CircularBuffer> circular_buffer_new_; 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?
The way Lock is implemented, doesn't this imply that you can only use Lock and use 1 frame at a time? That seems bad. I'm also a little concerned about Read/Write (and also MoveLimit). It seems like we don't actually want to allow both reading and writing on the same side of a stream... what do you think?
https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... File ppapi/proxy/io_stream_resource.cc (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:69: SetBuffer(scoped_ptr<base::SharedMemory>( On 2014/01/03 18:05:29, dmichael wrote: > Note I think this is only ever changing from an empty buffer to a real one. I > think you can get away with having an unitialized IOStreamBuffer that can get > initialized only once, and omit the shm_new_ and circular_buffer_new_ stuff. Actually this function can be used more than once. Probably it is better to rename it to SetBufferSize(). We support changing buffer size because two reasons: 1. In w3c spec, the track can be reconfigured by track.applyConstraints() (For example: change the video size). But chrome does not support applyConstraints() right now. Not sure if it will be supported in future. 2. In current implementation, when track host gets the first frame, it will allocate the shared buffer with a default size (max_buffered_frames * size_of_one_frame). But we have an API Configure() which can be used to change |max_buffered_frames| later. BTW, I am thinking if it is necessary to have an API to change the max_buffered_frames. Probably using a fixed number 2 should be good enough. For a live stream, if the plugin's speed is slow, I think having more buffered frames does help anything. It will only delay the happening of frame dropping, but it can not avoid frame dropping. I noticed the private video API does not support configure the buffer size. The dev video capture API supports it. But it is for multiple frames processing, not for handling the not match speed issue. See https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/dev/ppb_... https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:78: void IOStreamResource::OnPluginMsgInitReply( On 2014/01/03 18:05:29, dmichael wrote: > Am I understanding correctly that there are two distinct ways a client could > initialize the IOStreamResource? Either by calling Init, or because the host > sent a PpapiPluginMsg_IOStream_Init? > > Can we cut it down to just 1 way to do it? There are different requirements for Track & Producer. For track, plugin does not know the frame information (size, format, etc) until the host gets the first frame. So it is easier to let host allocate the buffer. For producer, plugin knows the frame informations, so it is better to let plugin allocate the buffer.
On 2014/01/03 18:33:17, dmichael wrote: > The way Lock is implemented, doesn't this imply that you can only use Lock and > use 1 frame at a time? That seems bad. Yeah. plugin can only access one frame at a time with the ring buffer. I realized the issue, after the circluar buffer is implemented. :( It is not good for multi-frame processing applications. They have to copy frames to local buffers. Do you think is it OK? Otherwise, we have to use different data structure. Thought? > > I'm also a little concerned about Read/Write (and also MoveLimit). It seems like > we don't actually want to allow both reading and writing on the same side of a > stream... what do you think? It does not allow both reading and writing on the same side. For input side, Read() will fail. Same Write() will fail for output side. Why keep them in one class is for avoiding many duplicated code. Actually, mediastream API only uses Lock()/Unlock(). So we can remove Read()/Write(). What do you think? But MoveLimit() is different. It is 'called' by the other side of an IOStream via an IPC message. It means there are more buffer available for reading/writing.
https://codereview.chromium.org/119853003/diff/660002/content/renderer/pepper... File content/renderer/pepper/pepper_io_stream_host.cc (right): https://codereview.chromium.org/119853003/diff/660002/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:14: nit: extra empty line https://codereview.chromium.org/119853003/diff/660002/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:25: base::PlatformFile ToPlatformFile(const base::SharedMemoryHandle& handle) { please consider putting it in shared_impl/platform_file. https://codereview.chromium.org/119853003/diff/660002/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:49: void PepperIOStreamHost::MovePeerLimit(uint32_t offset) { please order the method definitions in the same order as declarations. https://codereview.chromium.org/119853003/diff/660002/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:61: HostDispatcher* dispatcher = HostDispatcher::GetForInstance(pp_instance()); Please use RendererPpapiHost::ShareHandleWithRemote() https://codereview.chromium.org/119853003/diff/660002/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.cc:102: HostDispatcher* dispatcher = HostDispatcher::GetForInstance(pp_instance()); ditto https://codereview.chromium.org/119853003/diff/660002/content/renderer/pepper... File content/renderer/pepper/pepper_io_stream_host.h (right): https://codereview.chromium.org/119853003/diff/660002/content/renderer/pepper... content/renderer/pepper/pepper_io_stream_host.h:18: class PepperIOStreamHost Please consider naming it ...Base. https://codereview.chromium.org/119853003/diff/660002/ppapi/host/ppapi_host.cc File ppapi/host/ppapi_host.cc (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/host/ppapi_host.c... ppapi/host/ppapi_host.cc:21: using ppapi::proxy::SerializedHandle; nit: no need to have ppapi:: https://codereview.chromium.org/119853003/diff/660002/ppapi/host/ppapi_host.h File ppapi/host/ppapi_host.h (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/host/ppapi_host.h... ppapi/host/ppapi_host.h:69: const ppapi::proxy::SerializedHandle& handle); nit: no need to have "ppapi::". https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... File ppapi/proxy/io_stream_resource.cc (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:42: weak_factory_.GetWeakPtr(), size, callback)); You don't need this weak_factory_. You could use base::Unretained(). it is safe because the callback won't be run if this object goes away. https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:52: const proxy::ResourceMessageReplyParams& params, const IPC::Message& msg) { proxy:: is not needed. https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:66: const ResourceMessageReplyParams& params, uint32_t size) { On 2014/01/03 18:05:29, dmichael wrote: > Why is this "ResourceMessageReplyParams" instead of just > "ResourceMessageParams"? Same question for OnPluginMsgMoveLimit. I thought we always use ResourceMessageReplyParams for handling reply messages? https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.cc:70: new base::SharedMemory(shm_handle, is_input())), size); wrong indent https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... File ppapi/proxy/io_stream_resource.h (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:19: class PPAPI_PROXY_EXPORT IOStreamResource I think if it is intended to be a base class for media stream resources, it is better to name it IOStreamResource*Base*. What do you think? https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:39: virtual void OnReplyReceived(const proxy::ResourceMessageReplyParams& params, no need to have proxy:: https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/io_stream_r... ppapi/proxy/io_stream_resource.h:43: // Handle Plugin Init message. handle*s* (here and below) https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/ppapi_messa... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/proxy/ppapi_messa... ppapi/proxy/ppapi_messages.h:1408: IPC_MESSAGE_CONTROL1(PpapiHostMsg_IOStream_Init, Please add comments: - Since there are two ways of initialization, please comment what the messages means. - Some messages carry handles, please comment about that. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... File ppapi/shared_impl/circular_buffer.cc (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.cc:28: CHECK(offset); nit, optional: it seems harmless to allow offset==0 which is a no-op, right? That may simplifies some callers' logic, I think. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.cc:29: CHECK(offset <= remaining_); Please use CHECK_LE, or maybe we can just return failure? https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.cc:38: CHECK(offset); please see my comment of the previous method. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.cc:39: CHECK(offset <= (buffer_size_ - remaining_)); Please use CHECK_LE, or maybe we can just return failure? https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.cc:75: int32_t CircularBuffer::ReadInternal(void* buffer, uint32_t size) { Order definitions in the same order as declarations, please. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.cc:90: size = std::min(size, remaining_); 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. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.cc:122: CHECK(buffer); [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) https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... File ppapi/shared_impl/circular_buffer.h (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:15: // Construct a CircularBuffer by given underlying buffer and size. Construct*s* https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:31: // Reads data and move position forward. The actual read size will be move*s* https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:33: int32_t Read(void* buffer, uint32_t size); The uint32_t and int32_t mismatch seems a little weird. It is possible that |size| is too big that the return value cannot represent. (here and below) https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:39: // Writes data and move position forward. The actual write size will be move*s* https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:48: int32_t Lock(void** buffer, uint32_t size); What is the meaning of the return value? Do you mean it may return a value smaller than requested? https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:53: // Unlocks buffer which is locked by previous |Lock()| call. The locked block Is it still necessary to return the block size? https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer.h:55: int32_t Unlock(const void* buffer); Now that it is only allow to lock one block at a time, maybe we don't need |buffer| in this method and the Relock() method? https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... File ppapi/shared_impl/circular_buffer_unittest.cc (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/circu... ppapi/shared_impl/circular_buffer_unittest.cc:8: #include "ppapi/shared_impl/test_globals.h" This is not needed. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... File ppapi/shared_impl/io_stream_shared.cc (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:21: if (!size) nit: For numeric comparison, please use size == 0. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.cc:42: CHECK(!is_input_); Please see my comment about parameter validation in circular_buffer https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... File ppapi/shared_impl/io_stream_shared.h (right): https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:18: class PPAPI_SHARED_EXPORT IOStreamShared { 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. https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:20: int32_t Write(const void* buffer, uint32_t size); 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? https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:47: // Set the underlying shared memory buffer. If IOStreamShared has a buffer Set*s* https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:48: // set by |SetBUffer()|, the old buffer will be replaced. SetB*u*ffer https://codereview.chromium.org/119853003/diff/660002/ppapi/shared_impl/io_st... ppapi/shared_impl/io_stream_shared.h:78: scoped_ptr<CircularBuffer> circular_buffer_new_; +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: > 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?
On 2014/01/03 21:30:05, Peng wrote: > On 2014/01/03 18:33:17, dmichael wrote: > > The way Lock is implemented, doesn't this imply that you can only use Lock and > > use 1 frame at a time? That seems bad. > > Yeah. plugin can only access one frame at a time with the ring buffer. I > realized the issue, after the circluar buffer is implemented. :( > It is not good for multi-frame processing applications. They have to copy frames > to local buffers. Do you think is it OK? Otherwise, we have to use different > data structure. Thought? We definitely need at least 2 frames to be active, or there's really no point in having a ring buffer, right? A place for the producer to write, and a place for the consumer to read. If you want to make it only allow 2 for now, that might be OK, but the API and machinery around it would be more confusing if that's really all we allow, since it's depicted as being a lot more general. And it might be limiting for use cases where latency isn't important. So... I think I'd prefer if it was general. If it's not, we should be super clear in the documentation that this is really only a ping-pong kind of buffer. > > > > > I'm also a little concerned about Read/Write (and also MoveLimit). It seems > like > > we don't actually want to allow both reading and writing on the same side of a > > stream... what do you think? > > It does not allow both reading and writing on the same side. For input side, > Read() will fail. Same Write() will fail for output side. Why keep them in one > class is for avoiding many duplicated code. Actually, mediastream API only uses > Lock()/Unlock(). So we can remove Read()/Write(). What do you think? > But MoveLimit() is different. It is 'called' by the other side of an IOStream > via an IPC message. It means there are more buffer available for > reading/writing. I think removing Read/Write as they exist might be good. As Yuzhu mentioned, I think it makes sense to separate this in to a Reader and a Writer, or Producer/Consumer, to avoid confusion and misuse. In that case, you might actually do something like Lock currently does, but call it Read (or Write). Taking Read as an example... if there is contiguous space available, it returns a pointer to that space to the caller. When the reader side is done with that data, it can call something like ReadFinished, passing the pointer where the read had finished. Your CircularBuffer would probably need a queue of pending reads, or something, in addition to a read pointer/counter. The counter would point to the next region that should be returned by Read. When ReadFinished is called, pop off the front of the queue so you know how much space you can pass to the peer Writer. Something like that is probably not going to be too hard, and is probably worth the effort IMO.
On 2014/01/03 22:37:05, dmichael wrote: > On 2014/01/03 21:30:05, Peng wrote: > > On 2014/01/03 18:33:17, dmichael wrote: > > > The way Lock is implemented, doesn't this imply that you can only use Lock > and > > > use 1 frame at a time? That seems bad. > > > > Yeah. plugin can only access one frame at a time with the ring buffer. I > > realized the issue, after the circluar buffer is implemented. :( > > It is not good for multi-frame processing applications. They have to copy > frames > > to local buffers. Do you think is it OK? Otherwise, we have to use different > > data structure. Thought? > We definitely need at least 2 frames to be active, or there's really no point in > having a ring buffer, right? A place for the producer to write, and a place for > the consumer to read. (Maybe I haven't understood.) Because producer and consumer will use different CircularBuffer instances (which point to the same underlying shared memory buffer), so they can lock separately. Right? That being said, it seems helpful if we allow the reader/the writer to access multiple frames at the same time. Maybe we could consider it later, it won't require too much change to the Pepper API. For example, we just remove/loosen the restrictions that PPB_MediaStreamVideoTrack.RecycleFrame() must be called before the next PPB_MediaStreamVideoTrack.GetFrame(). > > It does not allow both reading and writing on the same side. For input side, > > Read() will fail. Same Write() will fail for output side. Why keep them in one > > class is for avoiding many duplicated code. I like the idea of avoiding duplicate code. But I don't think we should sacrifice readability to achieve that. I think we can use shared impl to reduce duplicate code but expose different interfaces for read/write. What do you think? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
