cc: Reject non-opaque fullscreen overlay candidates.

cc: Reject non-opaque fullscreen overlay candidates.

When replacing the primary plane with a fullscreen overlay,
we need to make sure that the quad is completely opaque.

OverlayCandidate::FromDrawQuad currently rejects non-opaque drawquads.

Since we're planning to use non-opaque overlays, FromDrawQuad will allow
transparent candidates in the future.

This CL makes sure that the fullscreen strategy explicitly rejects
non-opaque quads and doesn't rely on OverlayCandidate::FromDrawQuad
that will soon change its behavior.

BUG=695296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2739473002
Cr-Commit-Position: refs/heads/master@{#456583}
Committed: https://chromium.googlesource.com/chromium/src/+/f38d0be624e674372eef59873b7da2206e8275df

[Daniele Castagna, 2017-03-06 20:51:10]

Description was changed from

==========
cc: Reject non-opaque fullscreen overlay candidates.

When replacing the primary plane with a fullscreen overlay,
we need to make sure that the quad is completely opaque.

OverlayCandidate::FromDrawQuad currently rejects non-opaque drawquads.

Since we're planning to use non-opaque overlays, FromDrawQuad will allow
transparent candidates in the future.

This CL makes sure that the fullscreen strategy explicitly rejects
non-opaque quads and doesn't rely on OverlayCandidate::FromDrawQuad
that will soon change its behavior.

BUG=695296
==========

to

==========
cc: Reject non-opaque fullscreen overlay candidates.

When replacing the primary plane with a fullscreen overlay,
we need to make sure that the quad is completely opaque.

OverlayCandidate::FromDrawQuad currently rejects non-opaque drawquads.

Since we're planning to use non-opaque overlays, FromDrawQuad will allow
transparent candidates in the future.

This CL makes sure that the fullscreen strategy explicitly rejects
non-opaque quads and doesn't rely on OverlayCandidate::FromDrawQuad
that will soon change its behavior.

BUG=695296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Daniele Castagna, 2017-03-06 20:51:12]

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

[commit-bot: I haz the power, 2017-03-06 20:51:52]

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, 2017-03-06 23:08:51]

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

[commit-bot: I haz the power, 2017-03-06 23:08:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Daniele Castagna, 2017-03-08 19:03:27]

dcastagna@chromium.org changed reviewers:
+ reveman@chromium.org

[reveman, 2017-03-08 21:40:49]

https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
File cc/output/overlay_strategy_fullscreen.cc (right):

https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_fullscreen.cc:44: quad->shared_quad_state->blend_mode
!= SkBlendMode::kSrcOver)
what about SkBlendMode::kSrc? Android uses this when no blending is required.

[Daniele Castagna, 2017-03-13 02:09:19]

https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
File cc/output/overlay_strategy_fullscreen.cc (right):

https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_fullscreen.cc:44: quad->shared_quad_state->blend_mode
!= SkBlendMode::kSrcOver)
On 2017/03/08 at 21:40:49, reveman wrote:
> what about SkBlendMode::kSrc? Android uses this when no blending is required.

Just wanted to keep it consistent with the check we've had in
OverlayCandidate::FromDrawQuad so far.
Do you want to allow kSrc as a fullscreen overlay in this CL? What kind of
content can I use to test that?

[reveman, 2017-03-13 02:14:49]

https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
File cc/output/overlay_strategy_fullscreen.cc (right):

https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_fullscreen.cc:44: quad->shared_quad_state->blend_mode
!= SkBlendMode::kSrcOver)
On 2017/03/13 at 02:09:19, Daniele Castagna wrote:
> On 2017/03/08 at 21:40:49, reveman wrote:
> > what about SkBlendMode::kSrc? Android uses this when no blending is
required.
> 
> Just wanted to keep it consistent with the check we've had in
OverlayCandidate::FromDrawQuad so far.
> Do you want to allow kSrc as a fullscreen overlay in this CL? What kind of
content can I use to test that?

Play store should be enough to test this I think. EXO sets the opaque properties
when it sees SRC blending but it still needs to leave blending as SRC to render
correctly.

[Daniele Castagna, 2017-03-13 02:24:04]

