ozone: introduce OverlayCheckReturn_Params

ozone: introduce OverlayCheckReturn_Params

OverlayCheck_Params.is_overlay_candidate is used for both input
and output of validation. GPU callback slightly changes this value,
which is the member of the key of the cache map. It can cause subtle bugs.

For example, renderer checks if video layer is overlayable with
"is_overlay_candidate = true", but GPU process backend responds
"is_overlay_candidate = false", so the callback changes the key value.
At next time, renderer cannot find the result in the cache and request
overlay validation to GPU process again.

To resolve it as well as make the code understandable, this CL introduces
OverlayCheckReturn_Params as the output of validation.

BUG=683347
TEST=Run chrome on samus, ozone_unittests

Review-Url: https://codereview.chromium.org/2896173002
Cr-Commit-Position: refs/heads/master@{#477128}
Committed: https://chromium.googlesource.com/chromium/src/+/8ab26031647c8c612b1a051868fdc18c5d7db3f9

[inactive_dshwang_plz_cc_intel, 2017-05-23 05:17:29]

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

[commit-bot: I haz the power, 2017-05-23 05:17:45]

Dry run: CQ is trying da patch.

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

[dshwang, 2017-05-23 05:18:32]

dongseong.hwang@intel.com changed reviewers:
+ alexst@chromium.org, dcastagna@chromium.org, spang@chromium.org

[dshwang, 2017-05-23 05:18:33]

spang, alexst: could you review? current ozone hardly promote overlay. This CL
fixes it.

[commit-bot: I haz the power, 2017-05-23 05:43:58]

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

[commit-bot: I haz the power, 2017-05-23 05:43:59]

Dry run: This issue passed the CQ dry run.

[Daniele Castagna, 2017-05-23 06:44:58]

On 2017/05/23 at 05:18:33, dongseong.hwang wrote:
> spang, alexst: could you review? current ozone hardly promote overlay. This CL
fixes it.

Why would DrmOverlayValidator::TestPageFlip() be called before
HardwareDisplayController is installed?

[spang, 2017-05-25 23:02:20]

Description was changed from

==========
ozone: fix broken overlay promotion.

DrmOverlayValidator::TestPageFlip() can be called before
HardwareDisplayController is installed. In the case, it reports any planes are
not overlayable. However, the client keeps this information as cache and the
overlay never happens.

In the case, this CL makes the validator report Invalid mark and the client
will request the validation again.

In addition, OverlayCheck_Params.is_overlay_candidate is used for both input
and output of validation. It causes subtle bugs. e.g. the client wants to
validate top plane overlay candidate, but GpuSentOverlayResult callback can
change key's |is_overlay_candidate| to false, and the key is never completed.
To resolve it as well as make the code understandable, this CL introduces
OverlayCheckReturn_Params as the output of validation.

BUG=683347
TEST=Run chrome on samus, ozone_unittests
==========

to

==========
ozone: fix broken overlay promotion.

DrmOverlayValidator::TestPageFlip() can be called before
HardwareDisplayController is installed. In the case, it reports any planes are
not overlayable. However, the client keeps this information as cache and the
overlay never happens.

In the case, this CL makes the validator report Invalid mark and the client
will request the validation again.

In addition, OverlayCheck_Params.is_overlay_candidate is used for both input
and output of validation. It causes subtle bugs. e.g. the client wants to
validate top plane overlay candidate, but GpuSentOverlayResult callback can
change key's |is_overlay_candidate| to false, and the key is never completed.
To resolve it as well as make the code understandable, this CL introduces
OverlayCheckReturn_Params as the output of validation.

BUG=683347
TEST=Run chrome on samus, ozone_unittests
==========

[spang, 2017-05-25 23:02:20]

spang@chromium.org changed reviewers:
+ dnicoara@chromium.org

[spang, 2017-05-25 23:02:32]

+dnicoara

[dshwang, 2017-05-26 22:33:54]

On 2017/05/23 06:44:58, Daniele Castagna wrote:
> On 2017/05/23 at 05:18:33, dongseong.hwang wrote:
> > spang, alexst: could you review? current ozone hardly promote overlay. This
CL
> fixes it.
> 
> Why would DrmOverlayValidator::TestPageFlip() be called before
> HardwareDisplayController is installed?

Sorry for late response. I'm back from business trip. That's good point. 
When ChromeOS Browser launch with web page which has overlayable plane.

For example, when I run chrome on chromeos device by terminal, it happens
./chrome --ui-prioritize-in-gpu-process --use-gl=egl
--user-data-dir=/home/chronos/ --bwsi --login-user='$guest' --login-profile=user
--incognito --use-cras --enable-hardware-overlays --enable-drm-atomic
http://browsertests.herokuapp.com/media/fullscreen.html

It's kind of rare edge case. If you don't want to fix it, I'll change this CL to
be just refactorying CL.

[dnicoara, 2017-05-29 15:17:57]

On 2017/05/26 22:33:54, dshwang wrote:
> On 2017/05/23 06:44:58, Daniele Castagna wrote:
> > On 2017/05/23 at 05:18:33, dongseong.hwang wrote:
> > > spang, alexst: could you review? current ozone hardly promote overlay.
This
> CL
> > fixes it.
> > 
> > Why would DrmOverlayValidator::TestPageFlip() be called before
> > HardwareDisplayController is installed?
> 
> Sorry for late response. I'm back from business trip. That's good point. 
> When ChromeOS Browser launch with web page which has overlayable plane.
> 
> For example, when I run chrome on chromeos device by terminal, it happens
> ./chrome --ui-prioritize-in-gpu-process --use-gl=egl
> --user-data-dir=/home/chronos/ --bwsi --login-user='$guest'
--login-profile=user
> --incognito --use-cras --enable-hardware-overlays --enable-drm-atomic
> http://browsertests.herokuapp.com/media/fullscreen.html
> 
> It's kind of rare edge case. If you don't want to fix it, I'll change this CL
to
> be just refactorying CL.

Any idea how we're getting in that case? I'd expect it to never happen since the
cache is reset right after the window location is updated. Since the GPU
executes all queries sequentially, the window should be matched with a display
by the time an overlay query comes in. The only case this isn't true would be if
the window doesn't have any display (headless), but in that case it shouldn't
matter.

Also, for the second issue, could you please point out where the issue is? Is it
drm_overlay_manager.cc? Are the fixes related (wondering if they could be fixed
in separate changes for clarity)?

[inactive_dshwang_plz_cc_intel, 2017-05-31 18:48:22]

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

[commit-bot: I haz the power, 2017-05-31 18:49:18]

Dry run: CQ is trying da patch.

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

[dshwang, 2017-05-31 18:55:29]

Description was changed from

==========
ozone: fix broken overlay promotion.

DrmOverlayValidator::TestPageFlip() can be called before
HardwareDisplayController is installed. In the case, it reports any planes are
not overlayable. However, the client keeps this information as cache and the
overlay never happens.

In the case, this CL makes the validator report Invalid mark and the client
will request the validation again.

In addition, OverlayCheck_Params.is_overlay_candidate is used for both input
and output of validation. It causes subtle bugs. e.g. the client wants to
validate top plane overlay candidate, but GpuSentOverlayResult callback can
change key's |is_overlay_candidate| to false, and the key is never completed.
To resolve it as well as make the code understandable, this CL introduces
OverlayCheckReturn_Params as the output of validation.

BUG=683347
TEST=Run chrome on samus, ozone_unittests
==========

to

==========
ozone: introduce OverlayCheckReturn_Params

OverlayCheck_Params.is_overlay_candidate is used for both input
and output of validation. GPU callback slightly changes this value,
which is the member of the key of the cache map.
It can cause subtle bugs.
To resolve it as well as make the code understandable, this CL introduces
OverlayCheckReturn_Params as the output of validation.

BUG=683347
TEST=Run chrome on samus, ozone_unittests
==========

[dshwang, 2017-05-31 20:58:28]

Description was changed from

==========
ozone: introduce OverlayCheckReturn_Params

OverlayCheck_Params.is_overlay_candidate is used for both input
and output of validation. GPU callback slightly changes this value,
which is the member of the key of the cache map.
It can cause subtle bugs.
To resolve it as well as make the code understandable, this CL introduces
OverlayCheckReturn_Params as the output of validation.

BUG=683347
TEST=Run chrome on samus, ozone_unittests
==========

to

==========
ozone: introduce OverlayCheckReturn_Params

OverlayCheck_Params.is_overlay_candidate is used for both input
and output of validation. GPU callback slightly changes this value,
which is the member of the key of the cache map. It can cause subtle bugs.

For example, renderer checks if video layer is overlayable with
"is_overlay_candidate = true", but GPU process backend responds
"is_overlay_candidate = false", so the callback changes the key value.
At next time, renderer cannot find the result in the cache and request
overlay validation to GPU process again.

To resolve it as well as make the code understandable, this CL introduces
OverlayCheckReturn_Params as the output of validation.

BUG=683347
TEST=Run chrome on samus, ozone_unittests
==========

[dshwang, 2017-05-31 21:05:11]

Thank you for reviewing. I changes this CL as refactorying CL. Could you review
again?

On 2017/05/29 15:17:57, dnicoara wrote:
> Any idea how we're getting in that case? I'd expect it to never happen since
the
> cache is reset right after the window location is updated. Since the GPU
> executes all queries sequentially, the window should be matched with a display
> by the time an overlay query comes in. The only case this isn't true would be
if
> the window doesn't have any display (headless), but in that case it shouldn't
> matter.

I separate bug and refactorying. This issue is extracted to
https://codereview.chromium.org/2920613002

https://codereview.chromium.org/2896173002/diff/20001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_overlay_manager.cc (left):

https://codereview.chromium.org/2896173002/diff/20001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_overlay_manager.cc:120: cache_.Put(params,
false);
> Also, for the second issue, could you please point out where the issue is? Is
it
> drm_overlay_manager.cc? Are the fixes related (wondering if they could be
fixed
> in separate changes for clarity)?

