services/ui: Detect when a video is playing

services/ui: Detect when a video is playing.

Compositor frames submitted by mus clients include a flag to indicate whether the
client is playing a video or not. This information is plumbed through from
cc::Layer to cc::LayerImpl to cc::CompositorFrame[Metadata].

BUG=627140
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/32d1097c5645c0e1b0f49f93502d4f4701b67cec
Cr-Commit-Position: refs/heads/master@{#408864}

[sadrul, 2016-07-11 19:46:07]

Description was changed from

==========
[exp] video detection in mus.

BUG=627140
==========

to

==========
[exp] video detection in mus.

BUG=627140
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sadrul, 2016-07-22 03:49:34]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 03:50:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 03:50:10]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-22 03:52:26]

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

[commit-bot: I haz the power, 2016-07-22 03:52:27]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[sadrul, 2016-07-22 17:20:00]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 17:20:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 17:20:24]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[sadrul, 2016-07-22 17:31:26]

Description was changed from

==========
[exp] video detection in mus.

BUG=627140
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
[exp] video detection in mus.

BUG=627140
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[sadrul, 2016-07-22 18:36:54]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 18:37:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-07-22 19:14:22]

Description was changed from

==========
[exp] video detection in mus.

BUG=627140
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
services/ui: Detect when a video is playing.

Compositor frames submitted by mus clients include a flag to indicate whether
the
client is playing a video or not. This information is plumbed through from
cc::Layer to cc::LayerImpl to cc::CompositorFrame[Metadata].

BUG=627140
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[sadrul, 2016-07-22 19:16:00]

sadrul@chromium.org changed reviewers:
+ enne@chromium.org

[sadrul, 2016-07-22 19:16:01]

Hi! Would you mind looking at the changes in cc and see if that makes sense? The
CL isn't ready to land since it doesn't have any test coverage; I will add some
tests if the approach here looks good to you. Thanks!

[commit-bot: I haz the power, 2016-07-22 19:51:27]

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

[commit-bot: I haz the power, 2016-07-22 19:51:28]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2016-07-25 18:40:13]

Why this should be on the frame itself and not on the texture quad? Would you
ever want to know if the video was offscreen or occluded and do something
different?

[sadrul, 2016-07-25 18:58:00]

On 2016/07/25 18:40:13, enne wrote:
> Why this should be on the frame itself and not on the texture quad?

It seemed simpler to include this in the frame metadata, since that's a single
place. We could add this on the texture quad too I guess (and I think on
YUVVideoDrawQuad, StreamVideoDrawQuad too?), but I think that would make it a
bit more expensive to know whether a particular frame has any video in it? (I am
referring to the code in
https://codereview.chromium.org/2140783002/diff/80001/services/ui/ws/server_w...
and
https://codereview.chromium.org/2140783002/diff/80001/cc/trees/layer_tree_hos...)

> Would you
> ever want to know if the video was offscreen or occluded and do something
> different?

We do not do anything for videos that are not visible on screen right now. It's
hard to tell whether we would want that in the future. (note that with this CL,
we set the bit on the frame only if one of the 'maybe video' layers 'will draw',
so if the video is offscreen or occluded, then the bit is not set on the frame).

[enne (OOO), 2016-07-25 19:17:35]

https://codereview.chromium.org/2140783002/diff/80001/cc/ipc/render_pass_stru...
File cc/ipc/render_pass_struct_traits.cc (left):

https://codereview.chromium.org/2140783002/diff/80001/cc/ipc/render_pass_stru...
cc/ipc/render_pass_struct_traits.cc:27:
DCHECK(input->shared_quad_state_list.empty() ||
?

https://codereview.chromium.org/2140783002/diff/80001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2140783002/diff/80001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:705: if (!damage_rect.IsEmpty() &&
surface_frame.metadata.may_contain_video)
Why does the damage rect come into play here? If you had a 30 fps video but the
browser compositor had a tab spinner animation at 60fps, would you want the may
contain video to alternate true/false every other frame?

[sadrul, 2016-07-26 04:08:07]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 04:08:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 04:10:47]

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

