Support impl-side painting in Mandoline.

Support impl-side painting in Mandoline.

This is now the default in cc and the old code paths are being removed, so we need to use impl-side painting. The changes are:
-add necessary thunks for the gl methods
-expose gpu::GpuMemoryBufferManager and cc::TaskGraphRunner in ui and html_viewer
-IPC to view_manager and call the GPU code (modelled on content)

BUG=499030
NOPRESUBMIT=true

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

[jam, 2015-06-13 02:39:55]

Patchset #6 (id:100001) has been deleted

[jam, 2015-06-15 17:22:18]

jam@chromium.org changed reviewers:
+ piman@chromium.org, sky@chromium.org

[jam, 2015-06-15 17:23:40]

sky: Please review components/html_viewer and mandoline
piman: the rest

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
File components/view_manager/gles2/mojo_gpu_memory_buffer.h (right):

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/gles2/mojo_gpu_memory_buffer.h:19: ~
MojoGpuMemoryBufferImpl() override;
i saw this in my self review, and will fix it combined with other comments

[jam, 2015-06-15 17:24:02]

also i'll send the mojo side change upstream in
https://codereview.chromium.org/1185963002/

[danakj, 2015-06-15 17:47:33]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-06-15 17:47:34]

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
File components/html_viewer/discardable_memory_allocator.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
components/html_viewer/discardable_memory_allocator.cc:25: DCHECK(data_.get() ||
!is_locked_);
nit: you could DCHECK_IMPLIES(!data_, !is_locked_) might be easier to read?

nit: no .get() needed anyway

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
components/html_viewer/discardable_memory_allocator.cc:26: if (!is_locked_ &&
data_.get())
nit: no get() throughout

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
File components/html_viewer/discardable_memory_allocator.h (right):

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
components/html_viewer/discardable_memory_allocator.h:49: base::Lock lock_;
Could you comment or otherwise make it clear which variables this lock is
protecting?

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
File components/html_viewer/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
components/html_viewer/web_layer_tree_view_impl.cc:35:
settings.impl_side_painting = true;
would be nice to set use_one_copy = true here, or you might be falling into the
async uploads (also deprecated/unused now) path.

You could add a TODO to set the use_image_texture_target and
use_persistent_map_for_gpu_memory_buffers correctly? See
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/comp...

.. or for the renderer
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
and
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

For now can you hardcode them to GL_TEXTURE and false here respectively?

Can you also set gather_pixel_refs?
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/comp...

In content the renderer process has multiple threads and we turn that on, FWIW.
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
.

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
File components/view_manager/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/gles2/command_buffer_driver.cc:238: LOG(ERROR) << "Image
already exists with same ID.";
DLOGs for these?

[piman, 2015-06-15 18:14:34]

LGTM+nits

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:3058: 'extension':
"CHROMIUM_image",
nit: CHROMIUM_pixel_transfer_buffer_object
I realize this extension was not documented, but this is the extension name
that's associated with the functionality in the header.

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:3474: 'extension':
"CHROMIUM_image",
CHROMIUM_pixel_transfer_buffer_object

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:3869: 'extension':
"CHROMIUM_image",
nit: CHROMIUM_async_pixel_transfers (same deal).

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:3884: 'extension':
"CHROMIUM_image",
nit: CHROMIUM_async_pixel_transfers

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:3891: 'extension':
"CHROMIUM_image",
nit: CHROMIUM_async_pixel_transfers

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:3898: 'extension':
"CHROMIUM_image",
nit: CHROMIUM_async_pixel_transfers

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
mojo/gles2/command_buffer_client_impl.cc:294: //                                
                  InsertSyncPoint());
nit: remove/add TODO?

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
File mojo/gles2/command_buffer_client_impl.h (right):

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
mojo/gles2/command_buffer_client_impl.h:107: base::AtomicSequenceNumber
next_image_id_;
nit: shouldn't need to be atomic, this class should be used from a single thread
at a time.