It's the root of potential bug.

params.back().is_overlay_candidate can be false, while the requested
params.back().is_overlay_candidate is true. And then the request lost the key.

This CL separate request input (i.e. is_overlay_candidate) and response output
(i.e. status)

[commit-bot: I haz the power, 2017-05-31 21:09:00]

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

[commit-bot: I haz the power, 2017-05-31 21:09:01]

Dry run: This issue passed the CQ dry run.

[dshwang, 2017-06-01 19:20:35]

dnicoara, could you review this clean-up refactorying?

[dnicoara, 2017-06-01 19:28:54]

On 2017/06/01 19:20:35, dshwang wrote:
> dnicoara, could you review this clean-up refactorying?

Yes, will do.

[dnicoara, 2017-06-02 21:31:25]

https://codereview.chromium.org/2896173002/diff/20001/ui/ozone/common/gpu/ozo...
File ui/ozone/common/gpu/ozone_gpu_message_params.h (right):

https://codereview.chromium.org/2896173002/diff/20001/ui/ozone/common/gpu/ozo...
ui/ozone/common/gpu/ozone_gpu_message_params.h:81: enum Status { Pending, Able,
Not, Last = Not };
Enum constants should be ALL_CAPS.

https://codereview.chromium.org/2896173002/diff/20001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_overlay_manager.cc (right):

