Let OverlayStrategySingleOnTop select non-topmost quad

Let OverlayStrategySingleOnTop select non-topmost quad

Old code was too restrictive, and did not properly select the correct quad.
New code chooses the topmost overlayable plane that does not intersect any
rects above it.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288049

[danakj, 2014-08-05 20:56:39]

Is there a test for this?


On Tue, Aug 5, 2014 at 4:51 PM, <achaulk@chromium.org> wrote:

> Reviewers: reveman,
>
> Description:
> Fix OverlayStrategySingleOnTop
>
> Old code was too restrictive, and did not properly select the correct quad.
> New code chooses the topmost overlayable plane that does not intersect any
> rects above it.
>
> Please review this at https://codereview.chromium.org/440193002/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+36, -13 lines):
>   M cc/output/overlay_candidate.cc
>   M cc/output/overlay_strategy_single_on_top.cc
>
>
> Index: cc/output/overlay_candidate.cc
> diff --git a/cc/output/overlay_candidate.cc b/cc/output/overlay_candidate.
> cc
> index 41682539a58223201db0063681e5d43a6d7f043d..
> 307bb22359e213487022f202ad5809fb316fc32c 100644
> --- a/cc/output/overlay_candidate.cc
> +++ b/cc/output/overlay_candidate.cc
> @@ -24,7 +24,7 @@ OverlayCandidate::~OverlayCandidate() {}
>  gfx::OverlayTransform OverlayCandidate::GetOverlayTransform(
>      const gfx::Transform& quad_transform,
>      bool flipped) {
> -  if (!quad_transform.IsIdentityOrTranslation())
> +  if (!quad_transform.IsPositiveScaleOrTranslation())
>      return gfx::OVERLAY_TRANSFORM_INVALID;
>
>    return flipped ? gfx::OVERLAY_TRANSFORM_FLIP_VERTICAL
> @@ -34,7 +34,7 @@ gfx::OverlayTransform OverlayCandidate::
> GetOverlayTransform(
>  // static
>  gfx::Rect OverlayCandidate::GetOverlayRect(const gfx::Transform&
> quad_transform,
>                                             const gfx::Rect& rect) {
> -  DCHECK(quad_transform.IsIdentityOrTranslation());
> +  DCHECK(quad_transform.IsPositiveScaleOrTranslation());
>
>    gfx::RectF float_rect(rect);
>    quad_transform.TransformRect(&float_rect);
> Index: cc/output/overlay_strategy_single_on_top.cc
> diff --git a/cc/output/overlay_strategy_single_on_top.cc
> b/cc/output/overlay_strategy_single_on_top.cc
> index e8971b845459e355a85719fe7c9e956b8e3a76e1..
> e0dfc3901940ff2796013fcf7b5a1b23bd335c91 100644
> --- a/cc/output/overlay_strategy_single_on_top.cc
> +++ b/cc/output/overlay_strategy_single_on_top.cc
> @@ -28,22 +28,46 @@ bool OverlayStrategySingleOnTop::Attempt(
>    DCHECK(root_render_pass);
>
>    QuadList& quad_list = root_render_pass->quad_list;
> -  const DrawQuad* candidate_quad = quad_list.front();
> -  if (candidate_quad->material != DrawQuad::TEXTURE_CONTENT)
> -    return false;
> -
> -  const TextureDrawQuad& quad = *TextureDrawQuad::
> MaterialCast(candidate_quad);
> -  if (!resource_provider_->AllowOverlay(quad.resource_id))
> +  QuadList::iterator candidate_iterator = quad_list.end();
> +  for (QuadList::iterator it = quad_list.begin(); it != quad_list.end();
> ++it) {
> +    const DrawQuad* draw_quad = *it;
> +    if (draw_quad->material == DrawQuad::TEXTURE_CONTENT) {
> +      const TextureDrawQuad& quad = *TextureDrawQuad::
> MaterialCast(draw_quad);
> +      if (!resource_provider_->AllowOverlay(quad.resource_id)) {
> +        continue;
> +      }
> +      // Check that no prior quads overlap it.
> +      bool intersects = false;
> +      gfx::RectF rect = draw_quad->rect;
> +      draw_quad->quadTransform().TransformRect(&rect);
> +      for (QuadList::iterator overlap_iter = quad_list.begin();
> +           overlap_iter != it;
> +           ++overlap_iter) {
> +        gfx::RectF overlap_rect = (*overlap_iter)->rect;
> +        (*overlap_iter)->quadTransform().TransformRect(&overlap_rect);
> +        if (rect.Intersects(overlap_rect)) {
> +          intersects = true;
> +          break;
> +        }
> +      }
> +      if (intersects)
> +        continue;
> +      candidate_iterator = it;
> +      break;
> +    }
> +  }
> +  if (candidate_iterator == quad_list.end())
>      return false;
> +  const TextureDrawQuad& quad =
> +      *TextureDrawQuad::MaterialCast(*candidate_iterator);
>
>    // Simple quads only.
>    gfx::OverlayTransform overlay_transform =
>        OverlayCandidate::GetOverlayTransform(quad.quadTransform(),
> quad.flipped);
>    if (overlay_transform == gfx::OVERLAY_TRANSFORM_INVALID ||
> -      !quad.quadTransform().IsIdentityOrTranslation() ||
> quad.needs_blending ||
> -      quad.shared_quad_state->opacity != 1.f ||
> +      quad.needs_blending || quad.shared_quad_state->opacity != 1.f ||
>        quad.shared_quad_state->blend_mode != SkXfermode::kSrcOver_Mode ||
> -      quad.premultiplied_alpha || quad.background_color !=
> SK_ColorTRANSPARENT)
> +      quad.background_color != SK_ColorTRANSPARENT)
>      return false;
>
>    // Add our primary surface.
> @@ -69,8 +93,7 @@ bool OverlayStrategySingleOnTop::Attempt(
>
>    // If the candidate can be handled by an overlay, create a pass for it.
>    if (candidates[1].overlay_handled) {
> -    scoped_ptr<DrawQuad> overlay_quad = quad_list.take(quad_list.
> begin());
> -    quad_list.erase(quad_list.begin());
> +    quad_list.erase(candidate_iterator);
>      candidate_list->swap(candidates);
>      return true;
>    }
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[achaulk, 2014-08-06 15:23:58]

tests are updated

[danakj, 2014-08-06 15:52:59]

Thanks

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
File cc/output/overlay_unittest.cc (left):

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:370: TEST_F(SingleOverlayOnTopTest,
RejectPremultipliedAlpha) {
Where'd this go?

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:48: if (candidate.display_rect.width() == 64)
need {} on the if and the else, since the body is multi-line

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:49: EXPECT_EQ(kOverlayBottomRightRect.ToString(),
we don't need to ToString() rects anymore, it happens automatically now :D

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:164: TextureDrawQuad*
CreateCandidateQuad(ResourceProvider* resource_provider,
CreateFullscreenCandidateQuad?

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:180: void CreateCheckeredQuad(ResourceProvider*
resource_provider,
CreateFullscreenCheckeredQuad?

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:459:
->content_to_target_transform.RotateAboutXAxis(45.f);
Why this change? Does scale not get rejected anymore? Should it?

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:469: TEST_F(SingleOverlayOnTopTest,
AllowNotTopIfNotOccluded) {
What if it is occluded, is that covered?

[achaulk, 2014-08-06 16:00:11]

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
File cc/output/overlay_unittest.cc (left):

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:370: TEST_F(SingleOverlayOnTopTest,
RejectPremultipliedAlpha) {
On 2014/08/06 15:52:59, danakj wrote:
> Where'd this go?

Deleted, seems to work fine on actual hardware (so far anyway)

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:49: EXPECT_EQ(kOverlayBottomRightRect.ToString(),
On 2014/08/06 15:52:59, danakj wrote:
> we don't need to ToString() rects anymore, it happens automatically now :D

Done.

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:459:
->content_to_target_transform.RotateAboutXAxis(45.f);
On 2014/08/06 15:52:59, danakj wrote:
> Why this change? Does scale not get rejected anymore? Should it?

The hardware should be capable of handling basic positive scaling - on link for
example the scale is always 2, but it works.

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:469: TEST_F(SingleOverlayOnTopTest,
AllowNotTopIfNotOccluded) {
On 2014/08/06 15:52:59, danakj wrote:
> What if it is occluded, is that covered?

Covered by another test

[danakj, 2014-08-06 16:03:34]

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
File cc/output/overlay_unittest.cc (left):

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:370: TEST_F(SingleOverlayOnTopTest,
RejectPremultipliedAlpha) {
On 2014/08/06 16:00:10, achaulk wrote:
> On 2014/08/06 15:52:59, danakj wrote:
> > Where'd this go?
> 
> Deleted, seems to work fine on actual hardware (so far anyway)

Ok, it seems unrelated to this CL, can you do it separately?

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/440193002/diff/40001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:459:
->content_to_target_transform.RotateAboutXAxis(45.f);
On 2014/08/06 16:00:10, achaulk wrote:
> On 2014/08/06 15:52:59, danakj wrote:
> > Why this change? Does scale not get rejected anymore? Should it?
> 
> The hardware should be capable of handling basic positive scaling - on link
for
> example the scale is always 2, but it works.

Can we do this as a separate CL so if we have problems you don't have to roll
out the whole thing with other unrelated changes?

[achaulk, 2014-08-06 17:25:29]

Sure, I can split that bit off. Done

[danakj, 2014-08-06 18:08:08]

Thanks! LGTM

[achaulk, 2014-08-06 18:08:23]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-08-06 18:09:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/440193002/80001

[commit-bot: I haz the power, 2014-08-06 22:08:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[commit-bot: I haz the power, 2014-08-06 22:30:37]

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

[commit-bot: I haz the power, 2014-08-06 22:30:38]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[achaulk, 2014-08-07 14:29:34]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-08-07 14:30:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/440193002/80001

[commit-bot: I haz the power, 2014-08-07 15:42:47]

Change committed as 288049