Mac Overlays: Allow multiple overlays with sandwich strategy

Mac Overlays: Allow multiple overlays with sandwich strategy.

Allow multiple videos to be promoted to overlays, as long as they don't overlap.

BUG=527222
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/99750ddf53c9901b1e9f70c2f3c038f24df2c623
Cr-Commit-Position: refs/heads/master@{#347805}

[Andre, 2015-09-02 21:28:46]

andresantoso@chromium.org changed reviewers:
+ ccameron@chromium.org

[Andre, 2015-09-02 21:28:47]

ccameron WDYT?
Am I missing an easier way to do this?
If this is the way to go, I can split it up and add tests.

[ccameron, 2015-09-02 21:41:56]

This looks like it should work. One style suggestion (pass iterators around by
pointer), and one correctness issue (make sure no overlays intersect).

https://codereview.chromium.org/1304053016/diff/1/cc/base/list_container.h
File cc/base/list_container.h (right):

https://codereview.chromium.org/1304053016/diff/1/cc/base/list_container.h#ne...
cc/base/list_container.h:229: // This function does not deallocate memory.
I'd pass position by pointer here (to match InsertBefore's behavior below).

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

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_common.cc:44: it =
delegate_->TryOverlay(capability_checker_,
Then this just reads

  ...
  if (delegate_->TryOverlay(...) == STOP)) break;
  if (it == quad_list.end()) break;
  ++it;
}

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
File cc/output/overlay_strategy_common.h (right):

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_common.h:21: virtual ~OverlayStrategyCommonDelegate()
{}
It might be easier to read to create an enum for the return type, something to
the effect of STOP_TRYING_OVERLAYS or KEEP_TRYING_OVERLAYS. And then pass the
the "iter" argument by pointer (continuing on the theme from ListContainer).

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

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_sandwich.cc:81:
pixel_covered_region.Union(pixel_covered_rect);
We should only promote to an overlay layer here if we don't intersect any of the
existing candidates.

[Andre, 2015-09-02 21:54:51]

Thanks!

https://codereview.chromium.org/1304053016/diff/1/cc/base/list_container.h
File cc/base/list_container.h (right):

https://codereview.chromium.org/1304053016/diff/1/cc/base/list_container.h#ne...
cc/base/list_container.h:229: // This function does not deallocate memory.
On 2015/09/02 21:41:55, ccameron wrote:
> I'd pass position by pointer here (to match InsertBefore's behavior below).

Hmm, but ListContainer::InsertBefore below returns an Iterator just like this.
The one that takes a pointer is ListContainerBase::InsertBefore, and I followed
that style with ListContainerBase::Erase.

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
File cc/output/overlay_strategy_common.h (right):

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_common.h:21: virtual ~OverlayStrategyCommonDelegate()
{}
On 2015/09/02 21:41:55, ccameron wrote:
> It might be easier to read to create an enum for the return type, something to
> the effect of STOP_TRYING_OVERLAYS or KEEP_TRYING_OVERLAYS. And then pass the
> the "iter" argument by pointer (continuing on the theme from ListContainer).

Do you still want this change if ListContainer::Erase returns an Iterator just
like InsertBefore?
I slightly prefer returning an Iterator. I think passing iter by pointer is less
clear at the call site that it's an input+output argument, especially with that
big list of arguments.

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

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_sandwich.cc:81:
pixel_covered_region.Union(pixel_covered_rect);
On 2015/09/02 21:41:55, ccameron wrote:
> We should only promote to an overlay layer here if we don't intersect any of
the
> existing candidates.

Got it.

[Andre, 2015-09-02 22:08:52]

https://codereview.chromium.org/1304053016/diff/1/cc/base/list_container.h
File cc/base/list_container.h (right):

https://codereview.chromium.org/1304053016/diff/1/cc/base/list_container.h#ne...
cc/base/list_container.h:229: // This function does not deallocate memory.
On 2015/09/02 21:54:51, Andre wrote:
> On 2015/09/02 21:41:55, ccameron wrote:
> > I'd pass position by pointer here (to match InsertBefore's behavior below).
> 
> Hmm, but ListContainer::InsertBefore below returns an Iterator just like this.
> The one that takes a pointer is ListContainerBase::InsertBefore, and I
followed
> that style with ListContainerBase::Erase.

Also, making it take a pointer would make code like this awkward:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/base/list_conta...

[ccameron, 2015-09-02 22:22:33]

https://codereview.chromium.org/1304053016/diff/1/cc/base/list_container.h
File cc/base/list_container.h (right):