https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
File cc/output/overlay_strategy_fullscreen.cc (right):

https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_fullscreen.cc:44: quad->shared_quad_state->blend_mode
!= SkBlendMode::kSrcOver)
On 2017/03/13 at 02:14:49, reveman wrote:
> On 2017/03/13 at 02:09:19, Daniele Castagna wrote:
> > On 2017/03/08 at 21:40:49, reveman wrote:
> > > what about SkBlendMode::kSrc? Android uses this when no blending is
required.
> > 
> > Just wanted to keep it consistent with the check we've had in
OverlayCandidate::FromDrawQuad so far.
> > Do you want to allow kSrc as a fullscreen overlay in this CL? What kind of
content can I use to test that?
> 
> Play store should be enough to test this I think. EXO sets the opaque
properties when it sees SRC blending but it still needs to leave blending as SRC
to render correctly.

Play store uses kSrcOver, otherwise we'd reject it as overlay, while IIRC it
gets promoted to overlay today.
Let me check.

[Daniele Castagna, 2017-03-13 03:32:56]

https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
File cc/output/overlay_strategy_fullscreen.cc (right):

https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_fullscreen.cc:44: quad->shared_quad_state->blend_mode
!= SkBlendMode::kSrcOver)
On 2017/03/13 at 02:24:04, Daniele Castagna wrote:
> On 2017/03/13 at 02:14:49, reveman wrote:
> > On 2017/03/13 at 02:09:19, Daniele Castagna wrote:
> > > On 2017/03/08 at 21:40:49, reveman wrote:
> > > > what about SkBlendMode::kSrc? Android uses this when no blending is
required.
> > > 
> > > Just wanted to keep it consistent with the check we've had in
OverlayCandidate::FromDrawQuad so far.
> > > Do you want to allow kSrc as a fullscreen overlay in this CL? What kind of
content can I use to test that?
> > 
> > Play store should be enough to test this I think. EXO sets the opaque
properties when it sees SRC blending but it still needs to leave blending as SRC
to render correctly.
> 
> Play store uses kSrcOver, otherwise we'd reject it as overlay, while IIRC it
gets promoted to overlay today.
> Let me check.
Play Store is kSrcOver.

[reveman, 2017-03-13 11:58:12]

https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
File cc/output/overlay_strategy_fullscreen.cc (right):

https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_fullscreen.cc:44: quad->shared_quad_state->blend_mode
!= SkBlendMode::kSrcOver)
On 2017/03/13 at 03:32:56, Daniele Castagna wrote:
> On 2017/03/13 at 02:24:04, Daniele Castagna wrote:
> > On 2017/03/13 at 02:14:49, reveman wrote:
> > > On 2017/03/13 at 02:09:19, Daniele Castagna wrote:
> > > > On 2017/03/08 at 21:40:49, reveman wrote:
> > > > > what about SkBlendMode::kSrc? Android uses this when no blending is
required.
> > > > 
> > > > Just wanted to keep it consistent with the check we've had in
OverlayCandidate::FromDrawQuad so far.
> > > > Do you want to allow kSrc as a fullscreen overlay in this CL? What kind
of content can I use to test that?
> > > 
> > > Play store should be enough to test this I think. EXO sets the opaque
properties when it sees SRC blending but it still needs to leave blending as SRC
to render correctly.
> > 
> > Play store uses kSrcOver, otherwise we'd reject it as overlay, while IIRC it
gets promoted to overlay today.
> > Let me check.
> Play Store is kSrcOver.

Android M or N? What's the format of the buffer? I don't remember the exact
situations where kSrc was used but I know it's common for Android to use RGBA
formats and just set blending to kSrc to have us avoid the alpha channel. That's
the reason we have this API:
https://cs.chromium.org/chromium/src/components/exo/surface.cc?l=395

Where is the SkBlendMode::kSrcOver overlay check before this change? I'm failing
to see how this would not be a regression compared to before.

[Daniele Castagna, 2017-03-13 18:22:30]