[danakj, 2015-06-15 18:20:36]

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
File components/view_manager/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/gles2/command_buffer_driver.cc:263: if (type !=
gfx::SHARED_MEMORY_BUFFER) {
Could all/some of these early outs be DCHECKs since they are already checked in
mojo/gles2/command_buffer_client_impl.cc ?

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
File components/view_manager/gles2/mojo_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/gles2/mojo_gpu_memory_buffer_manager.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/gles2/mojo_gpu_memory_buffer_manager.cc:23: return
MojoGpuMemoryBufferImpl::Create(size, format, usage);
I would expect to find the SharedMemory creation type stuff over here instead of
inside Create() to be consistent with other implementations. Also the manager
may want to add logic to that over time? Maybe not important right now though.

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
File components/view_manager/gles2/mojo_gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/gles2/mojo_gpu_memory_buffer_manager.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
File components/view_manager/gles2/raster_thread_helper.h (right):

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/gles2/raster_thread_helper.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
File components/view_manager/surfaces/surfaces_context_provider.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/surfaces/surfaces_context_provider.cc:15:
capabilities_.gpu.image = true;
could you maybe add a comment pointing to where this is implemented?

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:3869: 'extension':
"CHROMIUM_image",
On 2015/06/15 18:14:33, piman (Very slow to review) wrote:
> nit: CHROMIUM_async_pixel_transfers (same deal).

And, FWIW if you use_one_copy = true, you probably don't need to add these async
things to the mojo autogens

https://codereview.chromium.org/1181013010/diff/270001/mojo/cc/context_provid...
File mojo/cc/context_provider_mojo.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/mojo/cc/context_provid...
mojo/cc/context_provider_mojo.cc:16: capabilities_.gpu.image = true;
Can you also comment here pointing readers to some context?

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
mojo/gles2/command_buffer_client_impl.cc:293:
//gpu_memory_buffer_manager->SetDestructionSyncPoint(gpu_memory_buffer,
is a TODO needed?

[yzshen1, 2015-06-15 20:04:41]

yzshen@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2015-06-15 20:04:42]

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
mojo/gles2/command_buffer_client_impl.cc:254: size->width = width;
FYI, I tried to compile the patch on Windows and it complained about conversion:

==================
d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(254) : error C2220:
warning treated as error - no 'object' file generated
d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(254) : warning C4267:
'=' : conversion from 'size_t' to 'int32_t', possible loss of data
d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(255) : warning C4267:
'=' : conversion from 'size_t' to 'int32_t', possible loss of data
d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(310) : warning C4267:
'argument' : conversion from 'size_t' to 'int', possible loss of data

[sky, 2015-06-15 21:47:47]

LGTM

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
File components/html_viewer/discardable_memory_allocator.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
components/html_viewer/discardable_memory_allocator.cc:105: return
make_scoped_ptr(chunk);
nit: move scoped_ptr to 89 and return that.

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
File components/html_viewer/discardable_memory_allocator.h (right):

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
components/html_viewer/discardable_memory_allocator.h:49: base::Lock lock_;
On 2015/06/15 17:47:34, danakj wrote:
> Could you comment or otherwise make it clear which variables this lock is
> protecting?

Also document what threads are in use here. That isn't apparent to me.

[jam, 2015-06-15 22:06:02]

btw I've been seeing

[0615/145740:ERROR:gles2_cmd_decoder.cc(12110)]
[GroupMarkerNotSet(crbug.com/242999)!:F890860D]GL ERROR :GL_INVALID_VALUE :
glCopySubTextureCHROMIUM:
source texture has no level 0

when I'm running mandoline now. However I can't tell exactly why this is
happening?

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
File components/html_viewer/discardable_memory_allocator.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
components/html_viewer/discardable_memory_allocator.cc:25: DCHECK(data_.get() ||
!is_locked_);
On 2015/06/15 17:47:34, danakj wrote:
> nit: you could DCHECK_IMPLIES(!data_, !is_locked_) might be easier to read?

I commented on the change that added DCHECK_IMPLIES :)

> 
> nit: no .get() needed anyway

Done

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
components/html_viewer/discardable_memory_allocator.cc:26: if (!is_locked_ &&
data_.get())
On 2015/06/15 17:47:34, danakj wrote:
> nit: no get() throughout

Done.

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
File components/html_viewer/discardable_memory_allocator.h (right):

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
components/html_viewer/discardable_memory_allocator.h:49: base::Lock lock_;
On 2015/06/15 17:47:34, danakj wrote:
> Could you comment or otherwise make it clear which variables this lock is
> protecting?

