gpu: Add IOSurface backed GpuMemoryBuffer implementation.

gpu: Add IOSurface backed GpuMemoryBuffer implementation.
    
This adds a GpuMemoryBuffer implementation on MacOSX that is backed
by IOSurfaces. These GpuMemmoryBuffers provide zero-copy texture
updates when using impl-side painting on MacOSX.
    
BUG=321785, 323338
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238841

[reveman, 2013-11-26 01:28:46]

I think this is ready for review. It requires punching a hole in the renderer
sandbox (https://codereview.chromium.org/85523009/ does that) but can of course
be used without that for testing purposes with --no-sandbox and gives us a good
idea of what kind of performance benefits we can get from zero-copy on other
platforms.

[jeremy, 2013-11-26 14:29:15]

Opening new holes in the renderer sandbox is a last resort, can we get around
this by adding code to "warm up" the required resource in the sandbox warmup
code?

Why isn't the .sb change part of this CL?

[reveman, 2013-11-26 23:40:39]

Latest patch adds IOSurfaceLookup to the sandbox warm up code. It seems to be
enough to make this work. PTAL.

[jeremy, 2013-11-27 08:26:08]

Sandbox warmup LGTM, can't review the rest of the CL.

Thank you for taking the time to do this right!

https://codereview.chromium.org/77023002/diff/210001/content/common/sandbox_m...
File content/common/sandbox_mac.mm (right):

https://codereview.chromium.org/77023002/diff/210001/content/common/sandbox_m...
content/common/sandbox_mac.mm:311: { // IOSurfaceLookup() - 10.7
Is there a bug you can link to?

Any idea how expensive this call is - asking from the point of view of startup
perf...

[reveman, 2013-11-27 16:04:28]

https://codereview.chromium.org/77023002/diff/210001/content/common/sandbox_m...
File content/common/sandbox_mac.mm (right):

https://codereview.chromium.org/77023002/diff/210001/content/common/sandbox_m...
content/common/sandbox_mac.mm:311: { // IOSurfaceLookup() - 10.7
On 2013/11/27 08:26:09, jeremy wrote:
> Is there a bug you can link to?

Done.

> 
> Any idea how expensive this call is - asking from the point of view of startup
> perf...

Added some tracing and IOSurfaceSupport::Initialize() consistently takes 0.2ms
and IOSurfaceLookup() 0.4ms on my Mac Pro.

[piman, 2013-12-03 02:59:51]

+kbr for the IOSurface things which I don't fully understand.

https://codereview.chromium.org/77023002/diff/310001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/77023002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1226: last_io_surface_ =
io_surface;
I don't understand the semantics this is trying to achieve here. It looks like
it's trying to keep a reference until the next time we call this? Is the
assumption that the renderer will grab a reference to the resource before
sending another message?
It seems very fragile, because RenderThreadImpl::AllocateGpuMemoryBuffer could
be called from multiple threads, with one clobbering this reference before the
other one had a chance to grab a reference to the system resource. Should we
grab a lock there?

At the very least, could you add a comment to document what's happening here,
the linked bug isn't very informative as to what's happening wrt this.

https://codereview.chromium.org/77023002/diff/310001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl.h (left):

https://codereview.chromium.org/77023002/diff/310001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:40:
DISALLOW_COPY_AND_ASSIGN(GpuMemoryBufferImpl);
nit: keep

https://codereview.chromium.org/77023002/diff/310001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl.h (right):

https://codereview.chromium.org/77023002/diff/310001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:24: static bool
IsFormatValid(unsigned internalformat);
This gets hidden by the GpuMemoryBufferImplIOSurface version. Should it move
(with the DCHECK) to GpuMemoryBufferImplShm?

https://codereview.chromium.org/77023002/diff/310001/ui/gl/gl_image_io_surface.h
File ui/gl/gl_image_io_surface.h (right):

https://codereview.chromium.org/77023002/diff/310001/ui/gl/gl_image_io_surfac...
ui/gl/gl_image_io_surface.h:31: DISALLOW_COPY_AND_ASSIGN(GLImageIOSurface);
nit: make this the last statement in the class definition.

[reveman, 2013-12-03 17:58:32]

+kbr

PTAL

https://codereview.chromium.org/77023002/diff/310001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/77023002/diff/310001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1226: last_io_surface_ =
io_surface;
On 2013/12/03 02:59:51, piman wrote:
> I don't understand the semantics this is trying to achieve here. It looks like
> it's trying to keep a reference until the next time we call this? Is the
> assumption that the renderer will grab a reference to the resource before
> sending another message?
> It seems very fragile, because RenderThreadImpl::AllocateGpuMemoryBuffer could
> be called from multiple threads, with one clobbering this reference before the
> other one had a chance to grab a reference to the system resource. Should we
> grab a lock there?

Added a ThreadChecker to ensure that this is not called from multiple threads.

> 
> At the very least, could you add a comment to document what's happening here,
> the linked bug isn't very informative as to what's happening wrt this.

Created a new bug related to this that I hope is a bit more informative. I think
it's a good idea to have a separate bug for this as it should be possible to
solve this without a lock if needed before mach_port_t based IOSurface sharing
is added.

Possible solution here: https://codereview.chromium.org/100053005

https://codereview.chromium.org/77023002/diff/310001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl.h (left):

https://codereview.chromium.org/77023002/diff/310001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:40:
DISALLOW_COPY_AND_ASSIGN(GpuMemoryBufferImpl);
On 2013/12/03 02:59:51, piman wrote:
> nit: keep

Done.

https://codereview.chromium.org/77023002/diff/310001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl.h (right):

https://codereview.chromium.org/77023002/diff/310001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:24: static bool
IsFormatValid(unsigned internalformat);
On 2013/12/03 02:59:51, piman wrote:
> This gets hidden by the GpuMemoryBufferImplIOSurface version. Should it move
> (with the DCHECK) to GpuMemoryBufferImplShm?

I think this belongs in GpuMemoryBufferImpl as it's used by
RenderThreadImpl::AllocateGpuMemoryBuffer to verify that format is a generally
valid format.

I renamed the GpuMemoryBufferImplIOSurface version to IsFormatSupported for this
to be less confusing.

[piman, 2013-12-03 21:17:08]

LGTM for my parts.

[reveman, 2013-12-03 21:21:18]

+cdn for ipc changes

[Ken Russell (switch to Gerrit), 2013-12-03 22:36:12]

LGTM assuming this has been tested.

I don't know how these GLImages are drawn by the compositor, but it seems clear
you know that IOSurfaces only work with the GL_TEXTURE_RECTANGLE_ARB texture
target. This means you need to use an alternate set of shaders when drawing them
to the screen because the texture coordinates are different (0..width,0..height
instead of 0..1,0..1).

https://codereview.chromium.org/77023002/diff/330001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/77023002/diff/330001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1226: last_io_surface_ =
io_surface;
Additionally, do you really want to hold on to the last_io_surface_ if
allocation of the new one happens to fail? Or do you think it would be better to
release it unconditionally?

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc (right):

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:44:
IOSurfaceSupport* io_surface_support = IOSurfaceSupport::Initialize();
Consider caching this object as a member.

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:46: LOG(ERROR) <<
"IOSurfaces only supported on 10.6.";
Chrome doesn't run on 10.5 any more, so this log message could just as well be a
CHECK. If not, it should at least read "10.6 and later".

Also, the LOGs in this file minimally should be VLOG. I think there's a new
presubmit check along those lines.

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:23: }
It would be nice to figure out a way to share this duplicated code among all the
implementations.

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_shm.cc (right):

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_shm.cc:34: void
GpuMemoryBufferImplShm::Map(AccessMode mode, void** vaddr) {
How often are Map/Unmap expected to be called?

I would expect shared memory map/unmap operations to be really expensive, but
IOSurface lock/unlock operations to be relatively cheap. For shared memory
backed GpuMemoryBufferImpls, I expect they could be kept mapped for the lifetime
of the object, and Map/Unmap to basically be no-ops. For the IOSurface backed
GpuMemoryBufferImpls, though, you would definitely need the Lock/Unlock
operations for correctness each time you rasterize into the IOSurface.

https://codereview.chromium.org/77023002/diff/330001/ui/gl/gl_image_io_surfac...
File ui/gl/gl_image_io_surface.cc (right):

https://codereview.chromium.org/77023002/diff/330001/ui/gl/gl_image_io_surfac...
ui/gl/gl_image_io_surface.cc:23: LOG(ERROR) << "IOSurfaces only supported on
10.6.";
Same comment as in earlier file about these LOG messages.

https://codereview.chromium.org/77023002/diff/330001/ui/gl/io_surface_support...
File ui/gl/io_surface_support_mac.h (right):

https://codereview.chromium.org/77023002/diff/330001/ui/gl/io_surface_support...
ui/gl/io_surface_support_mac.h:24: class GL_EXPORT IOSurfaceSupport {
Now that Chrome doesn't need to run on 10.5 any more, the implementation of this
class could statically link against IOSurface.framework and be massively
simplified. Would you mind filing a follow-up bug about that?

[Cris Neckar, 2013-12-03 22:54:47]

https://codereview.chromium.org/77023002/diff/330001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/77023002/diff/330001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1237: width * height *
GpuMemoryBufferImpl::BytesPerPixel(internalformat),
Need to check for integer overflow

[reveman, 2013-12-04 02:00:30]

Fyi, crrev.com/236981 added support for GL_TEXTURE_RECTANGLE_ARB to the
compositor and it's now used by default on Mac when updating textures using
zero-copy framework. That patch included all changes needed to properly use
non-normalized texture coordinates.

https://codereview.chromium.org/77023002/diff/330001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/77023002/diff/330001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1226: last_io_surface_ =
io_surface;
On 2013/12/03 22:36:13, Ken Russell wrote:
> Additionally, do you really want to hold on to the last_io_surface_ if
> allocation of the new one happens to fail? Or do you think it would be better
to
> release it unconditionally?

I should probably release it at the top of this function in that case. This is
all a bit nasty and I'm hoping to land a more appropriate fix such as
https://codereview.chromium.org/100053005/ in the near future. I prefer the
current code simply because it limits the last_io_surface_ nastiness to one
place.

https://codereview.chromium.org/77023002/diff/330001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1237: width * height *
GpuMemoryBufferImpl::BytesPerPixel(internalformat),
On 2013/12/03 22:54:48, Cris Neckar wrote:
> Need to check for integer overflow

Done.

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc (right):

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:44:
IOSurfaceSupport* io_surface_support = IOSurfaceSupport::Initialize();
On 2013/12/03 22:36:13, Ken Russell wrote:
> Consider caching this object as a member.

Done.

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:46: LOG(ERROR) <<
"IOSurfaces only supported on 10.6.";
On 2013/12/03 22:36:13, Ken Russell wrote:
> Chrome doesn't run on 10.5 any more, so this log message could just as well be
a
> CHECK. If not, it should at least read "10.6 and later".

Added CHECK to ctor.

> 
> Also, the LOGs in this file minimally should be VLOG. I think there's a new
> presubmit check along those lines.

Done.

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:23: }
On 2013/12/03 22:36:13, Ken Russell wrote:
> It would be nice to figure out a way to share this duplicated code among all
the
> implementations.

An alternative is to add some "if defined(OS_*)" code to
gpu_memory_buffer_impl.cc but I find duplicated code a bit cleaner. I'm also
expecting these to diverge in the near future.

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_shm.cc (right):

https://codereview.chromium.org/77023002/diff/330001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_shm.cc:34: void
GpuMemoryBufferImplShm::Map(AccessMode mode, void** vaddr) {
On 2013/12/03 22:36:13, Ken Russell wrote:
> How often are Map/Unmap expected to be called?

Less frequently than the buffer is used for drawing.
 
> 
> I would expect shared memory map/unmap operations to be really expensive, but
> IOSurface lock/unlock operations to be relatively cheap. For shared memory
> backed GpuMemoryBufferImpls, I expect they could be kept mapped for the
lifetime
> of the object, and Map/Unmap to basically be no-ops. For the IOSurface backed
> GpuMemoryBufferImpls, though, you would definitely need the Lock/Unlock
> operations for correctness each time you rasterize into the IOSurface.

I haven't seen map/unmap be very expensive. This class is mostly used for
testing purposes so performance is not critical but I'll keep this in mind in
case we ever consider using it for more than testing.

https://codereview.chromium.org/77023002/diff/330001/ui/gl/gl_image_io_surfac...
File ui/gl/gl_image_io_surface.cc (right):

https://codereview.chromium.org/77023002/diff/330001/ui/gl/gl_image_io_surfac...
ui/gl/gl_image_io_surface.cc:23: LOG(ERROR) << "IOSurfaces only supported on
10.6.";
On 2013/12/03 22:36:13, Ken Russell wrote:
> Same comment as in earlier file about these LOG messages.

Done.

https://codereview.chromium.org/77023002/diff/330001/ui/gl/io_surface_support...
File ui/gl/io_surface_support_mac.h (right):

https://codereview.chromium.org/77023002/diff/330001/ui/gl/io_surface_support...
ui/gl/io_surface_support_mac.h:24: class GL_EXPORT IOSurfaceSupport {
On 2013/12/03 22:36:13, Ken Russell wrote:
> Now that Chrome doesn't need to run on 10.5 any more, the implementation of
this
> class could statically link against IOSurface.framework and be massively
> simplified. Would you mind filing a follow-up bug about that?

Done. crbug.com/325572

[Cris Neckar, 2013-12-04 18:52:44]

https://codereview.chromium.org/77023002/diff/350001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/77023002/diff/350001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1236:
GpuMemoryBufferImpl::BytesPerPixel(internalformat);
max size_t * max size_t * 4 still overflows a 64 bit uint. Why don't we just
clamp these to some sane size.

[Cris Neckar, 2013-12-04 18:57:06]

On 2013/12/04 18:52:44, Cris Neckar wrote:
>
https://codereview.chromium.org/77023002/diff/350001/content/browser/renderer...
> File content/browser/renderer_host/render_message_filter.cc (right):
> 
>
https://codereview.chromium.org/77023002/diff/350001/content/browser/renderer...
> content/browser/renderer_host/render_message_filter.cc:1236:
> GpuMemoryBufferImpl::BytesPerPixel(internalformat);
> max size_t * max size_t * 4 still overflows a 64 bit uint. Why don't we just
> clamp these to some sane size.

Specifically maybe check that both height and width are below 32,000 at the
beginning of the handler. Or if there is a much lower number that you know will
never be exceeded you can use that.

[reveman, 2013-12-04 21:39:01]

https://codereview.chromium.org/77023002/diff/350001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/77023002/diff/350001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1236:
GpuMemoryBufferImpl::BytesPerPixel(internalformat);
On 2013/12/04 18:52:45, Cris Neckar wrote:
> max size_t * max size_t * 4 still overflows a 64 bit uint. Why don't we just
> clamp these to some sane size.

Oops, I just fixed the overflow check for now as I'd rather not put some
arbitrary limit on the dimensions of these buffers. If you insist that we should
add a limit then maybe we should change the type of width/height..

[Cris Neckar, 2013-12-04 21:54:47]

On 2013/12/04 21:39:01, David Reveman wrote:
>
https://codereview.chromium.org/77023002/diff/350001/content/browser/renderer...
> File content/browser/renderer_host/render_message_filter.cc (right):
> 
>
https://codereview.chromium.org/77023002/diff/350001/content/browser/renderer...
> content/browser/renderer_host/render_message_filter.cc:1236:
> GpuMemoryBufferImpl::BytesPerPixel(internalformat);
> On 2013/12/04 18:52:45, Cris Neckar wrote:
> > max size_t * max size_t * 4 still overflows a 64 bit uint. Why don't we just
> > clamp these to some sane size.
> 
> Oops, I just fixed the overflow check for now as I'd rather not put some
> arbitrary limit on the dimensions of these buffers. If you insist that we
should
> add a limit then maybe we should change the type of width/height..

IPC changes LGTM

[commit-bot: I haz the power, 2013-12-04 21:59:06]

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

[commit-bot: I haz the power, 2013-12-04 22:16:52]

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

[danakj, 2013-12-04 22:32:23]

ui/gfx rubberstamplgtm

[commit-bot: I haz the power, 2013-12-04 22:33:25]

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

[commit-bot: I haz the power, 2013-12-05 01:02:16]

Change committed as 238841