Mac Overlays: Wire up overlays on Mac

Mac Overlays: Wire up overlays on Mac

Add support for scanning out of BGRA memory buffers on Mac.

Add a very small image transport surface that uses the remote CALayer
API to set a CALayer's content to be the IOSurface that was rendered to.

Support is currently limited to one overlay surface.

BUG=510252
Committed: https://crrev.com/431311669957bc4d4674796bf80a9fed8ba2764a
Cr-Commit-Position: refs/heads/master@{#340223}

[ccameron, 2015-07-22 05:26:22]

ccameron@chromium.org changed reviewers:
+ alexst@chromium.org

[ccameron, 2015-07-22 05:26:22]

This still keeps the overlay validator as NULL, so I assume that exactly one
overlay plane will be scheduled per frame (legit assumption?).

https://codereview.chromium.org/1251783002/diff/1/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1251783002/diff/1/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_overlay_mac.mm:114:
pending_overlay_image_ = image;
So, is it legit to keep this weak pointer around until Swap? It appears to be
safe for this particular instance, but should we consider making these
refcounted or weak?

In particular, when we enable more overlays than just the one for the main
content, I'll want to stash all of the overlay images around, and then push the
changes to all of them inside the ScopedCAActionDisabler block in SwapBuffers.

[alexst (slow to review), 2015-07-22 14:38:02]

Just some notes, otherwise this all looks good!

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:89: return true;
I don't think this is true, post sub buffer expects you to fill the missing
pixels, so if partial swap gets enabled, you'll see corruption. I think it makes
sense to just do full swap as a first draft of this.

https://codereview.chromium.org/1251783002/diff/20001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1251783002/diff/20001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:204: CALayer* layer =
base::mac::ObjCCastStrict<CALayer>(widget);
In the type definitions 

#elif defined(OS_MACOSX)
typedef NSView* AcceleratedWidget;
...

If I understand correctly, you are using the widget as a transport type to pass
the layer into the image. Are CALayer and NSView related? Is it safe to cast
them around like that?

The way we use it on ozone and the way I thought of it conceptually is that you
are presenting the contents of an image as an overlay for the compositor whose
backing surface is represented by |widget|. If the types (per above) are
compatible and CALayer looks like it is indeed a form of backing surface, I
think this is fine. Otherwise, maybe you could just have a maps of widget ->
layer to resolve type safety.

[alexst (slow to review), 2015-07-22 14:43:53]

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:89: return true;
On 2015/07/22 14:38:02, alexst wrote:
> I don't think this is true, post sub buffer expects you to fill the missing
> pixels, so if partial swap gets enabled, you'll see corruption. I think it
makes
> sense to just do full swap as a first draft of this.

Never mind, I saw the other patch doing it in the browser, nothing to see here,
move along! :)

[Andre, 2015-07-22 21:34:35]

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

[Andre, 2015-07-22 21:34:36]

https://codereview.chromium.org/1251783002/diff/1/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_overlay_mac.h (right):

https://codereview.chromium.org/1251783002/diff/1/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_overlay_mac.h:8: #include
"base/mac/scoped_nsobject.h"
Use #import for an Obj-C header.

https://codereview.chromium.org/1251783002/diff/1/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_overlay_mac.h:11: #include
"ui/base/cocoa/remote_layer_api.h"
Do we need this include here?
Can we just forward declare @class CAContext?

https://codereview.chromium.org/1251783002/diff/1/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_overlay_mac.h:59: gfx::GLImage*
pending_overlay_image_;
Add weak comment?

https://codereview.chromium.org/1251783002/diff/1/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_overlay_mac.h:64: #endif  //
CONTENT_COMMON_GPU_IMAGE_TRANSPORT_SURFACE_FBO_MAC_H_
Comment is incorrect, should be
CONTENT_COMMON_GPU_IMAGE_TRANSPORT_SURFACE_OVERLAY_MAC_H_

https://codereview.chromium.org/1251783002/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/1251783002/diff/1/content/content_common.gypi...
content/content_common.gypi:342:
'common/gpu/image_transport_surface_overlay_mac.h',
Sorts after image_transport_surface_mac.