Done.

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
File components/html_viewer/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
components/html_viewer/web_layer_tree_view_impl.cc:35:
settings.impl_side_painting = true;
On 2015/06/15 17:47:34, danakj wrote:
> would be nice to set use_one_copy = true here, or you might be falling into
the
> async uploads (also deprecated/unused now) path.

Done, thanks.

> 
> You could add a TODO to set the use_image_texture_target and

Done, thanks

> use_persistent_map_for_gpu_memory_buffers correctly? See

left out for now per our discussion since content doesn't set it

>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/comp...
> 
> .. or for the renderer
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> and
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> 
> For now can you hardcode them to GL_TEXTURE and false here respectively?
> 
> Can you also set gather_pixel_refs?
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/comp...
> 
> In content the renderer process has multiple threads and we turn that on,
FWIW.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> .

added comment

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
File components/view_manager/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/gles2/command_buffer_driver.cc:238: LOG(ERROR) << "Image
already exists with same ID.";
On 2015/06/15 17:47:34, danakj wrote:
> DLOGs for these?

II had just copied what content does..

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/gles2/command_buffer_driver.cc:263: if (type !=
gfx::SHARED_MEMORY_BUFFER) {
On 2015/06/15 18:20:35, danakj wrote:
> Could all/some of these early outs be DCHECKs since they are already checked
in
> mojo/gles2/command_buffer_client_impl.cc ?

ditto (copied content)

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
File components/view_manager/gles2/mojo_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/gles2/mojo_gpu_memory_buffer_manager.cc:23: return
MojoGpuMemoryBufferImpl::Create(size, format, usage);
On 2015/06/15 18:20:35, danakj wrote:
> I would expect to find the SharedMemory creation type stuff over here instead
of
> inside Create() to be consistent with other implementations. Also the manager
> may want to add logic to that over time? Maybe not important right now though.

This is how I originally had it, but then this needs to be accessed by
CommandBufferClientImpl::CreateGpuMemoryBufferImage as wel. Mandoline's
CommandBufferClientImpl doesn't have access to MojoGpuMemoryBufferManager since
that's in a different module, so that's why I moved this method implementation
to somewhere else that can be shared.

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
File components/view_manager/gles2/mojo_gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/gles2/mojo_gpu_memory_buffer_manager.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2015/06/15 18:20:35, danakj wrote:
> 2015

Done.

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
File components/view_manager/gles2/raster_thread_helper.h (right):

https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
components/view_manager/gles2/raster_thread_helper.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/06/15 18:20:35, danakj wrote:
> 2015

Done.

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:3058: 'extension':
"CHROMIUM_image",
On 2015/06/15 18:14:33, piman (Very slow to review) wrote:
> nit: CHROMIUM_pixel_transfer_buffer_object
> I realize this extension was not documented, but this is the extension name
> that's associated with the functionality in the header.

Done.

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:3474: 'extension':
"CHROMIUM_image",
On 2015/06/15 18:14:33, piman (Very slow to review) wrote:
> CHROMIUM_pixel_transfer_buffer_object

Done.

https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:3869: 'extension':
"CHROMIUM_image",
On 2015/06/15 18:20:35, danakj wrote:
> On 2015/06/15 18:14:33, piman (Very slow to review) wrote:
> > nit: CHROMIUM_async_pixel_transfers (same deal).
> 
> And, FWIW if you use_one_copy = true, you probably don't need to add these
async
> things to the mojo autogens

set use_one_copy=true, and removed all changes that piman commented to use
CHROMIUM_async_pixel_transfers for

https://codereview.chromium.org/1181013010/diff/270001/mojo/cc/context_provid...
File mojo/cc/context_provider_mojo.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/mojo/cc/context_provid...
mojo/cc/context_provider_mojo.cc:16: capabilities_.gpu.image = true;
On 2015/06/15 18:20:36, danakj wrote:
> Can you also comment here pointing readers to some context?

like what kind of comment? for comparison, content also doesn't really document
this flag or others when it sets them.

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
mojo/gles2/command_buffer_client_impl.cc:254: size->width = width;
On 2015/06/15 20:04:42, yzshen1 wrote:
> FYI, I tried to compile the patch on Windows and it complained about
conversion:
> 
> ==================
> d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(254) : error C2220:
> warning treated as error - no 'object' file generated
> d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(254) : warning C4267:
> '=' : conversion from 'size_t' to 'int32_t', possible loss of data
> d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(255) : warning C4267:
> '=' : conversion from 'size_t' to 'int32_t', possible loss of data
> d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(310) : warning C4267:
> 'argument' : conversion from 'size_t' to 'int', possible loss of data
> 

that's strange, i'm compiling on Windows and I don't see that!
what are your build GN build args?

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
mojo/gles2/command_buffer_client_impl.cc:293:
//gpu_memory_buffer_manager->SetDestructionSyncPoint(gpu_memory_buffer,
On 2015/06/15 18:20:36, danakj wrote:
> is a TODO needed?

Done.

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
mojo/gles2/command_buffer_client_impl.cc:294: //                                
                  InsertSyncPoint());