[commit-bot: I haz the power, 2016-07-26 04:10:48]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[sadrul, 2016-07-26 04:39:20]

https://codereview.chromium.org/2140783002/diff/80001/cc/ipc/render_pass_stru...
File cc/ipc/render_pass_struct_traits.cc (left):

https://codereview.chromium.org/2140783002/diff/80001/cc/ipc/render_pass_stru...
cc/ipc/render_pass_struct_traits.cc:27:
DCHECK(input->shared_quad_state_list.empty() ||
On 2016/07/25 19:17:35, enne wrote:
> ?

Oops. Removed.

https://codereview.chromium.org/2140783002/diff/80001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2140783002/diff/80001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:705: if (!damage_rect.IsEmpty() &&
surface_frame.metadata.may_contain_video)
On 2016/07/25 19:17:35, enne wrote:
> Why does the damage rect come into play here? If you had a 30 fps video but
the
> browser compositor had a tab spinner animation at 60fps, would you want the
may
> contain video to alternate true/false every other frame?

Yes. The window server will need to notify observers of inactivity based on a
timeout anyway (e.g. if a video is paused, and no new frame is submitted, we
need to notify that video has stopped playing). So if the flag is toggled every
frame, that wouldn't be problematic here.

[enne (OOO), 2016-07-26 21:43:54]

lgtm

https://codereview.chromium.org/2140783002/diff/80001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2140783002/diff/80001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:705: if (!damage_rect.IsEmpty() &&
surface_frame.metadata.may_contain_video)
On 2016/07/26 at 04:39:20, sadrul wrote:
> On 2016/07/25 19:17:35, enne wrote:
> > Why does the damage rect come into play here? If you had a 30 fps video but
the
> > browser compositor had a tab spinner animation at 60fps, would you want the
may
> > contain video to alternate true/false every other frame?
> 
> Yes. The window server will need to notify observers of inactivity based on a
timeout anyway (e.g. if a video is paused, and no new frame is submitted, we
need to notify that video has stopped playing). So if the flag is toggled every
frame, that wouldn't be problematic here.

Hmm, makes sense for paused videos, thanks for the explanation.

[sadrul, 2016-07-27 19:14:32]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 19:15:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 19:15:07]

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

[commit-bot: I haz the power, 2016-07-27 19:15:08]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[sadrul, 2016-07-27 19:55:56]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 19:56:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 19:56:07]

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

[commit-bot: I haz the power, 2016-07-27 19:56:08]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[sadrul, 2016-07-27 19:58:21]

sadrul@chromium.org changed reviewers:
+ bbudge@chromium.org, sky@chromium.org, tsepez@chromium.org

[sadrul, 2016-07-27 19:58:22]

Added a test in layer_tree_host_impl_unittest.cc, updated a test in
layer_unittest.cc

+tsepez@ for //cc/ipc

+bbudge@ for //content/renderer/pepper

+sky@ for //services/ui

[Tom Sepez, 2016-07-27 20:02:53]

lgtm

[sky, 2016-07-27 21:19:35]

https://codereview.chromium.org/2140783002/diff/140001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2140783002/diff/140001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:77: cc::CompositorFrame
frame(GenerateCompositorFrame());
It's mildly confusing that GenerateCompositorFrame and DrawWindowTree update
may_contain_video_. Can DrawWindowTree() take a boolean* that is set if there
may be video and this checks frame.meta.data.may_contain_video?

[bbudge, 2016-07-27 21:41:29]

content/renderer/pepper lgtm

[sadrul, 2016-07-28 00:43:56]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 00:44:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 00:44:07]

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

[commit-bot: I haz the power, 2016-07-28 00:44:08]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[sadrul, 2016-07-28 00:45:54]

Patchset #9 (id:160001) has been deleted

[sadrul, 2016-07-28 00:47:47]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 00:47:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 00:48:00]

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

[commit-bot: I haz the power, 2016-07-28 00:48:01]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[sadrul, 2016-07-28 00:48:05]