https://codereview.chromium.org/2896173002/diff/20001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_overlay_manager.cc:93:
DCHECK(candidate.plane_z_order != 0);
Why did you removed the "overlay_params.at(i).is_overlay_candidate = false;"
statement?

[inactive_dshwang_plz_cc_intel, 2017-06-02 23:57:33]

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

[commit-bot: I haz the power, 2017-06-02 23:58:22]

Dry run: CQ is trying da patch.

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

[dshwang, 2017-06-02 23:59:00]

dnicoara, thx for reviewing. could you review again?

https://codereview.chromium.org/2896173002/diff/20001/ui/ozone/common/gpu/ozo...
File ui/ozone/common/gpu/ozone_gpu_message_params.h (right):

https://codereview.chromium.org/2896173002/diff/20001/ui/ozone/common/gpu/ozo...
ui/ozone/common/gpu/ozone_gpu_message_params.h:81: enum Status { Pending, Able,
Not, Last = Not };
On 2017/06/02 21:31:25, dnicoara wrote:
> Enum constants should be ALL_CAPS.

Done.

https://codereview.chromium.org/2896173002/diff/20001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_overlay_manager.cc (right):

https://codereview.chromium.org/2896173002/diff/20001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_overlay_manager.cc:93:
DCHECK(candidate.plane_z_order != 0);
On 2017/06/02 21:31:25, dnicoara wrote:
> Why did you removed the "overlay_params.at(i).is_overlay_candidate = false;"
> statement?