On 2015/06/15 18:14:33, piman (Very slow to review) wrote:
> nit: remove/add TODO?

Done.

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
File mojo/gles2/command_buffer_client_impl.h (right):

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
mojo/gles2/command_buffer_client_impl.h:107: base::AtomicSequenceNumber
next_image_id_;
On 2015/06/15 18:14:33, piman (Very slow to review) wrote:
> nit: shouldn't need to be atomic, this class should be used from a single
thread
> at a time.

Done.

[danakj, 2015-06-15 22:11:20]

LGTM

https://codereview.chromium.org/1181013010/diff/270001/mojo/cc/context_provid...
File mojo/cc/context_provider_mojo.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/mojo/cc/context_provid...
mojo/cc/context_provider_mojo.cc:16: capabilities_.gpu.image = true;
On 2015/06/15 22:06:01, jam wrote:
> On 2015/06/15 18:20:36, danakj wrote:
> > Can you also comment here pointing readers to some context?
> 
> like what kind of comment? for comparison, content also doesn't really
document
> this flag or others when it sets them.

Something like "Enabled the CHROMIUM_image extension to use GpuMemoryBuffers.
The implementation of which is used in FooClassName."

I wish content/ did too.

[danakj, 2015-06-15 22:12:59]

On 2015/06/15 22:06:02, jam wrote:
> btw I've been seeing
> 
> [0615/145740:ERROR:gles2_cmd_decoder.cc(12110)]
> [GroupMarkerNotSet(crbug.com/242999)!:F890860D]GL ERROR :GL_INVALID_VALUE :
> glCopySubTextureCHROMIUM:
> source texture has no level 0
> 
> when I'm running mandoline now. However I can't tell exactly why this is
> happening?

I'm not sure what that means. Hopefully piman or reveman would know.

[piman, 2015-06-15 22:13:53]

On Mon, Jun 15, 2015 at 3:06 PM, <jam@chromium.org> wrote:

> btw I've been seeing
>
> [0615/145740:ERROR:gles2_cmd_decoder.cc(12110)]
> [GroupMarkerNotSet(crbug.com/242999)!:F890860D]GL
> <http://crbug.com/242999)!:F890860D%5DGL> ERROR :GL_INVALID_VALUE :
> glCopySubTextureCHROMIUM:
> source texture has no level 0
>
> when I'm running mandoline now. However I can't tell exactly why this is
> happening?


It sounds like a legit error, but it's hard to trace with just this. Is it
the only error you get? It sounds like either the
CreateGpuMemoryBufferImage or the BindTexImage2DCHROMIUM failed...

Antoine