https://codereview.chromium.org/1251783002/diff/1/ui/gl/gl_image_io_surface.mm
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1251783002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:203: [layer setContents:(id)io_surface_.get()];
Style guide prohibits C style casts.

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:19: :
scale_factor_(1), pending_overlay_image_(NULL) {
nullptr

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:35: [CAContext
contextWithCGSConnection:connection_id options:dict]);
Can we pass nil or @{} for the options dict?

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:36: [ca_context_
retain];
I think it reads better if the retain happens before the reset.
Something like, 
  ca_context_.reset(
      [[CAContext contextWithCGSConnection:connection_id options:@{}] retain]);

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:68:
pending_overlay_image_ = NULL;
nullptr

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:97: return NULL;
nullptr

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:134:
latency_info_.push_back(latency_info[i]);
How about,
for (auto& info : latency_info)
  latency_info_.push_back(info);
or,
latency_info_.insert(latency_info_.end(),
                     latency_info.begin(), latency_info.end());

[ccameron, 2015-07-23 05:33:02]

Thanks!! Updated.

https://codereview.chromium.org/1251783002/diff/1/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_overlay_mac.h (right):

https://codereview.chromium.org/1251783002/diff/1/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_overlay_mac.h:8: #include
"base/mac/scoped_nsobject.h"
On 2015/07/22 21:34:36, Andre wrote:
> Use #import for an Obj-C header.

Done.

https://codereview.chromium.org/1251783002/diff/1/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_overlay_mac.h:11: #include
"ui/base/cocoa/remote_layer_api.h"
On 2015/07/22 21:34:36, Andre wrote:
> Do we need this include here?
> Can we just forward declare @class CAContext?

Done.

https://codereview.chromium.org/1251783002/diff/1/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_overlay_mac.h:59: gfx::GLImage*
pending_overlay_image_;
On 2015/07/22 21:34:36, Andre wrote:
> Add weak comment?

Done.

https://codereview.chromium.org/1251783002/diff/1/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_overlay_mac.h:64: #endif  //
CONTENT_COMMON_GPU_IMAGE_TRANSPORT_SURFACE_FBO_MAC_H_
On 2015/07/22 21:34:36, Andre wrote:
> Comment is incorrect, should be
> CONTENT_COMMON_GPU_IMAGE_TRANSPORT_SURFACE_OVERLAY_MAC_H_

Done.

https://codereview.chromium.org/1251783002/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/1251783002/diff/1/content/content_common.gypi...
content/content_common.gypi:342:
'common/gpu/image_transport_surface_overlay_mac.h',
On 2015/07/22 21:34:36, Andre wrote:
> Sorts after image_transport_surface_mac.

Done.

https://codereview.chromium.org/1251783002/diff/1/ui/gl/gl_image_io_surface.mm
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1251783002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:203: [layer setContents:(id)io_surface_.get()];
On 2015/07/22 21:34:36, Andre wrote:
> Style guide prohibits C style casts.

Changed to static_cast<id>.

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:19: :
scale_factor_(1), pending_overlay_image_(NULL) {
On 2015/07/22 21:34:36, Andre wrote:
> nullptr

Done.

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:35: [CAContext
contextWithCGSConnection:connection_id options:dict]);
On 2015/07/22 21:34:36, Andre wrote:
> Can we pass nil or @{} for the options dict?

Done.

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:36: [ca_context_
retain];
On 2015/07/22 21:34:36, Andre wrote:
> I think it reads better if the retain happens before the reset.
> Something like, 
>   ca_context_.reset(
>       [[CAContext contextWithCGSConnection:connection_id options:@{}]
retain]);

Done.

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:97: return NULL;
On 2015/07/22 21:34:36, Andre wrote:
> nullptr

Done.

https://codereview.chromium.org/1251783002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:134:
latency_info_.push_back(latency_info[i]);
On 2015/07/22 21:34:36, Andre wrote:
> How about,
> for (auto& info : latency_info)
>   latency_info_.push_back(info);
> or,
> latency_info_.insert(latency_info_.end(),
>                      latency_info.begin(), latency_info.end());

Good call -- went with the second one.