I didn't like to change the key after querying `cache_.Get(overlay_params);`
already at line 66. and I thought gpu process can handle it. However, I have
better idea now. see next patch set.

https://codereview.chromium.org/2896173002/diff/40001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_overlay_manager.cc (right):

https://codereview.chromium.org/2896173002/diff/40001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_overlay_manager.cc:66:
overlay_params.back().is_overlay_candidate = false;
move this logic to here. now the key is same, no matter cached or request.

[commit-bot: I haz the power, 2017-06-03 00:26:38]

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

[commit-bot: I haz the power, 2017-06-03 00:26:39]

Dry run: This issue passed the CQ dry run.

[dnicoara, 2017-06-05 21:05:09]

dnicoara@chromium.org changed reviewers:
+ kenrb@chromium.org

[dnicoara, 2017-06-05 21:05:09]

lgtm

+kenrb@ for security review

https://codereview.chromium.org/2896173002/diff/40001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_overlay_manager.cc (right):

https://codereview.chromium.org/2896173002/diff/40001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_overlay_manager.cc:66:
overlay_params.back().is_overlay_candidate = false;
On 2017/06/02 23:59:00, dshwang wrote:
> move this logic to here. now the key is same, no matter cached or request.

Nice cleanup! Thanks :)

[kenrb, 2017-06-05 21:20:01]

ipc lgtm

[dshwang, 2017-06-05 23:39:49]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2017-06-05 23:40:55]

CQ is trying da patch.

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

[dshwang, 2017-06-05 23:42:05]

thank you for reviewing!

[commit-bot: I haz the power, 2017-06-06 00:13:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496705989629240,
"parent_rev": "22393627cd746171f238bf3cde20fa15db66997d", "commit_rev":
"8ab26031647c8c612b1a051868fdc18c5d7db3f9"}

[commit-bot: I haz the power, 2017-06-06 00:13:59]

Description was changed from

==========
ozone: introduce OverlayCheckReturn_Params

OverlayCheck_Params.is_overlay_candidate is used for both input
and output of validation. GPU callback slightly changes this value,
which is the member of the key of the cache map. It can cause subtle bugs.

For example, renderer checks if video layer is overlayable with
"is_overlay_candidate = true", but GPU process backend responds
"is_overlay_candidate = false", so the callback changes the key value.
At next time, renderer cannot find the result in the cache and request
overlay validation to GPU process again.

To resolve it as well as make the code understandable, this CL introduces
OverlayCheckReturn_Params as the output of validation.

BUG=683347
TEST=Run chrome on samus, ozone_unittests
==========

to

==========
ozone: introduce OverlayCheckReturn_Params

OverlayCheck_Params.is_overlay_candidate is used for both input
and output of validation. GPU callback slightly changes this value,
which is the member of the key of the cache map. It can cause subtle bugs.

For example, renderer checks if video layer is overlayable with
"is_overlay_candidate = true", but GPU process backend responds
"is_overlay_candidate = false", so the callback changes the key value.
At next time, renderer cannot find the result in the cache and request
overlay validation to GPU process again.

To resolve it as well as make the code understandable, this CL introduces
OverlayCheckReturn_Params as the output of validation.

BUG=683347
TEST=Run chrome on samus, ozone_unittests

Review-Url: https://codereview.chromium.org/2896173002
Cr-Commit-Position: refs/heads/master@{#477128}
Committed:
https://chromium.googlesource.com/chromium/src/+/8ab26031647c8c612b1a051868fd...
==========

[commit-bot: I haz the power, 2017-06-06 00:14:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8ab26031647c8c612b1a051868fd...