>
>
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
> File components/html_viewer/discardable_memory_allocator.cc (right):
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
> components/html_viewer/discardable_memory_allocator.cc:25:
> DCHECK(data_.get() || !is_locked_);
> On 2015/06/15 17:47:34, danakj wrote:
>
>> nit: you could DCHECK_IMPLIES(!data_, !is_locked_) might be easier to
>>
> read?
>
> I commented on the change that added DCHECK_IMPLIES :)
>
>
>  nit: no .get() needed anyway
>>
>
> Done
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
> components/html_viewer/discardable_memory_allocator.cc:26: if
> (!is_locked_ && data_.get())
> On 2015/06/15 17:47:34, danakj wrote:
>
>> nit: no get() throughout
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
> File components/html_viewer/discardable_memory_allocator.h (right):
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
> components/html_viewer/discardable_memory_allocator.h:49: base::Lock
> lock_;
> On 2015/06/15 17:47:34, danakj wrote:
>
>> Could you comment or otherwise make it clear which variables this lock
>>
> is
>
>> protecting?
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
> File components/html_viewer/web_layer_tree_view_impl.cc (right):
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
> components/html_viewer/web_layer_tree_view_impl.cc:35:
> settings.impl_side_painting = true;
> On 2015/06/15 17:47:34, danakj wrote:
>
>> would be nice to set use_one_copy = true here, or you might be falling
>>
> into the
>
>> async uploads (also deprecated/unused now) path.
>>
>
> Done, thanks.
>
>
>  You could add a TODO to set the use_image_texture_target and
>>
>
> Done, thanks
>
>  use_persistent_map_for_gpu_memory_buffers correctly? See
>>
>
> left out for now per our discussion since content doesn't set it
>
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/comp...
>
>  .. or for the renderer
>>
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
>
>> and
>>
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
>
>  For now can you hardcode them to GL_TEXTURE and false here
>>
> respectively?
>
>  Can you also set gather_pixel_refs?
>>
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/comp...
>
>  In content the renderer process has multiple threads and we turn that
>>
> on, FWIW.
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
>
>> .
>>
>
> added comment
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
> File components/view_manager/gles2/command_buffer_driver.cc (right):
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
> components/view_manager/gles2/command_buffer_driver.cc:238: LOG(ERROR)
> << "Image already exists with same ID.";
> On 2015/06/15 17:47:34, danakj wrote:
>
>> DLOGs for these?
>>
>
> II had just copied what content does..
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
> components/view_manager/gles2/command_buffer_driver.cc:263: if (type !=
> gfx::SHARED_MEMORY_BUFFER) {
> On 2015/06/15 18:20:35, danakj wrote:
>
>> Could all/some of these early outs be DCHECKs since they are already
>>
> checked in
>
>> mojo/gles2/command_buffer_client_impl.cc ?
>>
>
> ditto (copied content)
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
> File components/view_manager/gles2/mojo_gpu_memory_buffer_manager.cc
> (right):
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
> components/view_manager/gles2/mojo_gpu_memory_buffer_manager.cc:23:
> return MojoGpuMemoryBufferImpl::Create(size, format, usage);
> On 2015/06/15 18:20:35, danakj wrote:
>
>> I would expect to find the SharedMemory creation type stuff over here
>>
> instead of
>
>> inside Create() to be consistent with other implementations. Also the
>>
> manager
>
>> may want to add logic to that over time? Maybe not important right now
>>
> though.
>
> This is how I originally had it, but then this needs to be accessed by
> CommandBufferClientImpl::CreateGpuMemoryBufferImage as wel. Mandoline's
> CommandBufferClientImpl doesn't have access to
> MojoGpuMemoryBufferManager since that's in a different module, so that's
> why I moved this method implementation to somewhere else that can be
> shared.
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
> File components/view_manager/gles2/mojo_gpu_memory_buffer_manager.h
> (right):
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
> components/view_manager/gles2/mojo_gpu_memory_buffer_manager.h:1: //
> Copyright 2014 The Chromium Authors. All rights reserved.
> On 2015/06/15 18:20:35, danakj wrote:
>
>> 2015
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
> File components/view_manager/gles2/raster_thread_helper.h (right):
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/components/view_manage...
> components/view_manager/gles2/raster_thread_helper.h:1: // Copyright
> 2014 The Chromium Authors. All rights reserved.
> On 2015/06/15 18:20:35, danakj wrote:
>
>> 2015
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
> File gpu/command_buffer/build_gles2_cmd_buffer.py (right):
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
> gpu/command_buffer/build_gles2_cmd_buffer.py:3058: 'extension':
> "CHROMIUM_image",
> On 2015/06/15 18:14:33, piman (Very slow to review) wrote:
>
>> nit: CHROMIUM_pixel_transfer_buffer_object
>> I realize this extension was not documented, but this is the extension
>>
> name
>
>> that's associated with the functionality in the header.
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
> gpu/command_buffer/build_gles2_cmd_buffer.py:3474: 'extension':
> "CHROMIUM_image",
> On 2015/06/15 18:14:33, piman (Very slow to review) wrote:
>
>> CHROMIUM_pixel_transfer_buffer_object
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/gpu/command_buffer/bui...
> gpu/command_buffer/build_gles2_cmd_buffer.py:3869: 'extension':
> "CHROMIUM_image",
> On 2015/06/15 18:20:35, danakj wrote:
>
>> On 2015/06/15 18:14:33, piman (Very slow to review) wrote:
>> > nit: CHROMIUM_async_pixel_transfers (same deal).
>>
>
>  And, FWIW if you use_one_copy = true, you probably don't need to add
>>
> these async
>
>> things to the mojo autogens
>>
>
> set use_one_copy=true, and removed all changes that piman commented to
> use CHROMIUM_async_pixel_transfers for
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/mojo/cc/context_provid...
> File mojo/cc/context_provider_mojo.cc (right):
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/mojo/cc/context_provid...
> mojo/cc/context_provider_mojo.cc:16: capabilities_.gpu.image = true;
> On 2015/06/15 18:20:36, danakj wrote:
>
>> Can you also comment here pointing readers to some context?
>>
>
> like what kind of comment? for comparison, content also doesn't really
> document this flag or others when it sets them.
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
> File mojo/gles2/command_buffer_client_impl.cc (right):
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
> mojo/gles2/command_buffer_client_impl.cc:254: size->width = width;
> On 2015/06/15 20:04:42, yzshen1 wrote:
>
>> FYI, I tried to compile the patch on Windows and it complained about
>>
> conversion:
>
>  ==================
>> d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(254) : error
>>
> C2220:
>
>> warning treated as error - no 'object' file generated
>> d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(254) : warning
>>
> C4267:
>
>> '=' : conversion from 'size_t' to 'int32_t', possible loss of data
>> d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(255) : warning
>>
> C4267:
>
>> '=' : conversion from 'size_t' to 'int32_t', possible loss of data
>> d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(310) : warning
>>
> C4267:
>
>> 'argument' : conversion from 'size_t' to 'int', possible loss of data
>>
>
>
> that's strange, i'm compiling on Windows and I don't see that!
> what are your build GN build args?
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
> mojo/gles2/command_buffer_client_impl.cc:293:
> //gpu_memory_buffer_manager->SetDestructionSyncPoint(gpu_memory_buffer,
> On 2015/06/15 18:20:36, danakj wrote:
>
>> is a TODO needed?
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
> mojo/gles2/command_buffer_client_impl.cc:294: //
>                           InsertSyncPoint());
> On 2015/06/15 18:14:33, piman (Very slow to review) wrote:
>
>> nit: remove/add TODO?
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
> File mojo/gles2/command_buffer_client_impl.h (right):
>
>
>
https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
> mojo/gles2/command_buffer_client_impl.h:107: base::AtomicSequenceNumber
> next_image_id_;
> On 2015/06/15 18:14:33, piman (Very slow to review) wrote:
>
>> nit: shouldn't need to be atomic, this class should be used from a
>>
> single thread
>
>> at a time.
>>
>
> Done.
>
> https://codereview.chromium.org/1181013010/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2015-06-15 22:27:57]

