gpu: Add GpuMemoryBufferFactory interface.

gpu: Add GpuMemoryBufferFactory interface.

Used for in-process GpuMemoryBuffer creation.

TEST=gpu_unittests --gtest_filter=MockGpuMemoryBufferTest.Lifecycle
BUG=261649
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214136

[reveman, 2013-07-26 05:00:39]

Please take a look. This is a prerequisite to:
https://codereview.chromium.org/20017005/

[joth, 2013-07-26 05:28:05]

aw/ lgtm, but would be good to have kaan's eye over it too.

https://codereview.chromium.org/20658002/diff/1/gpu/command_buffer/client/gpu...
File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/20658002/diff/1/gpu/command_buffer/client/gpu...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:15: virtual
gpu::GpuMemoryBuffer* CreateGpuMemoryBuffer(
Out of interest why does this need to be a factory now? In previous review we
had it as factory but were told as it was only one method it is preferable to
use callback.

As base::Bind() supports currying and attachment to member functions, you can
have a stateful factory with either approach.

[kaanb, 2013-07-26 05:59:13]

lgtm with a few suggestions

Thanks for breaking up the mega patch, it is much easier to follow what's going
on now.

https://codereview.chromium.org/20658002/diff/1/android_webview/browser/gpu_m...
File android_webview/browser/gpu_memory_buffer_factory_impl.cc (right):

https://codereview.chromium.org/20658002/diff/1/android_webview/browser/gpu_m...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:89:
DCHECK_EQ(static_cast<GLenum>(GL_RGBA8_OES), internalformat);
Checking the same condition in DCHECK and if is a bit odd. Could you move the
DCHECK within the if?

https://codereview.chromium.org/20658002/diff/1/android_webview/browser/gpu_m...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:95: if (!buffer_id)
consider using the ternary conditional operator:

return (buffer_id) ? new GpuMemoryBufferImpl(...) : NULL;

https://codereview.chromium.org/20658002/diff/1/gpu/command_buffer/client/gl_...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/20658002/diff/1/gpu/command_buffer/client/gl_...
gpu/command_buffer/client/gl_in_process_context.cc:597: #if !defined(NDEBUG)
isn't it possible to replace this with a function? something like
DCHECK_IS_ON()? I guess we only want this check when DEBUG mode is enabled, but
it's very hard to read.

[reveman, 2013-07-26 15:40:57]

+piman for owner review

https://codereview.chromium.org/20658002/diff/1/android_webview/browser/gpu_m...
File android_webview/browser/gpu_memory_buffer_factory_impl.cc (right):

https://codereview.chromium.org/20658002/diff/1/android_webview/browser/gpu_m...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:89:
DCHECK_EQ(static_cast<GLenum>(GL_RGBA8_OES), internalformat);
On 2013/07/26 05:59:13, kaanb wrote:
> Checking the same condition in DCHECK and if is a bit odd. Could you move the
> DCHECK within the if?

I think this is common in cases where we want a DCHECK but avoid crashing the
user. I could just make this a CHECK instead and remove the if statement if you
prefer that?

https://codereview.chromium.org/20658002/diff/1/android_webview/browser/gpu_m...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:95: if (!buffer_id)
On 2013/07/26 05:59:13, kaanb wrote:
> consider using the ternary conditional operator:
> 
> return (buffer_id) ? new GpuMemoryBufferImpl(...) : NULL;

I prefer the current code as it makes it more clear that (!buffer_id) is an
error.

https://codereview.chromium.org/20658002/diff/1/gpu/command_buffer/client/gl_...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/20658002/diff/1/gpu/command_buffer/client/gl_...
gpu/command_buffer/client/gl_in_process_context.cc:597: #if !defined(NDEBUG)
On 2013/07/26 05:59:13, kaanb wrote:
> isn't it possible to replace this with a function? something like
> DCHECK_IS_ON()? I guess we only want this check when DEBUG mode is enabled,
but
> it's very hard to read.

I don't think there's a good reason to avoid this when DCHECKs are enabled.
Added a SharedContextCount() function which makes this much cleaner.

https://codereview.chromium.org/20658002/diff/1/gpu/command_buffer/client/gpu...
File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/20658002/diff/1/gpu/command_buffer/client/gpu...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:15: virtual
gpu::GpuMemoryBuffer* CreateGpuMemoryBuffer(
On 2013/07/26 05:28:05, joth wrote:
> Out of interest why does this need to be a factory now? In previous review we
> had it as factory but were told as it was only one method it is preferable to
> use callback.
> 
> As base::Bind() supports currying and attachment to member functions, you can
> have a stateful factory with either approach.

This will replace the ImageFactory interface and be passed to the
CommandBufferService in follow up patches. base::Bind() does ref-counting
internally so it's not appropriate for storing in a global variable. We could
just use a raw function pointer as before but I strongly prefer a real interface
as it makes usage clearer and makes it easy to add a DestroyGpuMemoryBuffer
function which is likely needed to keep things clean when adding future buffer
types.

[kaanb, 2013-07-26 16:57:40]

https://codereview.chromium.org/20658002/diff/1/android_webview/browser/gpu_m...
File android_webview/browser/gpu_memory_buffer_factory_impl.cc (right):

https://codereview.chromium.org/20658002/diff/1/android_webview/browser/gpu_m...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:89:
DCHECK_EQ(static_cast<GLenum>(GL_RGBA8_OES), internalformat);
On 2013/07/26 15:40:57, David Reveman wrote:
> On 2013/07/26 05:59:13, kaanb wrote:
> > Checking the same condition in DCHECK and if is a bit odd. Could you move
the
> > DCHECK within the if?
> 
> I think this is common in cases where we want a DCHECK but avoid crashing the
> user. I could just make this a CHECK instead and remove the if statement if
you
> prefer that?

Just making this a CHECK sounds good.

[reveman, 2013-07-26 17:57:37]

https://codereview.chromium.org/20658002/diff/1/android_webview/browser/gpu_m...
File android_webview/browser/gpu_memory_buffer_factory_impl.cc (right):

https://codereview.chromium.org/20658002/diff/1/android_webview/browser/gpu_m...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:89:
DCHECK_EQ(static_cast<GLenum>(GL_RGBA8_OES), internalformat);
On 2013/07/26 16:57:40, kaanb wrote:
> On 2013/07/26 15:40:57, David Reveman wrote:
> > On 2013/07/26 05:59:13, kaanb wrote:
> > > Checking the same condition in DCHECK and if is a bit odd. Could you move
> the
> > > DCHECK within the if?
> > 
> > I think this is common in cases where we want a DCHECK but avoid crashing
the
> > user. I could just make this a CHECK instead and remove the if statement if
> you
> > prefer that?
> 
> Just making this a CHECK sounds good.

Done.

[piman, 2013-07-26 18:02:38]

lgtm

[commit-bot: I haz the power, 2013-07-26 18:43:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/20658002/12001

[commit-bot: I haz the power, 2013-07-28 00:12:52]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-07-28 00:29:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/20658002/12001

[commit-bot: I haz the power, 2013-07-28 02:18:29]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-07-28 17:26:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/20658002/12001

[commit-bot: I haz the power, 2013-07-28 18:44:18]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-07-28 19:01:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/20658002/12001

[commit-bot: I haz the power, 2013-07-28 20:09:25]

Change committed as 214136