Reuse PPB_Buffer_Impl in ContentDecryptorDelegate.

Reuse PPB_Buffer_Impl in ContentDecryptorDelegate.

PPB_Buffer_Impl wraps shared memory which is expensive to allocate. Allocating shared memory involved an sync IPC to to browser. To reduce the latency caused by this, reuse PPB_Buffer_Impl in ContentDecryptorDelegate for audio and video streams.

Test on Linux (Ubuntu 10.04) shows that this change reduces the time to MakeBufferResource() from 0.543ms to 0.040 ms per frame. The difference can be larger on slower platforms.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168632

[xhwang, 2012-11-17 01:27:58]

PTAL!

[ddorwin, 2012-11-17 02:07:37]

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:342: //
|{audio|video}_input_resource_size_| must be available here because there
What do you mean by "available"?

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:560: if
(!MakeBufferResource(pp_instance_,//media::Decryptor::kVideo,
Huh? Why not MakeMediaBufferResource?

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:929: media_resource_size =
kInitialMediaBufferSize;
Are you just trying to make it a power of 2? If so, comment that. Also, maybe
s/Initial/Minimum since it may not be the initial size.

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:932: media_resource_size *=
2;
Should we just do pow(ceiling(log(size)))?

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:942: }
If any of the above fails, we need to reset the appropriate media_resource_size
(to 0 since we destroyed the old buffer).

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:944: if
(!media_resource.get())
This can never fail outside the bracket at 942 unless we have a bug. Suggest
moving it up and DCHECKing if you want.

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
File webkit/plugins/ppapi/content_decryptor_delegate.h (right):

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.h:102: // copies |data| into the
buffer resource. This method reuses
Why "create" and copy in a single function? First, copy isn't really part of
"Make". Second, we'll have to break this up later when we try to eliminate the
memcpy.
Update: Oh, this is the way the existing function is. I guess we should stay
consistent for now.

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.h:142: // ScopedPPResource for
audio and video input buffers.
Maybe add
  // See MakeMediaBufferResource.
for an explanation

[xhwang, 2012-11-18 00:10:33]

PTAL again!

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:342: //
|{audio|video}_input_resource_size_| must be available here because there
On 2012/11/17 02:07:38, ddorwin wrote:
> What do you mean by "available"?

Done here and elsewhere.

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:560: if
(!MakeBufferResource(pp_instance_,//media::Decryptor::kVideo,
On 2012/11/17 02:07:38, ddorwin wrote:
> Huh? Why not MakeMediaBufferResource?

Oops, I used this to test how much gain I have and forgot to change it back.
Thanks for catching it. Done.

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:929: media_resource_size =
kInitialMediaBufferSize;
On 2012/11/17 02:07:38, ddorwin wrote:
> Are you just trying to make it a power of 2? If so, comment that. Also, maybe
> s/Initial/Minimum since it may not be the initial size.

It can really just be any size. Use power of 2 here only for potential alignment
benefits. Also, double the size below every time to make it grow exponentially.
Comments added.

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:932: media_resource_size *=
2;
On 2012/11/17 02:07:38, ddorwin wrote:
> Should we just do pow(ceiling(log(size)))?

Using a while loop and double the size (shift one bit) should be more efficient
than introducing math functions.

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:942: }
On 2012/11/17 02:07:38, ddorwin wrote:
> If any of the above fails, we need to reset the appropriate
media_resource_size
> (to 0 since we destroyed the old buffer).

Added reset of media_resource and media_resource_size in the following failure
cases. But I feel this is a little overkill because if we return false in this
function, the playback will be terminated and everything will be cleared later.
WDYT?

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:944: if
(!media_resource.get())
On 2012/11/17 02:07:38, ddorwin wrote:
> This can never fail outside the bracket at 942 unless we have a bug. Suggest
> moving it up and DCHECKing if you want.

This can happen for new created buffer. Move this block after 941.

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
File webkit/plugins/ppapi/content_decryptor_delegate.h (right):

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.h:102: // copies |data| into the
buffer resource. This method reuses
On 2012/11/17 02:07:38, ddorwin wrote:
> Why "create" and copy in a single function? First, copy isn't really part of
> "Make". Second, we'll have to break this up later when we try to eliminate the
> memcpy.
> Update: Oh, this is the way the existing function is. I guess we should stay
> consistent for now.

This memcpy is for input, which is usually small. Also, if we want to remove the
memcpy, we have to preallocate memory and pass it into the demuxers. I feel it's
not worth the effort.

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.h:142: // ScopedPPResource for
audio and video input buffers.
On 2012/11/17 02:07:38, ddorwin wrote:
> Maybe add
>   // See MakeMediaBufferResource.
> for an explanation

Done.

[ddorwin, 2012-11-18 00:37:19]

LGTM % nit. Thanks.

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:942: }
On 2012/11/18 00:10:33, xhwang wrote:
> On 2012/11/17 02:07:38, ddorwin wrote:
> > If any of the above fails, we need to reset the appropriate
> media_resource_size
> > (to 0 since we destroyed the old buffer).
> 
> Added reset of media_resource and media_resource_size in the following failure
> cases. But I feel this is a little overkill because if we return false in this
> function, the playback will be terminated and everything will be cleared
later.
> WDYT?

It's better to have a unit of code function correctly without making assumptions
about the caller.

Another option is to make a copy of the value and have a scoped resource then
only assign them to the members at 968.

http://codereview.chromium.org/11411053/diff/5001/webkit/plugins/ppapi/conten...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

http://codereview.chromium.org/11411053/diff/5001/webkit/plugins/ppapi/conten...
webkit/plugins/ppapi/content_decryptor_delegate.cc:340: //
|{audio|video}_input_resource_size_| is not being used by the plugin
s/size_/[size_]/

[xhwang, 2012-11-19 19:39:24]

dmichael@: could you please do a OWNERS review?

[dmichael (off chromium), 2012-11-19 20:11:35]

lgtm

[xhwang, 2012-11-19 21:16:34]

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

http://codereview.chromium.org/11411053/diff/1/webkit/plugins/ppapi/content_d...
webkit/plugins/ppapi/content_decryptor_delegate.cc:942: }
On 2012/11/18 00:37:19, ddorwin wrote:
> On 2012/11/18 00:10:33, xhwang wrote:
> > On 2012/11/17 02:07:38, ddorwin wrote:
> > > If any of the above fails, we need to reset the appropriate
> > media_resource_size
> > > (to 0 since we destroyed the old buffer).
> > 
> > Added reset of media_resource and media_resource_size in the following
failure
> > cases. But I feel this is a little overkill because if we return false in
this
> > function, the playback will be terminated and everything will be cleared
> later.
> > WDYT?
> 
> It's better to have a unit of code function correctly without making
assumptions
> about the caller.

okay
> 
> Another option is to make a copy of the value and have a scoped resource then
> only assign them to the members at 968.

http://codereview.chromium.org/11411053/diff/5001/webkit/plugins/ppapi/conten...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

http://codereview.chromium.org/11411053/diff/5001/webkit/plugins/ppapi/conten...
webkit/plugins/ppapi/content_decryptor_delegate.cc:340: //
|{audio|video}_input_resource_size_| is not being used by the plugin
On 2012/11/18 00:37:19, ddorwin wrote:
> s/size_/[size_]/

Done.

[commit-bot: I haz the power, 2012-11-19 21:17:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/11411053/6

[commit-bot: I haz the power, 2012-11-19 23:33:33]

Change committed as 168632