On 2015/06/15 22:13:53, piman (Very slow to review) wrote:
> On Mon, Jun 15, 2015 at 3:06 PM, <mailto:jam@chromium.org> wrote:
> 
> > btw I've been seeing
> >
> > [0615/145740:ERROR:gles2_cmd_decoder.cc(12110)]
> > [GroupMarkerNotSet(crbug.com/242999)!:F890860D]GL
> > <http://crbug.com/242999)!:F890860D%5DGL> ERROR :GL_INVALID_VALUE :
> > glCopySubTextureCHROMIUM:
> > source texture has no level 0
> >
> > when I'm running mandoline now. However I can't tell exactly why this is
> > happening?
> 
> 
> It sounds like a legit error, but it's hard to trace with just this. Is it
> the only error you get? It sounds like either the
> CreateGpuMemoryBufferImage or the BindTexImage2DCHROMIUM failed...
> 
correct, it's only that error that i get. It happens once or twice before.
actually now with latest patchset, it happens constantly and there are many
black boxes (i.e. rendering is busted). i'll debug going back through patchsets
to see what change caused this...

[jam, 2015-06-15 22:44:55]

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
File components/html_viewer/discardable_memory_allocator.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
components/html_viewer/discardable_memory_allocator.cc:105: return
make_scoped_ptr(chunk);
On 2015/06/15 21:47:46, sky wrote:
> nit: move scoped_ptr to 89 and return that.

Done.

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
File components/html_viewer/discardable_memory_allocator.h (right):