On 2017/03/13 at 11:58:12, reveman wrote:
>
https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
> File cc/output/overlay_strategy_fullscreen.cc (right):
> 
>
https://codereview.chromium.org/2739473002/diff/1/cc/output/overlay_strategy_...
> cc/output/overlay_strategy_fullscreen.cc:44:
quad->shared_quad_state->blend_mode != SkBlendMode::kSrcOver)
> On 2017/03/13 at 03:32:56, Daniele Castagna wrote:
> > On 2017/03/13 at 02:24:04, Daniele Castagna wrote:
> > > On 2017/03/13 at 02:14:49, reveman wrote:
> > > > On 2017/03/13 at 02:09:19, Daniele Castagna wrote:
> > > > > On 2017/03/08 at 21:40:49, reveman wrote:
> > > > > > what about SkBlendMode::kSrc? Android uses this when no blending is
required.
> > > > > 
> > > > > Just wanted to keep it consistent with the check we've had in
OverlayCandidate::FromDrawQuad so far.
> > > > > Do you want to allow kSrc as a fullscreen overlay in this CL? What
kind of content can I use to test that?
> > > > 
> > > > Play store should be enough to test this I think. EXO sets the opaque
properties when it sees SRC blending but it still needs to leave blending as SRC
to render correctly.
> > > 
> > > Play store uses kSrcOver, otherwise we'd reject it as overlay, while IIRC
it gets promoted to overlay today.
> > > Let me check.
> > Play Store is kSrcOver.
> 
> Android M or N? What's the format of the buffer? I don't remember the exact
situations where kSrc was used but I know it's common for Android to use RGBA
formats and just set blending to kSrc to have us avoid the alpha channel. That's
the reason we have this API:
https://cs.chromium.org/chromium/src/components/exo/surface.cc?l=395
> 
It's Android M. The format is RGBA and kSrcOver.

> Where is the SkBlendMode::kSrcOver overlay check before this change? I'm
failing to see how this would not be a regression compared to before.
The previous check (that is exactly like this one) is in
OverlayCandidate::FromDrawQuad.

[reveman, 2017-03-13 23:02:12]

lgtm as consistent with OverlayCandidate::FromDrawQuad

[reveman, 2017-03-13 23:02:19]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2017-03-13 23:04:13]

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, 2017-03-13 23:14:51]

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

[commit-bot: I haz the power, 2017-03-13 23:14:52]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Daniele Castagna, 2017-03-13 23:28:41]

The CQ bit was checked by dcastagna@chromium.org

[Daniele Castagna, 2017-03-13 23:28:42]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2739473002/#ps20001
(title: "Rebase on master.")

[commit-bot: I haz the power, 2017-03-13 23:29:42]

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, 2017-03-14 02:27:11]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489447721905560,
"parent_rev": "3e08d0dd8c897cc06417cc3528f15e3ded54d46b", "commit_rev":
"f38d0be624e674372eef59873b7da2206e8275df"}

[commit-bot: I haz the power, 2017-03-14 02:27:53]

Description was changed from

==========
cc: Reject non-opaque fullscreen overlay candidates.

When replacing the primary plane with a fullscreen overlay,
we need to make sure that the quad is completely opaque.

OverlayCandidate::FromDrawQuad currently rejects non-opaque drawquads.

Since we're planning to use non-opaque overlays, FromDrawQuad will allow
transparent candidates in the future.

This CL makes sure that the fullscreen strategy explicitly rejects
non-opaque quads and doesn't rely on OverlayCandidate::FromDrawQuad
that will soon change its behavior.

BUG=695296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Reject non-opaque fullscreen overlay candidates.

When replacing the primary plane with a fullscreen overlay,
we need to make sure that the quad is completely opaque.

OverlayCandidate::FromDrawQuad currently rejects non-opaque drawquads.

Since we're planning to use non-opaque overlays, FromDrawQuad will allow
transparent candidates in the future.

This CL makes sure that the fullscreen strategy explicitly rejects
non-opaque quads and doesn't rely on OverlayCandidate::FromDrawQuad
that will soon change its behavior.

BUG=695296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2739473002
Cr-Commit-Position: refs/heads/master@{#456583}
Committed:
https://chromium.googlesource.com/chromium/src/+/f38d0be624e674372eef59873b7d...
==========

[commit-bot: I haz the power, 2017-03-14 02:27:54]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f38d0be624e674372eef59873b7d...