Re-land: content: Single process support for native GpuMemoryBuffers.

Re-land: content: Single process support for native GpuMemoryBuffers.

This adds single process support for IOSurface and
SurfaceTexture backed GpuMemoryBuffers.

Also makes the SurfaceTexture code more consistent
with IOSurface code by using a singleton instead of
leaking a raw pointer.

BUG=497559
TEST=content_browsertests --gtest_also_run_disabled_tests --gtest_filter=ChildThreadImplGpuMemoryBufferBrowserTests/*

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

[reveman, 2015-05-01 00:28:57]

reveman@chromium.org changed reviewers:
+ dcastagna@chromium.org

[Daniele Castagna, 2015-05-01 20:11:42]

On 2015/05/01 at 00:28:58, reveman wrote:
> 

Can you describe the patch a little bit more in the description?
Tnx.

[reveman, 2015-05-01 20:41:02]

On 2015/05/01 at 20:11:42, dcastagna wrote:
> On 2015/05/01 at 00:28:58, reveman wrote:
> > 
> 
> Can you describe the patch a little bit more in the description?
> Tnx.

Done.

[reveman, 2015-05-06 18:11:51]

ping for review

[Daniele Castagna, 2015-05-06 18:39:41]

The test LGTM.

I'd try to minimize the changes in browser/browser_main_loop.cc moving the code
somewhere else since it's primarily going to be used for testing IIUC.

https://codereview.chromium.org/1120873002/diff/40001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1120873002/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:160: #if defined(OS_ANDROID)
Could this class be somewhere else?

https://codereview.chromium.org/1120873002/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:190: typedef base::ScopedPtrHashMap<int,
scoped_ptr<gfx::ScopedJavaSurface>>
Is there something like a ConcurrentMap so you can avoid to explicitly use a
lock?

https://codereview.chromium.org/1120873002/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:620: if
(parsed_command_line_.HasSwitch(switches::kSingleProcess)) {
This shouldn't be true on any real platform, right?

https://codereview.chromium.org/1120873002/diff/40001/content/child/child_thr...
File content/child/child_thread_impl_browsertest.cc (right):

https://codereview.chromium.org/1120873002/diff/40001/content/child/child_thr...
content/child/child_thread_impl_browsertest.cc:126: enum NativeBufferFlag {
kDisableNativeBuffers, kEnableNativeBuffers };
Should this be in an unnamed namespace?

https://codereview.chromium.org/1120873002/diff/40001/content/child/child_thr...
content/child/child_thread_impl_browsertest.cc:136: switch
(::testing::get<0>(GetParam())) {
Maybe 
NativeBufferFlag native_buffer_flag = ::testing::get<0>(GetParam());
switch (native_buffer_flag) {
would make it a little bit clearer you're getting the first param of the
combination.

https://codereview.chromium.org/1120873002/diff/40001/content/child/child_thr...
content/child/child_thread_impl_browsertest.cc:162: ASSERT_TRUE(rv);
Why ASSERT here? EXPECT is typically preferred.

[reveman, 2015-05-06 20:37:47]

https://codereview.chromium.org/1120873002/diff/40001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1120873002/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:160: #if defined(OS_ANDROID)
On 2015/05/06 at 18:39:41, Daniele Castagna wrote:
> Could this class be somewhere else?

I'm Ok with leaving it here or moving it somewhere else as long as we can be
consistent with how we handle things on MacOSX and upcoming IOSurfaceManager
changes.

https://codereview.chromium.org/1120873002/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:190: typedef base::ScopedPtrHashMap<int,
scoped_ptr<gfx::ScopedJavaSurface>>
On 2015/05/06 at 18:39:41, Daniele Castagna wrote:
> Is there something like a ConcurrentMap so you can avoid to explicitly use a
lock?

Not that I know.

https://codereview.chromium.org/1120873002/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:620: if
(parsed_command_line_.HasSwitch(switches::kSingleProcess)) {
On 2015/05/06 at 18:39:41, Daniele Castagna wrote:
> This shouldn't be true on any real platform, right?

It's always the true on Android Webview and it will of course be true on all
other platform if you pass --single-process flag to chrome.

https://codereview.chromium.org/1120873002/diff/40001/content/child/child_thr...
File content/child/child_thread_impl_browsertest.cc (right):

https://codereview.chromium.org/1120873002/diff/40001/content/child/child_thr...
content/child/child_thread_impl_browsertest.cc:126: enum NativeBufferFlag {
kDisableNativeBuffers, kEnableNativeBuffers };
On 2015/05/06 at 18:39:41, Daniele Castagna wrote:
> Should this be in an unnamed namespace?

Moved everything into anonymous namespace

https://codereview.chromium.org/1120873002/diff/40001/content/child/child_thr...
content/child/child_thread_impl_browsertest.cc:136: switch
(::testing::get<0>(GetParam())) {
On 2015/05/06 at 18:39:41, Daniele Castagna wrote:
> Maybe 
> NativeBufferFlag native_buffer_flag = ::testing::get<0>(GetParam());
> switch (native_buffer_flag) {
> would make it a little bit clearer you're getting the first param of the
combination.

Done.

https://codereview.chromium.org/1120873002/diff/40001/content/child/child_thr...
content/child/child_thread_impl_browsertest.cc:162: ASSERT_TRUE(rv);
On 2015/05/06 at 18:39:41, Daniele Castagna wrote:
> Why ASSERT here? EXPECT is typically preferred.

We're going to crash below if this failed. ASSERT is preferred in this case.

[Daniele Castagna, 2015-05-06 20:46:27]

https://codereview.chromium.org/1120873002/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1120873002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:182:
DCHECK(surfaces_.find(surface_texture_id) != surfaces_.end());
nit: DCHECK_EQ should be preferred.

https://codereview.chromium.org/1120873002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:188:
DCHECK(surfaces_.find(surface_texture_id) != surfaces_.end());
Same here: DCHECK_NE

https://codereview.chromium.org/1120873002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:191: env,
surfaces_.get(surface_texture_id)->j_surface().obj());
You can use surfaces_->operator[] if you prefer.

[reveman, 2015-06-08 22:20:49]

ptal

https://codereview.chromium.org/1120873002/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1120873002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:182:
DCHECK(surfaces_.find(surface_texture_id) != surfaces_.end());
On 2015/05/06 at 20:46:26, Daniele Castagna wrote:
> nit: DCHECK_EQ should be preferred.

doesn't work with that macro

https://codereview.chromium.org/1120873002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:188:
DCHECK(surfaces_.find(surface_texture_id) != surfaces_.end());
On 2015/05/06 at 20:46:26, Daniele Castagna wrote:
> Same here: DCHECK_NE

same

https://codereview.chromium.org/1120873002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:191: env,
surfaces_.get(surface_texture_id)->j_surface().obj());
On 2015/05/06 at 20:46:26, Daniele Castagna wrote:
> You can use surfaces_->operator[] if you prefer.

kept use get() of get to be consistent with code in content_test_suite.cc

[Daniele Castagna, 2015-06-09 14:45:07]

https://codereview.chromium.org/1120873002/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1120873002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:177: class InProcessSurfaceTextureManager :
public SurfaceTextureManager {
I still think these two classes should live somewhere else.

https://codereview.chromium.org/1120873002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:190:
DCHECK(surface_textures_.find(surface_texture_id) ==
nit: DCHECK(!bla.contains()).

https://codereview.chromium.org/1120873002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:199:
DCHECK(surface_textures_.find(surface_texture_id) !=
nit: same as above.

[reveman, 2015-06-09 17:48:13]

reveman@chromium.org changed reviewers:
+ avi@chromium.org

[reveman, 2015-06-09 17:48:14]

+avi

https://codereview.chromium.org/1120873002/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1120873002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:177: class InProcessSurfaceTextureManager :
public SurfaceTextureManager {
On 2015/06/09 at 14:45:07, Daniele Castagna wrote:
> I still think these two classes should live somewhere else.

Done.

https://codereview.chromium.org/1120873002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:190:
DCHECK(surface_textures_.find(surface_texture_id) ==
On 2015/06/09 at 14:45:07, Daniele Castagna wrote:
> nit: DCHECK(!bla.contains()).

Kept the existing DCHECK to be consistent with Browser*Manager implementations.

https://codereview.chromium.org/1120873002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:199:
DCHECK(surface_textures_.find(surface_texture_id) !=
On 2015/06/09 at 14:45:06, Daniele Castagna wrote:
> nit: same as above.

ditto

[Avi (use Gerrit), 2015-06-09 22:05:38]

lgtm

https://codereview.chromium.org/1120873002/diff/100001/content/browser/androi...
File content/browser/android/browser_surface_texture_manager.cc (right):

https://codereview.chromium.org/1120873002/diff/100001/content/browser/androi...
content/browser/android/browser_surface_texture_manager.cc:139:
SurfaceTexturePeer::InitInstance(NULL);
nullptr

https://codereview.chromium.org/1120873002/diff/100001/content/browser/androi...
File content/browser/android/in_process_surface_texture_manager.cc (right):

https://codereview.chromium.org/1120873002/diff/100001/content/browser/androi...
content/browser/android/in_process_surface_texture_manager.cc:67:
SurfaceTexturePeer::InitInstance(NULL);
nullptr

[reveman, 2015-06-09 22:15:15]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-06-09 22:15:16]

https://codereview.chromium.org/1120873002/diff/100001/content/browser/androi...
File content/browser/android/browser_surface_texture_manager.cc (right):

https://codereview.chromium.org/1120873002/diff/100001/content/browser/androi...
content/browser/android/browser_surface_texture_manager.cc:139:
SurfaceTexturePeer::InitInstance(NULL);
On 2015/06/09 at 22:05:38, Avi wrote:
> nullptr

Done.

https://codereview.chromium.org/1120873002/diff/100001/content/browser/androi...
File content/browser/android/in_process_surface_texture_manager.cc (right):

https://codereview.chromium.org/1120873002/diff/100001/content/browser/androi...
content/browser/android/in_process_surface_texture_manager.cc:67:
SurfaceTexturePeer::InitInstance(NULL);
On 2015/06/09 at 22:05:38, Avi wrote:
> nullptr

Done.

[reveman, 2015-06-09 22:15:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1120873002/#ps120001
(title: "nullptr")

[commit-bot: I haz the power, 2015-06-09 22:16:58]

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

[commit-bot: I haz the power, 2015-06-09 23:14:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-09 23:14:36]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[reveman, 2015-06-09 23:25:30]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-06-09 23:25:31]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
dcastagna@chromium.org
Link to the patchset: https://codereview.chromium.org/1120873002/#ps140001
(title: "CONTENT_EXPORT")

[commit-bot: I haz the power, 2015-06-09 23:28:03]

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

[commit-bot: I haz the power, 2015-06-10 00:48:37]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-06-10 00:49:30]

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

[kjellander_chromium, 2015-06-10 07:33:20]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1171293005/ by kjellander@chromium.org.

The reason for reverting is: Breaks Mac10.6 Tests:
https://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/builds/3331.

[dshwang, 2015-06-10 13:34:29]

Hi, can I ask some novice question? Why don't we reuse
browser_surface_texture_manager.h in --single-process mode?
As far as I understand, in_process_surface_texture_manager.h has optimization to
avoid IPC and system call, but is this optimization for --single-process to
worth to set off maintenance cost?

[reveman, 2015-06-10 14:25:26]

On 2015/06/10 at 13:34:29, dongseong.hwang wrote:
> Hi, can I ask some novice question? Why don't we reuse
browser_surface_texture_manager.h in --single-process mode?
> As far as I understand, in_process_surface_texture_manager.h has optimization
to avoid IPC and system call, but is this optimization for --single-process to
worth to set off maintenance cost?

I would have avoided this special case for single process mode if I could. We
don't have a child process launcher service in single process mode and that
prevents our multi process surface texture code from working there.

I had the iosurface code working but it was much cleaner and easier to
understand if the iosurface code was consistent with our surface texture code.

[reveman, 2015-06-10 20:41:04]

FYI, I'm going to re-land this with the new test disabled by default as I don't
want these single process gpu service problems that are unrelated to the
SurfaceTextures and IOSurfaces code to block the use of IOSurface in
multi-process mode.

I'll investigate these issues and try to have the test turned on by default
asap.

[reveman, 2015-06-10 20:41:19]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-06-10 20:41:20]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
dcastagna@chromium.org
Link to the patchset: https://codereview.chromium.org/1120873002/#ps160001
(title: "disable test for now")

[commit-bot: I haz the power, 2015-06-10 20:42:20]

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

[vignatti (out of this project), 2015-06-10 21:49:15]

BTW what's expected from these tests in CrOS? Right now none of the tests in
child_thread_impl_browsertest.cc work and I found that
ChildThreadImplBrowserTest may be interesting for building test cases for the
upcoming native GBM based implementation.

[reveman, 2015-06-10 21:55:37]

On 2015/06/10 at 21:49:15, tiago.vignatti wrote:
> BTW what's expected from these tests in CrOS? Right now none of the tests in
child_thread_impl_browsertest.cc work and I found that
ChildThreadImplBrowserTest may be interesting for building test cases for the
upcoming native GBM based implementation.

They are expected to pass if you're on a device with a working gpu service in
single process mode. What do you mean by none of the tests work on CrOS? They
should work and pass. Any GBM related support should be exercised by these tests
and passing them is a requirement to land any such code.

[commit-bot: I haz the power, 2015-06-10 22:42:04]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-06-10 22:43:03]

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

[vignatti (out of this project), 2015-06-12 21:05:45]

On 2015/06/10 21:55:37, reveman wrote:
> On 2015/06/10 at 21:49:15, tiago.vignatti wrote:
> > BTW what's expected from these tests in CrOS? Right now none of the tests in
> child_thread_impl_browsertest.cc work and I found that
> ChildThreadImplBrowserTest may be interesting for building test cases for the
> upcoming native GBM based implementation.
> 
> They are expected to pass if you're on a device with a working gpu service in
> single process mode. What do you mean by none of the tests work on CrOS? They
> should work and pass. Any GBM related support should be exercised by these
tests
> and passing them is a requirement to land any such code.

you're right reveman. I've double-checked now and the tests themselves are
working fine under GBM (and falling back like expected to shared memory buffer).
I got confused with their output though, because they print some error messages
like the following:

[23441:23467:0612/135911:525564198:ERROR:gpu_channel_host.cc(180)]
GpuChannelHost::CreateViewCommandBuffer failed.
[23441:23467:0612/135911:525564359:ERROR:webgraphicscontext3d_command_buffer_impl.cc(229)]
Failed to initialize command buffer.
[23441:23449:0612/135911:525567081:ERROR:channel.cc(307)] RawChannel read error
(connection broken)

these messages happen after the tests are run, so the exit path is somewhat
broken and breaking the sequence of tests to run in a row when in
--single-process mode. Anyhow, this is a minor thing just.