https://codereview.chromium.org/1304053016/diff/1/cc/base/list_container.h#ne...
cc/base/list_container.h:229: // This function does not deallocate memory.
On 2015/09/02 21:54:51, Andre wrote:
> On 2015/09/02 21:41:55, ccameron wrote:
> > I'd pass position by pointer here (to match InsertBefore's behavior below).
> 
> Hmm, but ListContainer::InsertBefore below returns an Iterator just like this.
> The one that takes a pointer is ListContainerBase::InsertBefore, and I
followed
> that style with ListContainerBase::Erase.

Yes, you're right, that was a bad suggestion.

Maybe add to the comment saying that the returned iterator is the element after
|position|.

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

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_common.cc:44: it =
delegate_->TryOverlay(capability_checker_,
On 2015/09/02 21:41:55, ccameron wrote:
> Then this just reads
> 
>   ...
>   if (delegate_->TryOverlay(...) == STOP)) break;
>   if (it == quad_list.end()) break;
>   ++it;
> }

Errr...

  result = delegate_->TryOverlay(...)
  if (result == ...STOP) break
  if (result == DID_NOT_ADD) ++iter

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
File cc/output/overlay_strategy_common.h (right):

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_common.h:21: virtual ~OverlayStrategyCommonDelegate()
{}
On 2015/09/02 21:54:51, Andre wrote:
> On 2015/09/02 21:41:55, ccameron wrote:
> > It might be easier to read to create an enum for the return type, something
to
> > the effect of STOP_TRYING_OVERLAYS or KEEP_TRYING_OVERLAYS. And then pass
the
> > the "iter" argument by pointer (continuing on the theme from ListContainer).
> 
> Do you still want this change if ListContainer::Erase returns an Iterator just
> like InsertBefore?
> I slightly prefer returning an Iterator. I think passing iter by pointer is
less
> clear at the call site that it's an input+output argument, especially with
that
> big list of arguments.

I think there needs to be a clear "STOP_LOOKING_FOR_OVERLAYS versus
KEEP_LOOKING_FOR_OVERLAYS", and I think that the return value is the place for
it.

I think, actually, we need 3 return values
- CREATED_OVERLAY_STOP_LOOKING
  - in this case, we just return after TryOverlay
- CREATED_OVERLAY_KEEP_LOOKING
  - in this case, we keep calling TryOverlay. This requires that |iter| be
updated by TryOverlay (not too much to ask, because TryOverlay is already
already updated |candidate_list|
- DID_NOT_CREATE_OVERLAY
  - in this cause, we have to do ++iter

Blurgh, still not particularly clean.

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

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_single_on_top.cc:37: return quad_list.end();
This shouldn't return the end() iter -- we want to keep trying!

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_single_on_top.cc:57:
candidate_list->swap(candidates);
here we want to return STOP -- this strategy only allows 1 overlay.

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_single_on_top.cc:60: return quad_list.end();
likewise about keeping trying.

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

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_underlay.cc:50: candidate_list->swap(candidates);
same issue about return values here.

[Andre, 2015-09-03 18:28:31]

Patchset #3 (id:40001) has been deleted

[Andre, 2015-09-03 21:21:53]

Patchset #3 (id:60001) has been deleted

[Andre, 2015-09-03 22:44:31]

Patchset #4 (id:100001) has been deleted

[Andre, 2015-09-03 22:45:59]

ccameron PTAL.

https://codereview.chromium.org/1304053016/diff/1/cc/base/list_container.h
File cc/base/list_container.h (right):

https://codereview.chromium.org/1304053016/diff/1/cc/base/list_container.h#ne...
cc/base/list_container.h:229: // This function does not deallocate memory.
On 2015/09/02 22:22:33, ccameron wrote:
> On 2015/09/02 21:54:51, Andre wrote:
> > On 2015/09/02 21:41:55, ccameron wrote:
> > > I'd pass position by pointer here (to match InsertBefore's behavior
below).
> > 
> > Hmm, but ListContainer::InsertBefore below returns an Iterator just like
this.
> > The one that takes a pointer is ListContainerBase::InsertBefore, and I
> followed
> > that style with ListContainerBase::Erase.
> 
> Yes, you're right, that was a bad suggestion.
> 
> Maybe add to the comment saying that the returned iterator is the element
after
> |position|.

Done. This was split off into another CL.

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

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_common.cc:44: it =
delegate_->TryOverlay(capability_checker_,
On 2015/09/02 22:22:33, ccameron wrote:
> On 2015/09/02 21:41:55, ccameron wrote:
> > Then this just reads
> > 
> >   ...
> >   if (delegate_->TryOverlay(...) == STOP)) break;
> >   if (it == quad_list.end()) break;
> >   ++it;
> > }
> 
> Errr...
> 
>   result = delegate_->TryOverlay(...)
>   if (result == ...STOP) break
>   if (result == DID_NOT_ADD) ++iter
> 