https://codereview.chromium.org/1181013010/diff/270001/components/html_viewer...
components/html_viewer/discardable_memory_allocator.h:49: base::Lock lock_;
On 2015/06/15 21:47:46, sky wrote:
> On 2015/06/15 17:47:34, danakj wrote:
> > Could you comment or otherwise make it clear which variables this lock is
> > protecting?
> 
> Also document what threads are in use here. That isn't apparent to me. 

Done.

https://codereview.chromium.org/1181013010/diff/270001/mojo/cc/context_provid...
File mojo/cc/context_provider_mojo.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/mojo/cc/context_provid...
mojo/cc/context_provider_mojo.cc:16: capabilities_.gpu.image = true;
On 2015/06/15 22:11:20, danakj wrote:
> On 2015/06/15 22:06:01, jam wrote:
> > On 2015/06/15 18:20:36, danakj wrote:
> > > Can you also comment here pointing readers to some context?
> > 
> > like what kind of comment? for comparison, content also doesn't really
> document
> > this flag or others when it sets them.
> 
> Something like "Enabled the CHROMIUM_image extension to use GpuMemoryBuffers.
> The implementation of which is used in FooClassName."
> 
> I wish content/ did too.

Done.

[jam, 2015-06-16 01:28:02]

On 2015/06/15 22:27:57, jam wrote:
> On 2015/06/15 22:13:53, piman (Very slow to review) wrote:
> > On Mon, Jun 15, 2015 at 3:06 PM, <mailto:jam@chromium.org> wrote:
> > 
> > > btw I've been seeing
> > >
> > > [0615/145740:ERROR:gles2_cmd_decoder.cc(12110)]
> > > [GroupMarkerNotSet(crbug.com/242999)!:F890860D]GL
> > > <http://crbug.com/242999)!:F890860D%5DGL> ERROR :GL_INVALID_VALUE :
> > > glCopySubTextureCHROMIUM:
> > > source texture has no level 0
> > >
> > > when I'm running mandoline now. However I can't tell exactly why this is
> > > happening?
> > 
> > 
> > It sounds like a legit error, but it's hard to trace with just this. Is it
> > the only error you get? It sounds like either the
> > CreateGpuMemoryBufferImage or the BindTexImage2DCHROMIUM failed...
> > 
> correct, it's only that error that i get. It happens once or twice before.
> actually now with latest patchset, it happens constantly and there are many
> black boxes (i.e. rendering is busted). i'll debug going back through
patchsets
> to see what change caused this...

actually, looks like it was because I forgot to initialize next_image_id_ so it
was returning negative values, which the code in cc didn't expect. fixing that
now gets rid of the warnings and rendering works fine.

[jam, 2015-06-16 02:49:51]

The CQ bit was checked by jam@chromium.org

[jam, 2015-06-16 02:49:51]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
piman@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1181013010/#ps350001
(title: "fix uninitialized variable")

[commit-bot: I haz the power, 2015-06-16 02:50:29]

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

[commit-bot: I haz the power, 2015-06-16 02:58:33]

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

[commit-bot: I haz the power, 2015-06-16 02:58:34]

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

[jam, 2015-06-16 03:03:04]

Adding NOPRESUBMIT=true since the python file adds an include to a generated
header that is > 80 chars.

[jam, 2015-06-16 03:03:20]

The CQ bit was checked by jam@chromium.org

[jam, 2015-06-16 03:03:21]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
sky@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1181013010/#ps370001
(title: "fix presubmit warning")

[commit-bot: I haz the power, 2015-06-16 03:03:51]

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

[jam, 2015-06-16 03:36:04]

The CQ bit was unchecked by jam@chromium.org

[jam, 2015-06-16 14:21:49]

The CQ bit was checked by jam@chromium.org

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

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

[commit-bot: I haz the power, 2015-06-16 14:55:29]

Committed patchset #19 (id:370001)

[commit-bot: I haz the power, 2015-06-16 14:56:28]

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