https://codereview.chromium.org/2140783002/diff/140001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2140783002/diff/140001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:77: cc::CompositorFrame
frame(GenerateCompositorFrame());
On 2016/07/27 21:19:34, sky wrote:
> It's mildly confusing that GenerateCompositorFrame and DrawWindowTree update
> may_contain_video_. Can DrawWindowTree() take a boolean* that is set if there
> may be video and this checks frame.meta.data.may_contain_video?

Done (also made GenerateCompositorFrame() and DrawWindowTree() const methods)

[sadrul, 2016-07-30 01:11:21]

sadrul@chromium.org changed reviewers:
+ ben@chromium.org

[sadrul, 2016-07-30 01:11:22]

+ben@ since sky is ooo

[sadrul, 2016-07-30 01:20:11]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-30 01:20:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-30 01:20:40]

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

[commit-bot: I haz the power, 2016-07-30 01:20:41]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[sadrul, 2016-07-30 01:49:43]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-30 01:50:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-30 01:50:01]

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

[commit-bot: I haz the power, 2016-07-30 01:50:03]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[sadrul, 2016-07-30 01:51:40]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-30 01:52:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-30 01:52:13]

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

[commit-bot: I haz the power, 2016-07-30 01:52:14]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[sadrul, 2016-07-30 02:10:20]

Description was changed from

==========
services/ui: Detect when a video is playing.

Compositor frames submitted by mus clients include a flag to indicate whether
the
client is playing a video or not. This information is plumbed through from
cc::Layer to cc::LayerImpl to cc::CompositorFrame[Metadata].

BUG=627140
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
services/ui: Detect when a video is playing.

Compositor frames submitted by mus clients include a flag to indicate whether
the
client is playing a video or not. This information is plumbed through from
cc::Layer to cc::LayerImpl to cc::CompositorFrame[Metadata].

BUG=627140
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[sadrul, 2016-07-30 02:10:28]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-30 02:10:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-30 03:05:41]

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

[commit-bot: I haz the power, 2016-07-30 03:05:42]

Dry run: This issue passed the CQ dry run.

[Ben Goodger (Google), 2016-07-30 03:18:14]

lgtm

[sadrul, 2016-07-30 05:22:27]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-07-30 05:22:28]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
tsepez@chromium.org, bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/2140783002/#ps200001
(title: "tot merge")

[commit-bot: I haz the power, 2016-07-30 05:22:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-30 05:26:36]

Description was changed from

==========
services/ui: Detect when a video is playing.

Compositor frames submitted by mus clients include a flag to indicate whether
the
client is playing a video or not. This information is plumbed through from
cc::Layer to cc::LayerImpl to cc::CompositorFrame[Metadata].

BUG=627140
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
services/ui: Detect when a video is playing.

Compositor frames submitted by mus clients include a flag to indicate whether
the
client is playing a video or not. This information is plumbed through from
cc::Layer to cc::LayerImpl to cc::CompositorFrame[Metadata].

BUG=627140
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-07-30 05:26:39]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-07-30 05:30:07]

Description was changed from

==========
services/ui: Detect when a video is playing.

Compositor frames submitted by mus clients include a flag to indicate whether
the
client is playing a video or not. This information is plumbed through from
cc::Layer to cc::LayerImpl to cc::CompositorFrame[Metadata].

BUG=627140
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
services/ui: Detect when a video is playing.

Compositor frames submitted by mus clients include a flag to indicate whether
the
client is playing a video or not. This information is plumbed through from
cc::Layer to cc::LayerImpl to cc::CompositorFrame[Metadata].

BUG=627140
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/32d1097c5645c0e1b0f49f93502d4f4701b67cec
Cr-Commit-Position: refs/heads/master@{#408864}
==========

[commit-bot: I haz the power, 2016-07-30 05:30:10]

Patchset 10 (id:??) landed as
https://crrev.com/32d1097c5645c0e1b0f49f93502d4f4701b67cec
Cr-Commit-Position: refs/heads/master@{#408864}