https://codereview.chromium.org/1251783002/diff/20001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1251783002/diff/20001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:204: CALayer* layer =
base::mac::ObjCCastStrict<CALayer>(widget);
On 2015/07/22 14:38:02, alexst wrote:
> In the type definitions 
> 
> #elif defined(OS_MACOSX)
> typedef NSView* AcceleratedWidget;
> ...
> 
> If I understand correctly, you are using the widget as a transport type to
pass
> the layer into the image. Are CALayer and NSView related? Is it safe to cast
> them around like that?
> 
> The way we use it on ozone and the way I thought of it conceptually is that
you
> are presenting the contents of an image as an overlay for the compositor whose
> backing surface is represented by |widget|. If the types (per above) are
> compatible and CALayer looks like it is indeed a form of backing surface, I
> think this is fine. Otherwise, maybe you could just have a maps of widget ->
> layer to resolve type safety.

I'm not sure if we actually use NSView as a type anywhere. In the browser
process, we use a phony NSView* and a map to internal structures here:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/accelerated_wid...

As it is, I'm essentially treating it as a void*, with some casts.

[alexst (slow to review), 2015-07-23 12:54:13]

lgtm

[Andre, 2015-07-23 18:14:02]

LGTM

https://codereview.chromium.org/1251783002/diff/60001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1251783002/diff/60001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:7: #include
<OpenGL/GL.h>
I think it's lowercase gl.h

https://codereview.chromium.org/1251783002/diff/60001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:45: widget_ =
reinterpret_cast<gfx::AcceleratedWidget>(previous_widget);
I'm not sure this is better than before, code that expects the widget as a
NSView would still crash as before.
I almost prefer the CALayer cast, equally dangerous but at least it's simpler.
I don't have any better idea though, other than making ScheduleOverlayPlane
accept an abstract interface instead of gfx::AcceleratedWidget.

[alexst (slow to review), 2015-07-23 18:54:33]

>
https://codereview.chromium.org/1251783002/diff/60001/content/common/gpu/imag...
> content/common/gpu/image_transport_surface_overlay_mac.mm:45: widget_ =
> reinterpret_cast<gfx::AcceleratedWidget>(previous_widget);
> I'm not sure this is better than before, code that expects the widget as a
> NSView would still crash as before.
> I almost prefer the CALayer cast, equally dangerous but at least it's simpler.
> I don't have any better idea though, other than making ScheduleOverlayPlane
> accept an abstract interface instead of gfx::AcceleratedWidget.

I think it's actually safe in this case, because NSView is used as a handle
only. Chris mentioned earlier in the patch that NSView functionality may not
even be used anywhere and it's only a dummy ptr? 

On ozone, AcceleratedWidget is an intptr because we abstract platforms and don't
know what the actual type may be. If mac is using nsview the same way, maybe it
makes sense to change the typedef?

[ccameron, 2015-07-23 19:15:24]

On 2015/07/23 18:54:33, alexst wrote:
> >
>
https://codereview.chromium.org/1251783002/diff/60001/content/common/gpu/imag...
> > content/common/gpu/image_transport_surface_overlay_mac.mm:45: widget_ =
> > reinterpret_cast<gfx::AcceleratedWidget>(previous_widget);
> > I'm not sure this is better than before, code that expects the widget as a
> > NSView would still crash as before.
> > I almost prefer the CALayer cast, equally dangerous but at least it's
simpler.
> > I don't have any better idea though, other than making ScheduleOverlayPlane
> > accept an abstract interface instead of gfx::AcceleratedWidget.
> 
> I think it's actually safe in this case, because NSView is used as a handle
> only. Chris mentioned earlier in the patch that NSView functionality may not
> even be used anywhere and it's only a dummy ptr? 
> 
> On ozone, AcceleratedWidget is an intptr because we abstract platforms and
don't
> know what the actual type may be. If mac is using nsview the same way, maybe
it
> makes sense to change the typedef?

Yeah, the only other place it's used, it's also a uintptr_t. I changed both
instances.