[yzshen1, 2015-06-16 16:34:25]

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1181013010/diff/270001/mojo/gles2/command_buf...
mojo/gles2/command_buffer_client_impl.cc:254: size->width = width;
On 2015/06/15 22:06:02, jam wrote:
> On 2015/06/15 20:04:42, yzshen1 wrote:
> > FYI, I tried to compile the patch on Windows and it complained about
> conversion:
> > 
> > ==================
> > d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(254) : error C2220:
> > warning treated as error - no 'object' file generated
> > d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(254) : warning
C4267:
> > '=' : conversion from 'size_t' to 'int32_t', possible loss of data
> > d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(255) : warning
C4267:
> > '=' : conversion from 'size_t' to 'int32_t', possible loss of data
> > d:\src\cr6\src\mojo\gles2\command_buffer_client_impl.cc(310) : warning
C4267:
> > 'argument' : conversion from 'size_t' to 'int', possible loss of data
> > 
> 
> that's strange, i'm compiling on Windows and I don't see that!
> what are your build GN build args?

That is strange. I didn't explicitly set any gn args. When I ran 'gn args
out\Debug' it showed no args.

[Marijn Kruisselbrink, 2015-06-16 17:51:47]

A revert of this CL (patchset #19 id:370001) has been created in
https://codereview.chromium.org/1189833005/ by mek@chromium.org.

The reason for reverting is: The same compile errors also happen on the Win x64
GN bot:
http://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/2869/...

e:\b\build\slave\win_x64_gn\build\src\mojo\gles2\command_buffer_client_impl.cc(255)
:error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win_x64_gn\build\src\mojo\gles2\command_buffer_client_impl.cc(255)
: warning C4267: '=' : conversion from 'size_t' to 'int32_t', possible loss of
data
e:\b\build\slave\win_x64_gn\build\src\mojo\gles2\command_buffer_client_impl.cc(256)
: warning C4267: '=' : conversion from 'size_t' to 'int32_t', possible loss of
data
e:\b\build\slave\win_x64_gn\build\src\mojo\gles2\command_buffer_client_impl.cc(313)
: warning C4267: 'argument' : conversion from 'size_t' to 'int', possible loss
of data

.

[jam, 2015-06-16 17:58:34]

On 2015/06/16 17:51:47, Marijn Kruisselbrink wrote:
> A revert of this CL (patchset #19 id:370001) has been created in
> https://codereview.chromium.org/1189833005/ by mailto:mek@chromium.org.
> 
> The reason for reverting is: The same compile errors also happen on the Win
x64
> GN bot:
>
http://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/2869/...
> 
>
e:\b\build\slave\win_x64_gn\build\src\mojo\gles2\command_buffer_client_impl.cc(255)
> :error C2220: warning treated as error - no 'object' file generated
>
e:\b\build\slave\win_x64_gn\build\src\mojo\gles2\command_buffer_client_impl.cc(255)
> : warning C4267: '=' : conversion from 'size_t' to 'int32_t', possible loss of
> data
>
e:\b\build\slave\win_x64_gn\build\src\mojo\gles2\command_buffer_client_impl.cc(256)
> : warning C4267: '=' : conversion from 'size_t' to 'int32_t', possible loss of
> data
>
e:\b\build\slave\win_x64_gn\build\src\mojo\gles2\command_buffer_client_impl.cc(313)
> : warning C4267: 'argument' : conversion from 'size_t' to 'int', possible loss
> of data
> 
> .

yzshen has a fix out. no need to immediately revert without checking with cl
authors for non-CQ bots.

[tfarina, 2015-10-26 12:12:09]

https://codereview.chromium.org/1181013010/diff/370001/components/view_manage...
File components/view_manager/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1181013010/diff/370001/components/view_manage...
components/view_manager/gles2/command_buffer_driver.cc:285: gfx_handle.handle =
base::FileDescriptor(platform_handle, false);
Unfortunately this does not work on Mac:

../../components/mus/gles2/command_buffer_driver.cc:249:10: error: no viable
overloaded '='
  handle = base::FileDescriptor(platform_handle, false);
  ~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/memory/shared_memory_handle.h:117:23: note: candidate function not
viable: no known conversion from 'base::FileDescriptor' to 'const
base::SharedMemoryHandle' for 1st argument
  SharedMemoryHandle& operator=(const SharedMemoryHandle& handle);
                      ^
1 error generated.

base::SharedMemoryHandle (base/memory/shared_memory_handle.h) is declared as
follow:

#if defined(OS_POSIX) && !(defined(OS_MACOSX) && !defined(OS_IOS))
typedef FileDescriptor SharedMemoryHandle;
#elif defined(OS_WIN)
class BASE_EXPORT SharedMemoryHandle {
...
};
#else
class BASE_EXPORT SharedMemoryHandle {
};
#endif