Done.

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
File cc/output/overlay_strategy_common.h (right):

https://codereview.chromium.org/1304053016/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_common.h:21: virtual ~OverlayStrategyCommonDelegate()
{}
On 2015/09/02 22:22:33, ccameron wrote:
> On 2015/09/02 21:54:51, Andre wrote:
> > On 2015/09/02 21:41:55, ccameron wrote:
> > > It might be easier to read to create an enum for the return type,
something
> to
> > > the effect of STOP_TRYING_OVERLAYS or KEEP_TRYING_OVERLAYS. And then pass
> the
> > > the "iter" argument by pointer (continuing on the theme from
ListContainer).
> > 
> > Do you still want this change if ListContainer::Erase returns an Iterator
just
> > like InsertBefore?
> > I slightly prefer returning an Iterator. I think passing iter by pointer is
> less
> > clear at the call site that it's an input+output argument, especially with
> that
> > big list of arguments.
> 
> I think there needs to be a clear "STOP_LOOKING_FOR_OVERLAYS versus
> KEEP_LOOKING_FOR_OVERLAYS", and I think that the return value is the place for
> it.
> 
> I think, actually, we need 3 return values
> - CREATED_OVERLAY_STOP_LOOKING
>   - in this case, we just return after TryOverlay
> - CREATED_OVERLAY_KEEP_LOOKING
>   - in this case, we keep calling TryOverlay. This requires that |iter| be
> updated by TryOverlay (not too much to ask, because TryOverlay is already
> already updated |candidate_list|
> - DID_NOT_CREATE_OVERLAY
>   - in this cause, we have to do ++iter
> 
> Blurgh, still not particularly clean.

Done.

[Andre, 2015-09-03 22:52:19]

https://codereview.chromium.org/1304053016/diff/110001/cc/output/overlay_stra...
File cc/output/overlay_strategy_single_on_top.cc (left):

https://codereview.chromium.org/1304053016/diff/110001/cc/output/overlay_stra...
cc/output/overlay_strategy_single_on_top.cc:27:
draw_quad->shared_quad_state->quad_to_target_transform.TransformRect(&rect);
This calculation should be the same as just using candidate.display_rect, right?

[Andre, 2015-09-04 15:03:00]

andresantoso@chromium.org changed reviewers:
+ alexst@chromium.org

[Andre, 2015-09-04 15:03:00]

alexst PTAL.

[alexst (slow to review), 2015-09-04 15:32:10]

lgtm is a nit. This should work and scale for other strategies.

https://codereview.chromium.org/1304053016/diff/110001/cc/output/overlay_stra...
File cc/output/overlay_strategy_common.cc (right):

https://codereview.chromium.org/1304053016/diff/110001/cc/output/overlay_stra...
cc/output/overlay_strategy_common.cc:53: return true;
This is a little weird because on this path you don't touch |success| at all.
Maybe you could do

success = true;
break;

instead? That way everyone goes on the same exit path.

https://codereview.chromium.org/1304053016/diff/110001/cc/output/overlay_stra...
File cc/output/overlay_strategy_single_on_top.cc (left):

https://codereview.chromium.org/1304053016/diff/110001/cc/output/overlay_stra...
cc/output/overlay_strategy_single_on_top.cc:27:
draw_quad->shared_quad_state->quad_to_target_transform.TransformRect(&rect);
On 2015/09/03 22:52:19, Andre wrote:
> This calculation should be the same as just using candidate.display_rect,
right?

Yeah, looks like it is, good catch.

[Andre, 2015-09-04 17:22:47]

Thanks!

https://codereview.chromium.org/1304053016/diff/110001/cc/output/overlay_stra...
File cc/output/overlay_strategy_common.cc (right):

https://codereview.chromium.org/1304053016/diff/110001/cc/output/overlay_stra...
cc/output/overlay_strategy_common.cc:53: return true;
On 2015/09/04 15:32:10, alexst wrote:
> This is a little weird because on this path you don't touch |success| at all.
> Maybe you could do
> 
> success = true;
> break;
> 
> instead? That way everyone goes on the same exit path.

Done.

[Andre, 2015-09-04 17:23:00]

ccameron PTAL

[alexst (slow to review), 2015-09-04 17:53:57]

https://codereview.chromium.org/1304053016/diff/130001/cc/output/overlay_stra...
File cc/output/overlay_strategy_common.cc (right):

https://codereview.chromium.org/1304053016/diff/130001/cc/output/overlay_stra...
cc/output/overlay_strategy_common.cc:52: if (result ==
CREATED_OVERLAY_STOP_LOOKING) {
Actually, one more thought, we don't handle DID_NOT_CREATE_OVERLAY anywhere, I
know it's preferred to handle all enum cases so they don't accidentally fall
through the cracks and result in undefined behaviour. What do you think of a
switch statement instead?

[Andre, 2015-09-08 16:55:37]

https://codereview.chromium.org/1304053016/diff/130001/cc/output/overlay_stra...
File cc/output/overlay_strategy_common.cc (right):

https://codereview.chromium.org/1304053016/diff/130001/cc/output/overlay_stra...
cc/output/overlay_strategy_common.cc:52: if (result ==
CREATED_OVERLAY_STOP_LOOKING) {
On 2015/09/04 17:53:57, alexst wrote:
> Actually, one more thought, we don't handle DID_NOT_CREATE_OVERLAY anywhere, I
> know it's preferred to handle all enum cases so they don't accidentally fall
> through the cracks and result in undefined behaviour. What do you think of a
> switch statement instead?

Done.
The combination of continue (for the loop) and break (for the switch) didn't
look great, so I did a little refactor to improve it a bit.
PTAL.

[ccameron, 2015-09-08 17:16:06]

lgtm % small variable name nits.

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
File cc/output/overlay_strategy_common.cc (right):

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
cc/output/overlay_strategy_common.cc:42: bool success = false;
Maybe change "success" name to "created_overlay" (cause it's successful to have
none, after all).

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
cc/output/overlay_strategy_common.cc:170: auto* quad =
TextureDrawQuad::MaterialCast(&draw_quad);
Maybe move the allow_overlay() into GetTextureQuadInfo, GetVideoQuadInfo, and
GetIOSurfaceQuadInfo?

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
File cc/output/overlay_strategy_single_on_top.cc (right):

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
cc/output/overlay_strategy_single_on_top.cc:37: // Add the overlay.
Might be clearer to rename "candidates" to "new_candidate_list".

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
File cc/output/overlay_strategy_underlay.cc (right):

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
cc/output/overlay_strategy_underlay.cc:25: // Add the overlay.
(same as before)
Might be clearer to rename "candidates" to "new_candidate_list".

[alexst (slow to review), 2015-09-08 17:18:01]

lgtm

[Andre, 2015-09-08 17:30:03]

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
File cc/output/overlay_strategy_common.cc (right):

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
cc/output/overlay_strategy_common.cc:42: bool success = false;
On 2015/09/08 17:16:06, ccameron wrote:
> Maybe change "success" name to "created_overlay" (cause it's successful to
have
> none, after all).

Done.

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
cc/output/overlay_strategy_common.cc:170: auto* quad =
TextureDrawQuad::MaterialCast(&draw_quad);
On 2015/09/08 17:16:05, ccameron wrote:
> Maybe move the allow_overlay() into GetTextureQuadInfo, GetVideoQuadInfo, and
> GetIOSurfaceQuadInfo?

Done.

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
File cc/output/overlay_strategy_single_on_top.cc (right):

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
cc/output/overlay_strategy_single_on_top.cc:37: // Add the overlay.
On 2015/09/08 17:16:06, ccameron wrote:
> Might be clearer to rename "candidates" to "new_candidate_list".

Done.

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
File cc/output/overlay_strategy_underlay.cc (right):

https://codereview.chromium.org/1304053016/diff/150001/cc/output/overlay_stra...
cc/output/overlay_strategy_underlay.cc:25: // Add the overlay.
On 2015/09/08 17:16:06, ccameron wrote:
> (same as before)
> Might be clearer to rename "candidates" to "new_candidate_list".

Done.

[Andre, 2015-09-08 17:30:16]

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

[commit-bot: I haz the power, 2015-09-08 17:30:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304053016/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304053016/170001

[commit-bot: I haz the power, 2015-09-08 17:32:54]

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

[commit-bot: I haz the power, 2015-09-08 17:32:55]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Andre, 2015-09-08 19:34:45]

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

[commit-bot: I haz the power, 2015-09-08 19:35:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304053016/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304053016/190001

[commit-bot: I haz the power, 2015-09-08 21:52:16]

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

[commit-bot: I haz the power, 2015-09-08 21:52:17]

Dry run: This issue passed the CQ dry run.

[Andre, 2015-09-08 21:52:38]

The CQ bit was checked by andresantoso@chromium.org

[Andre, 2015-09-08 21:52:39]

The patchset sent to the CQ was uploaded after l-g-t-m from alexst@chromium.org,
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/1304053016/#ps190001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-08 21:53:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304053016/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304053016/190001

[commit-bot: I haz the power, 2015-09-08 22:00:41]

Committed patchset #8 (id:190001)

[commit-bot: I haz the power, 2015-09-08 22:01:32]

Patchset 8 (id:??) landed as
https://crrev.com/99750ddf53c9901b1e9f70c2f3c038f24df2c623
Cr-Commit-Position: refs/heads/master@{#347805}