ppapi: add helper methods to MediaStreamBufferManager

ppapi: MediaStreamBufferManager: expose underlying shared memory buffer

BUG=417589
TEST=none

Committed: https://crrev.com/ca697c4cbcdb839adc3119a05ce2a5058e6454d6
Cr-Commit-Position: refs/heads/master@{#313933}

[llandwerlin-old, 2015-01-28 13:39:11]

lionel.g.landwerlin@intel.com changed reviewers:
+ bbudge@chromium.org

[llandwerlin-old, 2015-01-28 13:39:11]

Some utility methods for the coming PPB_VideoEncoder.

[bbudge, 2015-01-28 18:52:48]

Could you add more detail to the issue description? You're exposing some
internals here and it would be helpful to know why it's necessary.

[bbudge, 2015-01-28 22:05:18]

On 2015/01/28 18:52:48, bbudge wrote:
> Could you add more detail to the issue description? You're exposing some
> internals here and it would be helpful to know why it's necessary.

Specifically, why do you need to add ContainsBuffer? It seems like a client
could determine this by storing buffers it got from the manager in a
std::hash_set and querying that.

[llandwerlin-old, 2015-01-28 23:10:10]

On 2015/01/28 22:05:18, bbudge wrote:
> On 2015/01/28 18:52:48, bbudge wrote:
> > Could you add more detail to the issue description? You're exposing some
> > internals here and it would be helpful to know why it's necessary.
> 
> Specifically, why do you need to add ContainsBuffer? It seems like a client
> could determine this by storing buffers it got from the manager in a
> std::hash_set and querying that.

Yes, that's probably a better way to do this, thanks.

[llandwerlin-old, 2015-01-29 12:42:44]

On 2015/01/28 23:10:10, llandwerlin wrote:
> On 2015/01/28 22:05:18, bbudge wrote:
> > On 2015/01/28 18:52:48, bbudge wrote:
> > > Could you add more detail to the issue description? You're exposing some
> > > internals here and it would be helpful to know why it's necessary.
> > 
> > Specifically, why do you need to add ContainsBuffer? It seems like a client
> > could determine this by storing buffers it got from the manager in a
> > std::hash_set and querying that.
> 
> Yes, that's probably a better way to do this, thanks.

Here is a much simpler version, just adding an accessor to the shared memory
buffer.

[bbudge, 2015-01-29 13:44:31]

https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
File ppapi/shared_impl/media_stream_buffer_manager.h (right):

https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
ppapi/shared_impl/media_stream_buffer_manager.h:56: base::SharedMemory* shm() {
return shm_.get(); }
Do you need the actual SharedMemory object, or just where it Mapped into memory?

[llandwerlin-old, 2015-01-29 13:46:15]

On 2015/01/29 13:44:31, bbudge wrote:
>
https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
> File ppapi/shared_impl/media_stream_buffer_manager.h (right):
> 
>
https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
> ppapi/shared_impl/media_stream_buffer_manager.h:56: base::SharedMemory* shm()
{
> return shm_.get(); }
> Do you need the actual SharedMemory object, or just where it Mapped into
memory?

I need the SharedMemoryHandle to pass it to the renderer.

[bbudge, 2015-01-29 13:52:18]

On 2015/01/29 13:44:31, bbudge wrote:
>
https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
> File ppapi/shared_impl/media_stream_buffer_manager.h (right):
> 
>
https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
> ppapi/shared_impl/media_stream_buffer_manager.h:56: base::SharedMemory* shm()
{
> return shm_.get(); }
> Do you need the actual SharedMemory object, or just where it Mapped into
memory?

Don't you pass it in to this class?

https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...

[llandwerlin-old, 2015-01-29 13:55:30]

On 2015/01/29 13:52:18, bbudge wrote:
> On 2015/01/29 13:44:31, bbudge wrote:
> >
>
https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
> > File ppapi/shared_impl/media_stream_buffer_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
> > ppapi/shared_impl/media_stream_buffer_manager.h:56: base::SharedMemory*
shm()
> {
> > return shm_.get(); }
> > Do you need the actual SharedMemory object, or just where it Mapped into
> memory?
> 
> Don't you pass it in to this class?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...

That's true, but I'm not supposed to keep the reference to it, as it's a
scoped_ptr. It will be owned by the MediaStreamBufferManager.
Also in the future it will be used to access PPB_VideoFrame created within the
PPB_MediaStreamVideoTrack.

[bbudge, 2015-01-29 21:58:27]

On 2015/01/29 13:55:30, llandwerlin wrote:
> On 2015/01/29 13:52:18, bbudge wrote:
> > On 2015/01/29 13:44:31, bbudge wrote:
> > >
> >
>
https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
> > > File ppapi/shared_impl/media_stream_buffer_manager.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
> > > ppapi/shared_impl/media_stream_buffer_manager.h:56: base::SharedMemory*
> shm()
> > {
> > > return shm_.get(); }
> > > Do you need the actual SharedMemory object, or just where it Mapped into
> > memory?
> > 
> > Don't you pass it in to this class?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...
> 
> That's true, but I'm not supposed to keep the reference to it, as it's a
> scoped_ptr. It will be owned by the MediaStreamBufferManager.
> Also in the future it will be used to access PPB_VideoFrame created within the
> PPB_MediaStreamVideoTrack.

Do you create the SharedMemoryHandle for the base::SharedMemory instance? If so,
you could grab it at that point. I'm assuming your host class will own the MSBM
so you don't have to worry about the handle being closed out from under you
(though if that happens, nothing would work.)

I'm trying to avoid leaking this information from MSBM so clients have to go
through the buffer interface rather than getting to memory directly.

[llandwerlin-old, 2015-01-30 08:41:50]

On 2015/01/29 21:58:27, bbudge wrote:
> On 2015/01/29 13:55:30, llandwerlin wrote:
> > On 2015/01/29 13:52:18, bbudge wrote:
> > > On 2015/01/29 13:44:31, bbudge wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
> > > > File ppapi/shared_impl/media_stream_buffer_manager.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
> > > > ppapi/shared_impl/media_stream_buffer_manager.h:56: base::SharedMemory*
> > shm()
> > > {
> > > > return shm_.get(); }
> > > > Do you need the actual SharedMemory object, or just where it Mapped into
> > > memory?
> > > 
> > > Don't you pass it in to this class?
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...
> > 
> > That's true, but I'm not supposed to keep the reference to it, as it's a
> > scoped_ptr. It will be owned by the MediaStreamBufferManager.
> > Also in the future it will be used to access PPB_VideoFrame created within
the
> > PPB_MediaStreamVideoTrack.
> 
> Do you create the SharedMemoryHandle for the base::SharedMemory instance? If
so,
> you could grab it at that point. I'm assuming your host class will own the
MSBM
> so you don't have to worry about the handle being closed out from under you
> (though if that happens, nothing would work.)

In the case where frames are allocated by the PepperVideoEncoderHost, the
VideoEncoderResource receives the base::SharedMemoryHandle. So I could store
this and do not need it from the MSBM.
For now, it's only convenient to have all the data in one place.

But if we allow the user to pass VideoFrameResouce created by the
MediaStreamVideoTrackResource, I would need a way to get this
base::SharedMemoryHandle. 

> 
> I'm trying to avoid leaking this information from MSBM so clients have to go
> through the buffer interface rather than getting to memory directly.

I'm not sure to understand what you would like to avoid to leak. Unless you
would like to make MSBM work with non shared memory buffers, its api explicitly
requires a base::SharedMemory to do the setup. So where is the harm in exposing
it?

I don't really need this change for the first iteration of the encoder api. It
just makes things cleaner.
Maybe we can wait a bit, and I will upload something more substantial (to
support encoding frames from MediaStreamVideoTrack) so you can see how it all
fits together.

[bbudge, 2015-01-30 13:56:53]

On 2015/01/30 08:41:50, llandwerlin wrote:
> On 2015/01/29 21:58:27, bbudge wrote:
> > On 2015/01/29 13:55:30, llandwerlin wrote:
> > > On 2015/01/29 13:52:18, bbudge wrote:
> > > > On 2015/01/29 13:44:31, bbudge wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
> > > > > File ppapi/shared_impl/media_stream_buffer_manager.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/884723002/diff/20001/ppapi/shared_impl/media_...
> > > > > ppapi/shared_impl/media_stream_buffer_manager.h:56:
base::SharedMemory*
> > > shm()
> > > > {
> > > > > return shm_.get(); }
> > > > > Do you need the actual SharedMemory object, or just where it Mapped
into
> > > > memory?
> > > > 
> > > > Don't you pass it in to this class?
> > > > 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...
> > > 
> > > That's true, but I'm not supposed to keep the reference to it, as it's a
> > > scoped_ptr. It will be owned by the MediaStreamBufferManager.
> > > Also in the future it will be used to access PPB_VideoFrame created within
> the
> > > PPB_MediaStreamVideoTrack.
> > 
> > Do you create the SharedMemoryHandle for the base::SharedMemory instance? If
> so,
> > you could grab it at that point. I'm assuming your host class will own the
> MSBM
> > so you don't have to worry about the handle being closed out from under you
> > (though if that happens, nothing would work.)
> 
> In the case where frames are allocated by the PepperVideoEncoderHost, the
> VideoEncoderResource receives the base::SharedMemoryHandle. So I could store
> this and do not need it from the MSBM.
> For now, it's only convenient to have all the data in one place.
> 
> But if we allow the user to pass VideoFrameResouce created by the
> MediaStreamVideoTrackResource, I would need a way to get this
> base::SharedMemoryHandle. 

OK, that makes sense. I wasn't thinking of that use case.

> 
> > 
> > I'm trying to avoid leaking this information from MSBM so clients have to go
> > through the buffer interface rather than getting to memory directly.
> 
> I'm not sure to understand what you would like to avoid to leak. Unless you
> would like to make MSBM work with non shared memory buffers, its api
explicitly
> requires a base::SharedMemory to do the setup. So where is the harm in
exposing
> it?

I'm worried that it makes the API more complex, since base::SharedMemory has its
own API. I want to be sure that this change is really necessary. 

> 
> I don't really need this change for the first iteration of the encoder api. It
> just makes things cleaner.
> Maybe we can wait a bit, and I will upload something more substantial (to
> support encoding frames from MediaStreamVideoTrack) so you can see how it all
> fits together.

[bbudge, 2015-01-30 13:57:14]

OK, LGTM.

[llandwerlin-old, 2015-01-30 16:33:02]

The CQ bit was checked by lionel.g.landwerlin@intel.com

[commit-bot: I haz the power, 2015-01-30 16:33:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/884723002/20001

[commit-bot: I haz the power, 2015-01-30 17:28:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-30 17:29:03]

Patchset 2 (id:??) landed as
https://crrev.com/ca697c4cbcdb839adc3119a05ce2a5058e6454d6
Cr-Commit-Position: refs/heads/master@{#313933}