Mac: Use AVSampleBufferDisplayLayer for 4:2:0 surfaces

Mac: Use AVSampleBufferDisplayLayer for 4:2:0 surfaces

Enqueuing frames into an AVSampleBufferDisplayLayer is more
power efficient than setting the contents of the layer directly, and
also can be used in detached mode.

BUG=594449
Committed: https://crrev.com/fbaed4750ac46ce84d4b5d361b2601447db5390d
Cr-Commit-Position: refs/heads/master@{#382975}

[ccameron, 2016-03-23 08:43:47]

Description was changed from

==========
BUG=
==========

to

==========
Mac: Use AVSampleBufferDisplayLayer for 4:2:0 surfaces

Enqueuing frames into an AVSampleBufferDisplayLayer is more
power efficient than setting the contents of the layer directly, and
also can be used in detached mode.

BUG=594449
==========

[ccameron, 2016-03-23 08:57:10]

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

[ccameron, 2016-03-23 08:57:10]

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
File content/common/gpu/ca_layer_tree_mac.mm (right):

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:18: 
I tried putting this into sdk_forward_declarations.h and that seemed to be out
of control (it started pulling in headers from CoreVideo, CoreMedia, and
AVFoundation) ... this localized version seems more reasonable to me.

[erikchen, 2016-03-23 17:45:00]

https://codereview.chromium.org/1828523003/diff/1/content/common/BUILD.gn
File content/common/BUILD.gn (right):

https://codereview.chromium.org/1828523003/diff/1/content/common/BUILD.gn#new...
content/common/BUILD.gn:248: "CoreMedia.framework",
I see you using CM* prefixed functions, but no includes for CoreMedia. Did you
just forget to IWYU?

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
File content/common/gpu/ca_layer_tree_mac.mm (right):

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:20: MAC_OS_X_VERSION_MIN_REQUIRED <
MAC_OS_X_VERSION_10_8
I assume this will no longer be required after we bump the deployment target to
10.8+?

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:21: BASE_EXPORT extern NSString* const
AVLayerVideoGravityResize;
BASE_EXPORT?

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:51: bool
AVSampleBufferDisplayLayerEnqueueIOSurface(
Can you add a comment on the ownership semantics of this function? It looks like
on success, io_surface gets retained by cv_pixel_buffer, which in turn becomes
owned by av_layer?

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:60: kCFAllocatorDefault, io_surface,
pixel_buffer_attributes.get(),
Isn't pixel_buffer_attributes.get() always nullptr?

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:60: kCFAllocatorDefault, io_surface,
pixel_buffer_attributes.get(),
shouldn't we pass null to use the defualt allocator? Mostly looking for
consistency between this and CMVideoFormatDescriptionCreateForImageBuffer

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:62: if (cv_return) {
I think you want cv_return != kCVReturnSuccess

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:70: if (os_status) {
use proper conditional here and later in function.

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:76: CMTime frame_time = CMTimeMake(0,
30);
Where did this number 30 come from?

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:79:
base::ScopedCFTypeRef<CMSampleBufferRef> sample_buffer = NULL;
s/NULL/nullptr

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:95: CFMutableDictionaryRef
attachments_dictionary =
Check that array has size >=1 first

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:512: ca_layer.reset([[CALayer alloc]
init]);
Shouldn't we reset av_layer as well [to release a reference?]

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:530: if (update_contents_rect)
What if we're using a new av layer and need to update the contents rect (to
become 0, 0, 1, 1)?

[ccameron, 2016-03-23 18:26:21]

Thanks! Updated.

https://codereview.chromium.org/1828523003/diff/1/content/common/BUILD.gn
File content/common/BUILD.gn (right):

https://codereview.chromium.org/1828523003/diff/1/content/common/BUILD.gn#new...
content/common/BUILD.gn:248: "CoreMedia.framework",
On 2016/03/23 17:44:59, erikchen wrote:
> I see you using CM* prefixed functions, but no includes for CoreMedia. Did you
> just forget to IWYU?

Added CoreMedia.h to ca_layer_tree_mac.mm

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
File content/common/gpu/ca_layer_tree_mac.mm (right):

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:20: MAC_OS_X_VERSION_MIN_REQUIRED <
MAC_OS_X_VERSION_10_8
On 2016/03/23 17:44:59, erikchen wrote:
> I assume this will no longer be required after we bump the deployment target
to
> 10.8+?

Exactly.

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:21: BASE_EXPORT extern NSString* const
AVLayerVideoGravityResize;
On 2016/03/23 17:45:00, erikchen wrote:
> BASE_EXPORT?

Mmh, should have removed that when I removed these from base. Gone now.

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:51: bool
AVSampleBufferDisplayLayerEnqueueIOSurface(
On 2016/03/23 17:45:00, erikchen wrote:
> Can you add a comment on the ownership semantics of this function? It looks
like
> on success, io_surface gets retained by cv_pixel_buffer, which in turn becomes
> owned by av_layer?

Added a comment. I just said that the io_surface is retained and marked in-use
until it is displayed. In practice cv_pixel_buffer is an intermediate in that.

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:60: kCFAllocatorDefault, io_surface,
pixel_buffer_attributes.get(),
On 2016/03/23 17:44:59, erikchen wrote:
> Isn't pixel_buffer_attributes.get() always nullptr?

I spent so long trying to put random things in that dictionary that I didn't
want to forget it completely. nullptr now.

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:60: kCFAllocatorDefault, io_surface,
pixel_buffer_attributes.get(),
On 2016/03/23 17:45:00, erikchen wrote:
> shouldn't we pass null to use the defualt allocator? Mostly looking for
> consistency between this and CMVideoFormatDescriptionCreateForImageBuffer

Changed to nullptr.

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:62: if (cv_return) {
On 2016/03/23 17:45:00, erikchen wrote:
> I think you want cv_return != kCVReturnSuccess

Done (kCVReturnSuccess == 0).

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:70: if (os_status) {
On 2016/03/23 17:45:00, erikchen wrote:
> use proper conditional here and later in function.

Done.

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:76: CMTime frame_time = CMTimeMake(0,
30);
On 2016/03/23 17:44:59, erikchen wrote:
> Where did this number 30 come from?

Added a comment saying that frame time doesn't matter because we're displaying
immediately. And changed the denominator to 1.

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:79:
base::ScopedCFTypeRef<CMSampleBufferRef> sample_buffer = NULL;
On 2016/03/23 17:45:00, erikchen wrote:
> s/NULL/nullptr

Done.

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:95: CFMutableDictionaryRef
attachments_dictionary =
On 2016/03/23 17:45:00, erikchen wrote:
> Check that array has size >=1 first

Done.

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:512: ca_layer.reset([[CALayer alloc]
init]);
On 2016/03/23 17:44:59, erikchen wrote:
> Shouldn't we reset av_layer as well [to release a reference?]

We construct a new CALayerTree every frame (yeah, crazy, I know), so everything
here gets re-set every frame. If it's going to have a CALayer, it needs allocate
it or pull it from |old_layer|.

https://codereview.chromium.org/1828523003/diff/1/content/common/gpu/ca_layer...
content/common/gpu/ca_layer_tree_mac.mm:530: if (update_contents_rect)
On 2016/03/23 17:45:00, erikchen wrote:
> What if we're using a new av layer and need to update the contents rect (to
> become 0, 0, 1, 1)? 

We only use_av_layer when the content rect is (0,0,1,1) -- we may be able to
relax this, but I didn't bother with it.

If the content rect changed from not-(0,0,1,1) to (0,0,1,1), then
"old_layer->use_av_layer == use_av_layer" will make us create a new layer and
set everything again.

[erikchen, 2016-03-23 18:33:20]

lgtm

[ccameron, 2016-03-23 18:39:55]

Thanks!

[ccameron, 2016-03-23 18:40:02]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-03-23 18:40:32]

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

[commit-bot: I haz the power, 2016-03-23 18:49:19]

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

[commit-bot: I haz the power, 2016-03-23 18:49:20]

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, 2016-03-23 20:19:37]

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

[ccameron, 2016-03-23 20:19:37]

Adding sievers@ for content/ stamp

[no sievers, 2016-03-23 23:39:19]

On 2016/03/23 20:19:37, ccameron wrote:
> Adding sievers@ for content/ stamp

lgtm

[ccameron, 2016-03-23 23:40:12]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-03-23 23:40:34]

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

[commit-bot: I haz the power, 2016-03-23 23:46:42]

Description was changed from

==========
Mac: Use AVSampleBufferDisplayLayer for 4:2:0 surfaces

Enqueuing frames into an AVSampleBufferDisplayLayer is more
power efficient than setting the contents of the layer directly, and
also can be used in detached mode.

BUG=594449
==========

to

==========
Mac: Use AVSampleBufferDisplayLayer for 4:2:0 surfaces

Enqueuing frames into an AVSampleBufferDisplayLayer is more
power efficient than setting the contents of the layer directly, and
also can be used in detached mode.

BUG=594449
==========

[commit-bot: I haz the power, 2016-03-23 23:46:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-23 23:48:12]

Description was changed from

==========
Mac: Use AVSampleBufferDisplayLayer for 4:2:0 surfaces

Enqueuing frames into an AVSampleBufferDisplayLayer is more
power efficient than setting the contents of the layer directly, and
also can be used in detached mode.

BUG=594449
==========

to

==========
Mac: Use AVSampleBufferDisplayLayer for 4:2:0 surfaces

Enqueuing frames into an AVSampleBufferDisplayLayer is more
power efficient than setting the contents of the layer directly, and
also can be used in detached mode.

BUG=594449

Committed: https://crrev.com/fbaed4750ac46ce84d4b5d361b2601447db5390d
Cr-Commit-Position: refs/heads/master@{#382975}
==========

[commit-bot: I haz the power, 2016-03-23 23:48:13]

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