Change WebMediaPlayerMS layers transparency dynamically

Change WebMediaPlayerMS layers transparency dynamically

This CL changes how WebMediaPlayerMS sets its transparency
based on the incoming frame formats.

BUG=647886
Committed: https://crrev.com/5ddf895c72c8c5fe77f1f88698eaeaadf93b880e
Cr-Commit-Position: refs/heads/master@{#419816}

[emircan, 2016-09-16 23:57:03]

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

[commit-bot: I haz the power, 2016-09-16 23:57:14]

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-09-17 00:07:44]

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[emircan, 2016-09-17 00:16:06]

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

[commit-bot: I haz the power, 2016-09-17 00:16:35]

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

[emircan, 2016-09-17 00:18:56]

Patchset #1 (id:1) has been deleted

[emircan, 2016-09-17 00:26:43]

Description was changed from

==========
dynamic change.

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

to

==========
Change WebMediaPlayerMS layers transparency dynamically	

This CL changes how WebMediaPlayerMS sets its transparency
based on the incoming frame formats.

BUG=647886
==========

[commit-bot: I haz the power, 2016-09-17 01:09:32]

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

[commit-bot: I haz the power, 2016-09-17 01:09:33]

Dry run: This issue passed the CQ dry run.

[emircan, 2016-09-19 20:02:58]

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

[commit-bot: I haz the power, 2016-09-19 20:03:42]

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-09-19 20:55:37]

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

[commit-bot: I haz the power, 2016-09-19 20:55:38]

Dry run: This issue passed the CQ dry run.

[emircan, 2016-09-19 23:46:54]

Patchset #1 (id:20001) has been deleted

[emircan, 2016-09-19 23:53:55]

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

[emircan, 2016-09-19 23:54:02]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-09-19 23:54:34]

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

[emircan, 2016-09-19 23:55:33]

emircan@chromium.org changed reviewers:
+ mcasas@chromium.org

[emircan, 2016-09-19 23:55:33]

PTAL.

This is addressing the same issue as
https://codereview.chromium.org/2339983007/. However, rather than a revert, I am
trying to accomodate for both cases here.

[mcasas, 2016-09-20 00:35:14]

lgtm % comments below.

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:522: // Change layer's transparency
dynamically with incoming frames.
Maybe add a comment here e.g.
"...only configure opacity on changes, since it's an 
expensive operation https://crbug.com/647886".

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:523: if (video_frame_provider_.get()
&& video_weblayer_ &&
Isn't video_frame_provider_.get() redundant?
It isn't used in the body of the if(), and someone
must have provided |frame|.

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms_unittest.cc (right):

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_unittest.cc:776: CHECK(web_layer_);
Don't use CHECK()s in unittests, since they
crash the executable and prevent continuing
with other test cases. Instead, use ASSERT().

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_unittest.cc:783: transparent_tokens +
sizeof(transparent_tokens) / sizeof(int));
|transparent_tokens| is static, so we can use here
arraysize():
std::vector<int> transparent_timestamps(
    transparent_tokens,
    transparent_tokens + arraysize(transparent_tokens));

Also applies to l.764;

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_unittest.cc:790:
provider->QueueFrames(opaque_timestamps, true);
|opaque_timestamps| has a single frame with a 
timestamp 0, does it mean the class under test
doesn't care about timestamps? If so, we don't
need |transparent_timestamps| and we can use
a dummy static int dummy_tokens{} = {...} etc.

[commit-bot: I haz the power, 2016-09-20 01:03:58]

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

[commit-bot: I haz the power, 2016-09-20 01:03:58]

Dry run: This issue passed the CQ dry run.

[emircan, 2016-09-20 02:53:05]

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

[commit-bot: I haz the power, 2016-09-20 02:53:25]

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

[emircan, 2016-09-20 03:00:06]

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:522: // Change layer's transparency
dynamically with incoming frames.
On 2016/09/20 00:35:14, mcasas wrote:
> Maybe add a comment here e.g.
> "...only configure opacity on changes, since it's an 
> expensive operation https://crbug.com/647886%22.

Done.

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:523: if (video_frame_provider_.get()
&& video_weblayer_ &&
On 2016/09/20 00:35:14, mcasas wrote:
> Isn't video_frame_provider_.get() redundant?
> It isn't used in the body of the if(), and someone
> must have provided |frame|.

Done.

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms_unittest.cc (right):

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_unittest.cc:776: CHECK(web_layer_);
On 2016/09/20 00:35:14, mcasas wrote:
> Don't use CHECK()s in unittests, since they
> crash the executable and prevent continuing
> with other test cases. Instead, use ASSERT().

Done.

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_unittest.cc:783: transparent_tokens +
sizeof(transparent_tokens) / sizeof(int));
On 2016/09/20 00:35:14, mcasas wrote:
> |transparent_tokens| is static, so we can use here
> arraysize():
> std::vector<int> transparent_timestamps(
>     transparent_tokens,
>     transparent_tokens + arraysize(transparent_tokens));
> 
> Also applies to l.764;

Done.

https://codereview.chromium.org/2348903003/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_unittest.cc:790:
provider->QueueFrames(opaque_timestamps, true);
On 2016/09/20 00:35:14, mcasas wrote:
> |opaque_timestamps| has a single frame with a 
> timestamp 0, does it mean the class under test
> doesn't care about timestamps? If so, we don't
> need |transparent_timestamps| and we can use
> a dummy static int dummy_tokens{} = {...} etc.

We don't care about timestamps, except the TestBrake which helps us check status
between calls. I just wanted to be explicit with defining new frames, but I
agree that using default tokens would make it more clear.

[commit-bot: I haz the power, 2016-09-20 03:37:15]

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

[commit-bot: I haz the power, 2016-09-20 03:37:17]

Dry run: This issue passed the CQ dry run.

[emircan, 2016-09-20 18:12:16]

The CQ bit was checked by emircan@chromium.org

[emircan, 2016-09-20 18:12:16]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2348903003/#ps80001
(title: "mcasas@ comments.")

[commit-bot: I haz the power, 2016-09-20 18:12:50]

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-09-20 18:18:46]

Description was changed from

==========
Change WebMediaPlayerMS layers transparency dynamically	

This CL changes how WebMediaPlayerMS sets its transparency
based on the incoming frame formats.

BUG=647886
==========

to

==========
Change WebMediaPlayerMS layers transparency dynamically	

This CL changes how WebMediaPlayerMS sets its transparency
based on the incoming frame formats.

BUG=647886
==========

[commit-bot: I haz the power, 2016-09-20 18:18:48]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2016-09-20 18:21:20]

Description was changed from

==========
Change WebMediaPlayerMS layers transparency dynamically	

This CL changes how WebMediaPlayerMS sets its transparency
based on the incoming frame formats.

BUG=647886
==========

to

==========
Change WebMediaPlayerMS layers transparency dynamically

This CL changes how WebMediaPlayerMS sets its transparency
based on the incoming frame formats.

BUG=647886

Committed: https://crrev.com/5ddf895c72c8c5fe77f1f88698eaeaadf93b880e
Cr-Commit-Position: refs/heads/master@{#419816}
==========

[commit-bot: I haz the power, 2016-09-20 18:21:22]

Patchset 2 (id:??) landed as
https://crrev.com/5ddf895c72c8c5fe77f1f88698eaeaadf93b880e
Cr-Commit-Position: refs/heads/master@{#419816}