Overlays: Remove special casing of primary overlay plane

Overlays: Remove special casing of primary overlay plane

Mac will soon want to skip drawing the primary overlay plane, which
now can be accomplished by having the overlay strategy simply not emit
it.

BUG=533687
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e532ed0710935ef66a860b08b8e2a1e36a426b27
Cr-Commit-Position: refs/heads/master@{#352186}

[ccameron, 2015-09-29 21:17:12]

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

[ccameron, 2015-09-29 21:17:13]

ptal

[alexst (slow to review), 2015-09-29 21:27:36]

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co...

schedules the plane by itself, so it will need to be modified and directed down
the use primary for overlay path.

[ccameron, 2015-09-29 21:45:15]

Uah, I too aggressively pruned my change there. Updated.

[alexst (slow to review), 2015-09-29 22:01:38]

alexst@chromium.org changed reviewers:
+ achaulk@chromium.org, dnicoara@chromium.org

[alexst (slow to review), 2015-09-29 22:01:39]

lgtm, but please wait until Danny and/or Albert have a quick look to double
check.

[ccameron, 2015-09-30 18:38:57]

dnicoara, achaulk: any remarks, or is this good to go?

[achaulk, 2015-09-30 19:04:19]

https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
File cc/output/overlay_processor.cc (left):

https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
cc/output/overlay_processor.cc:28: void OverlayProcessor::ProcessForOverlays(
This doesn't always get called, because there are situations where it can do
nothing, which will also stop rendering. I suggest you move the primary plane
add to DirectRenderer::DrawFrame

https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
File cc/output/overlay_processor.cc (right):

https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
cc/output/overlay_processor.cc:35: if (strategies_.empty())
Having no strategies should be valid, but this will cause the list to not get
populated which means there will be no planes scheduled

https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
cc/output/overlay_processor.cc:41: primary_surface.display_rect =
gfx::RectF(root_render_pass->output_rect);
This needs to be in texture coordinates, eg 0..1

While ignored by current implementations, it might not always be

[ccameron, 2015-10-01 18:59:31]

Thanks -- updated

https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
File cc/output/overlay_processor.cc (right):

https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
cc/output/overlay_processor.cc:35: if (strategies_.empty())
On 2015/09/30 19:04:19, achaulk wrote:
> Having no strategies should be valid, but this will cause the list to not get
> populated which means there will be no planes scheduled

Moving the primary plane to DirectRenderer cleared this up.

https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
cc/output/overlay_processor.cc:41: primary_surface.display_rect =
gfx::RectF(root_render_pass->output_rect);
On 2015/09/30 19:04:19, achaulk wrote:
> This needs to be in texture coordinates, eg 0..1
> 
> While ignored by current implementations, it might not always be

Are you sure -- display_rect is read in GLRenderer::ScheduleOverlays, and is
rounded to the nearest integer.

I suggest we delete one of display_rect and quad_rect_in_target_space, to
clarify things.

[achaulk, 2015-10-01 19:41:46]

https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
File cc/output/overlay_processor.cc (right):

https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
cc/output/overlay_processor.cc:41: primary_surface.display_rect =
gfx::RectF(root_render_pass->output_rect);
On 2015/10/01 18:59:31, ccameron wrote:
> On 2015/09/30 19:04:19, achaulk wrote:
> > This needs to be in texture coordinates, eg 0..1
> > 
> > While ignored by current implementations, it might not always be
> 
> Are you sure -- display_rect is read in GLRenderer::ScheduleOverlays, and is
> rounded to the nearest integer.
> 
> I suggest we delete one of display_rect and quad_rect_in_target_space, to
> clarify things.

Oh right, this one is pixel coordinates. You do need to set uv_rect to 0,0 & 1,1

I'm not actually sure what quad_rect_in_target_space is for

[alexst (slow to review), 2015-10-01 19:46:37]

On 2015/10/01 19:41:46, achaulk wrote:
>
https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
> File cc/output/overlay_processor.cc (right):
> 
>
https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
> cc/output/overlay_processor.cc:41: primary_surface.display_rect =
> gfx::RectF(root_render_pass->output_rect);
> On 2015/10/01 18:59:31, ccameron wrote:
> > On 2015/09/30 19:04:19, achaulk wrote:
> > > This needs to be in texture coordinates, eg 0..1
> > > 
> > > While ignored by current implementations, it might not always be
> > 
> > Are you sure -- display_rect is read in GLRenderer::ScheduleOverlays, and is
> > rounded to the nearest integer.
> > 
> > I suggest we delete one of display_rect and quad_rect_in_target_space, to
> > clarify things.
> 
> Oh right, this one is pixel coordinates. You do need to set uv_rect to 0,0 &
1,1
> 
> I'm not actually sure what quad_rect_in_target_space is for

that one is from here https://codereview.chromium.org/1311013002/

[achaulk, 2015-10-01 19:48:40]

On 2015/10/01 19:46:37, alexst wrote:
> On 2015/10/01 19:41:46, achaulk wrote:
> >
>
https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
> > File cc/output/overlay_processor.cc (right):
> > 
> >
>
https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
> > cc/output/overlay_processor.cc:41: primary_surface.display_rect =
> > gfx::RectF(root_render_pass->output_rect);
> > On 2015/10/01 18:59:31, ccameron wrote:
> > > On 2015/09/30 19:04:19, achaulk wrote:
> > > > This needs to be in texture coordinates, eg 0..1
> > > > 
> > > > While ignored by current implementations, it might not always be
> > > 
> > > Are you sure -- display_rect is read in GLRenderer::ScheduleOverlays, and
is
> > > rounded to the nearest integer.
> > > 
> > > I suggest we delete one of display_rect and quad_rect_in_target_space, to
> > > clarify things.
> > 
> > Oh right, this one is pixel coordinates. You do need to set uv_rect to 0,0 &
> 1,1
> > 
> > I'm not actually sure what quad_rect_in_target_space is for
> 
> that one is from here https://codereview.chromium.org/1311013002/

Ah. Still, it's not really relevant to the primary plane, so it can probably be
ignored for this case

[ccameron, 2015-10-01 21:08:48]

https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
File cc/output/overlay_processor.cc (right):

https://codereview.chromium.org/1376883004/diff/40001/cc/output/overlay_proce...
cc/output/overlay_processor.cc:41: primary_surface.display_rect =
gfx::RectF(root_render_pass->output_rect);
On 2015/10/01 19:41:46, achaulk wrote:
> On 2015/10/01 18:59:31, ccameron wrote:
> > On 2015/09/30 19:04:19, achaulk wrote:
> > > This needs to be in texture coordinates, eg 0..1
> > > 
> > > While ignored by current implementations, it might not always be
> > 
> > Are you sure -- display_rect is read in GLRenderer::ScheduleOverlays, and is
> > rounded to the nearest integer.
> > 
> > I suggest we delete one of display_rect and quad_rect_in_target_space, to
> > clarify things.
> 
> Oh right, this one is pixel coordinates. You do need to set uv_rect to 0,0 &
1,1
> 
> I'm not actually sure what quad_rect_in_target_space is for

That's done by the ctor at 
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/overlay_...

[achaulk, 2015-10-02 15:32:07]

lgtm

[ccameron, 2015-10-02 19:06:12]

Thanks!

[ccameron, 2015-10-02 19:06:16]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-10-02 19:06:16]

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

[commit-bot: I haz the power, 2015-10-02 19:06:54]

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

[commit-bot: I haz the power, 2015-10-03 00:07:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-03 00:09:13]

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

[ccameron, 2015-10-05 17:47:19]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1383293002/ by ccameron@chromium.org.

The reason for reverting is: Speculative revert because this might be related to
a browser crash rate uptick in crbug.com/539374..