It's a bit scary in that the value of that integer means different things in the
GPU and the browser process (not sure there's anything to do about that).

[ccameron, 2015-07-23 19:44:48]

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

[ccameron, 2015-07-23 19:44:49]

Thanks!

Adding avi@ for ui/ OWNER

Also, leaving the feature disabled in this patch (I'll enable it in a separate
one, in case reverts are needed).

https://codereview.chromium.org/1251783002/diff/60001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1251783002/diff/60001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:7: #include
<OpenGL/GL.h>
On 2015/07/23 18:14:02, Andre wrote:
> I think it's lowercase gl.h

Done.

https://codereview.chromium.org/1251783002/diff/60001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:45: widget_ =
reinterpret_cast<gfx::AcceleratedWidget>(previous_widget);
On 2015/07/23 18:14:02, Andre wrote:
> I'm not sure this is better than before, code that expects the widget as a
> NSView would still crash as before.
> I almost prefer the CALayer cast, equally dangerous but at least it's simpler.
> I don't have any better idea though, other than making ScheduleOverlayPlane
> accept an abstract interface instead of gfx::AcceleratedWidget.

Changed it to a uintptr_t.

[Avi (use Gerrit), 2015-07-23 20:13:58]

lgtm

stamp with nits.

https://codereview.chromium.org/1251783002/diff/100001/content/common/gpu/ima...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1251783002/diff/100001/content/common/gpu/ima...
content/common/gpu/image_transport_surface_overlay_mac.mm:38: [[CAContext
contextWithCGSConnection:connection_id options:@{}] retain]);
I thought we weren't going to have to use SPI anymore... :)

https://codereview.chromium.org/1251783002/diff/100001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1251783002/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:16: #include <OpenGL/CGLIOSurface.h>
Alphabetical?

https://codereview.chromium.org/1251783002/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:204: DCHECK_EQ(z_order, 0);
expected value first.

https://codereview.chromium.org/1251783002/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:206: DCHECK_EQ(transform,
gfx::OVERLAY_TRANSFORM_NONE);
expected value first

[ccameron, 2015-07-23 22:16:39]

Thanks! Updated.

https://codereview.chromium.org/1251783002/diff/100001/content/common/gpu/ima...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1251783002/diff/100001/content/common/gpu/ima...
content/common/gpu/image_transport_surface_overlay_mac.mm:38: [[CAContext
contextWithCGSConnection:connection_id options:@{}] retain]);
On 2015/07/23 20:13:57, Avi wrote:
> I thought we weren't going to have to use SPI anymore... :)

In principle, this can be done without SPI, but the architecture is arranged in
such a way that SPI is easier (things are assumed to be done in the GPU process,
etc).

https://codereview.chromium.org/1251783002/diff/100001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1251783002/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:16: #include <OpenGL/CGLIOSurface.h>
On 2015/07/23 20:13:57, Avi wrote:
> Alphabetical?

Done.

https://codereview.chromium.org/1251783002/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:204: DCHECK_EQ(z_order, 0);
On 2015/07/23 20:13:57, Avi wrote:
> expected value first.

Done.

https://codereview.chromium.org/1251783002/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:206: DCHECK_EQ(transform,
gfx::OVERLAY_TRANSFORM_NONE);
On 2015/07/23 20:13:57, Avi wrote:
> expected value first

Done.

[ccameron, 2015-07-23 22:28:12]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-07-23 22:28:13]

The patchset sent to the CQ was uploaded after l-g-t-m from alexst@chromium.org,
andresantoso@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1251783002/#ps120001
(title: "Incorporate review feedback")

[commit-bot: I haz the power, 2015-07-23 22:28:40]

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

[commit-bot: I haz the power, 2015-07-23 22:48:20]

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

[commit-bot: I haz the power, 2015-07-23 22:48:21]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ccameron, 2015-07-23 23:00:25]

On 2015/07/23 22:48:21, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Charming ... Angle defines EGLNativeWindowType as NSView* for fun. So, we'll
have to find some complicated handshake to fix the type. I'll leave the
reinterpret_cast from uintptr_t around for now.

[ccameron, 2015-07-23 23:36:47]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-07-23 23:36:48]

The patchset sent to the CQ was uploaded after l-g-t-m from alexst@chromium.org,
avi@chromium.org, andresantoso@chromium.org
Link to the patchset: https://codereview.chromium.org/1251783002/#ps140001
(title: "Leave widget type unchanged")

[commit-bot: I haz the power, 2015-07-23 23:37:19]

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

[commit-bot: I haz the power, 2015-07-24 00:05:58]

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

[commit-bot: I haz the power, 2015-07-24 00:05:59]

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...)

[ccameron, 2015-07-24 01:00:06]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-07-24 03:55:41]

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

[commit-bot: I haz the power, 2015-07-24 05:10:12]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-07-24 05:10:48]

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