Use SwapBuffersWithBounds on Chromecast

Use SwapBuffersWithBounds on Chromecast

1) Compute bounds of non-transparent content in Cast overlay
strategy.
2) Pass these bounds through via swap call (SwapBuffersWithBounds)

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

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

[halliwell, 2017-02-13 23:55:08]

Description was changed from

==========
Use SwapBuffersWithBounds on Chromecast

1) Compute bounds of non-transparent content in Cast overlay
strategy.
2) Pass these bounds through via swap call (SwapBuffersWithBounds)

BUG=687690
==========

to

==========
Use SwapBuffersWithBounds on Chromecast

1) Compute bounds of non-transparent content in Cast overlay
strategy.
2) Pass these bounds through via swap call (SwapBuffersWithBounds)

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

[halliwell, 2017-02-13 23:55:42]

Description was changed from

==========
Use SwapBuffersWithBounds on Chromecast

1) Compute bounds of non-transparent content in Cast overlay
strategy.
2) Pass these bounds through via swap call (SwapBuffersWithBounds)

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

to

==========
Use SwapBuffersWithBounds on Chromecast

1) Compute bounds of non-transparent content in Cast overlay
strategy.
2) Pass these bounds through via swap call (SwapBuffersWithBounds)

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

[halliwell, 2017-02-13 23:55:43]

halliwell@chromium.org changed reviewers:
+ danakj@chromium.org

[halliwell, 2017-02-13 23:58:11]

This is a newer attempt at https://codereview.chromium.org/2573603005/ now that
our swap API has been renamed to SwapBuffersWithBounds.

We previously discussed converting black quads (e.g. video letterbox) to
overlays, but ran into a bunch of issues - places that assume limited #overlays
/ frame, not having a valid resource for the overlay.  Seemed like this way is
simpler.

[danakj, 2017-02-14 17:12:55]

On 2017/02/13 23:58:11, halliwell wrote:
> This is a newer attempt at https://codereview.chromium.org/2573603005/ now
that
> our swap API has been renamed to SwapBuffersWithBounds.
> 
> We previously discussed converting black quads (e.g. video letterbox) to
> overlays, but ran into a bunch of issues - places that assume limited
#overlays
> / frame, not having a valid resource for the overlay.  Seemed like this way is
> simpler.

FWIW I believe the overlay code could say it will turn them into overlays (ie it
will handle them) and then just do nothing with them instead, cuz you get black
as a result right?

[danakj, 2017-02-14 17:28:59]

https://codereview.chromium.org/2693023002/diff/1/cc/output/output_surface_fr...
File cc/output/output_surface_frame.h (right):

https://codereview.chromium.org/2693023002/diff/1/cc/output/output_surface_fr...
cc/output/output_surface_frame.h:30: std::vector<gfx::Rect> content_bounds;
Can you make clear in comments the relationship between this and sub_buffer_rect
vs neither being set? Also can you give guidance on what rects are valid, can
they overlap? etc

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_processor.cc
File cc/output/overlay_processor.cc (right):

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_processor...
cc/output/overlay_processor.cc:67: content_bounds->insert(content_bounds->end(),
content_bounds_.begin(),
why append and not replace the contents?

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_processor...
cc/output/overlay_processor.cc:106: gfx::Rect* damage_rect) {
Shall we just add the bounds here so we don't need class state? Then it would be
in directrenderer which can be more globally used as needed, and probably stored
as part of DirectRenderer::DrawingFrame?

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

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_underlay_cast.h:27: void
GetContentBounds(std::vector<gfx::Rect>* bounds) override;
shall we add the bounds to the Attempt method instead? Then we won't need a
member var and class-level state between these two methods.

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_unittest.cc
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_unittest....
cc/output/overlay_unittest.cc:391: typedef
OverlayTest<UnderlayCastOverlayValidator> UnderlayCastTest;
using instead of typedef

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_unittest....
cc/output/overlay_unittest.cc:1243: TEST_F(UnderlayCastTest,
NoOverlayDamageRect) {
These tests are named for damage rects but that seems incorrect

https://codereview.chromium.org/2693023002/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2693023002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:111: if
(swap_rect != gfx::Rect(frame.size))
Why did you choose this ordering?

I would have assumed that if bounds are given we should use that API so it would
come first, but can you explain?

[danakj, 2017-02-14 17:32:36]

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_unittest.cc
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_unittest....
cc/output/overlay_unittest.cc:1243: TEST_F(UnderlayCastTest,
NoOverlayDamageRect) {
On 2017/02/14 17:28:58, danakj wrote:
> These tests are named for damage rects but that seems incorrect

.. but then should they be testing the damage rect and candidate list too, or
that will become untested code now?

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_unittest....
cc/output/overlay_unittest.cc:1278: EXPECT_TRUE(content_bounds[0].IsEmpty());
What is the difference between swapping an empty rect vs an empty vector?

[halliwell, 2017-02-15 22:28:55]

On 2017/02/14 17:12:55, danakj wrote:
> On 2017/02/13 23:58:11, halliwell wrote:
> > This is a newer attempt at https://codereview.chromium.org/2573603005/ now
> that
> > our swap API has been renamed to SwapBuffersWithBounds.
> > 
> > We previously discussed converting black quads (e.g. video letterbox) to
> > overlays, but ran into a bunch of issues - places that assume limited
> #overlays
> > / frame, not having a valid resource for the overlay.  Seemed like this way
is
> > simpler.
> 
> FWIW I believe the overlay code could say it will turn them into overlays (ie
it
> will handle them) and then just do nothing with them instead, cuz you get
black
> as a result right?

Do you mean having the strategy class do the transparent-quad replacement, but
not to bother inserting an item into the OverlayCandidateList?  Around here:
https://cs.chromium.org/chromium/src/cc/output/overlay_strategy_underlay.cc?q...
 - that would work (most of the challenges come from putting something into the
OverlayCandidateList that isn't really an overlay), although I'm not sure what
it would gain - for example, would it allow reusing an existing calculation of
content bounds in the compositor?  (i.e. to get rid of calculating the bounds in
the overlay strategy class).

[halliwell, 2017-02-15 22:29:05]

https://codereview.chromium.org/2693023002/diff/1/cc/output/output_surface_fr...
File cc/output/output_surface_frame.h (right):

https://codereview.chromium.org/2693023002/diff/1/cc/output/output_surface_fr...
cc/output/output_surface_frame.h:30: std::vector<gfx::Rect> content_bounds;
On 2017/02/14 17:28:58, danakj wrote:
> Can you make clear in comments the relationship between this and
sub_buffer_rect
> vs neither being set? Also can you give guidance on what rects are valid, can
> they overlap? etc

Done.

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_processor.cc
File cc/output/overlay_processor.cc (right):

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_processor...
cc/output/overlay_processor.cc:67: content_bounds->insert(content_bounds->end(),
content_bounds_.begin(),
On 2017/02/14 17:28:58, danakj wrote:
> why append and not replace the contents?

This function removed now.

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_processor...
cc/output/overlay_processor.cc:106: gfx::Rect* damage_rect) {
On 2017/02/14 17:28:58, danakj wrote:
> Shall we just add the bounds here so we don't need class state? Then it would
be
> in directrenderer which can be more globally used as needed, and probably
stored
> as part of DirectRenderer::DrawingFrame?

Done.

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

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_strategy_...
cc/output/overlay_strategy_underlay_cast.h:27: void
GetContentBounds(std::vector<gfx::Rect>* bounds) override;
On 2017/02/14 17:28:58, danakj wrote:
> shall we add the bounds to the Attempt method instead? Then we won't need a
> member var and class-level state between these two methods.

Done.

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_unittest.cc
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_unittest....
cc/output/overlay_unittest.cc:391: typedef
OverlayTest<UnderlayCastOverlayValidator> UnderlayCastTest;
On 2017/02/14 17:28:58, danakj wrote:
> using instead of typedef

Done.

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_unittest....
cc/output/overlay_unittest.cc:1243: TEST_F(UnderlayCastTest,
NoOverlayDamageRect) {
On 2017/02/14 17:32:35, danakj wrote:
> On 2017/02/14 17:28:58, danakj wrote:
> > These tests are named for damage rects but that seems incorrect
> 
> .. but then should they be testing the damage rect and candidate list too, or
> that will become untested code now?

They are mis-named (I copied them over from previous CL).  Renamed now to
*ContentBounds.  The point of the tests is to assert on content bounds returned
from Attempt.

https://codereview.chromium.org/2693023002/diff/1/cc/output/overlay_unittest....
cc/output/overlay_unittest.cc:1278: EXPECT_TRUE(content_bounds[0].IsEmpty());
On 2017/02/14 17:32:35, danakj wrote:
> What is the difference between swapping an empty rect vs an empty vector?

I clarified in the comment on OutputFrame.  Empty vector is the default value
for content_bounds, and is equivalent to swapping full screen.  An empty rect
means no content (as in this test, we expect to see this when playing
full-screen video, and it means the driver can completely ignore graphics plane
at scanout time).

https://codereview.chromium.org/2693023002/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2693023002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:111: if
(swap_rect != gfx::Rect(frame.size))
On 2017/02/14 17:28:58, danakj wrote:
> Why did you choose this ordering?
> 
> I would have assumed that if bounds are given we should use that API so it
would
> come first, but can you explain?

For now, it seems they are mutually exclusive options (I updated the comment in
OutputFrame to this effect).  GLRenderer chooses either one or the other.  So I
think the order here isn't super important.

[danakj, 2017-02-16 18:19:50]

On Wed, Feb 15, 2017 at 5:28 PM, <halliwell@chromium.org> wrote:

> On 2017/02/14 17:12:55, danakj wrote:
> > On 2017/02/13 23:58:11, halliwell wrote:
> > > This is a newer attempt at https://codereview.chromium.org/2573603005/
> now
> > that
> > > our swap API has been renamed to SwapBuffersWithBounds.
> > >
> > > We previously discussed converting black quads (e.g. video letterbox)
> to
> > > overlays, but ran into a bunch of issues - places that assume limited
> > #overlays
> > > / frame, not having a valid resource for the overlay. Seemed like this
> way
> is
> > > simpler.
> >
> > FWIW I believe the overlay code could say it will turn them into
> overlays (ie
> it
> > will handle them) and then just do nothing with them instead, cuz you get
> black
> > as a result right?
>
> Do you mean having the strategy class do the transparent-quad replacement,
> but
> not to bother inserting an item into the OverlayCandidateList? Around here:
> https://cs.chromium.org/chromium/src/cc/output/
> overlay_strategy_underlay.cc?q=overlay_strategy_under+
> package:%5Echromium$&l=45
> - that would work (most of the challenges come from putting something into
> the
> OverlayCandidateList that isn't really an overlay), although I'm not sure
> what
> it would gain - for example, would it allow reusing an existing
> calculation of
> content bounds in the compositor? (i.e. to get rid of calculating the
> bounds in
> the overlay strategy class).
>

I'm not sure what would buy us either, but just wanted to throw it out
there as you were running into issues, and I don't want things to block you
that we can just sidestep.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-02-16 18:54:20]

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:421: return
context_provider->ContextCapabilities().post_sub_buffer;
Should this be false if use_swap_with_bounds_? We're entering a bit of a strange
tristate represented by 2 bools which have 2^2=4 states. So there is now an
invalid/unused state represented. What if use_partial_swap_ and
use_swap_with_bounds_ became an enum instead?

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2638: swap_buffer_rect_ = gfx::Rect(surface_size);
Do you intentionally overload not supporting empty swaps with swap_with_bounds_?
See the else case below.

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:353: bool use_swap_with_bounds_;
can be const

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:353: bool use_swap_with_bounds_;
can you group these two use_ with the other use_ above? maybe make them all
const?

https://codereview.chromium.org/2693023002/diff/20001/cc/output/output_surfac...
File cc/output/output_surface_frame.h (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/output_surfac...
cc/output/output_surface_frame.h:32: // |sub_buffer_rect| and |content_bounds|
is not supported.
I think this is a little misleading. sub_buffer_rect is always provided. But it
may be == surface size. It may also be empty even if not using partial swap but
there's no damage (after doing overlays? when the context supports empty swaps)

That said.. comparing rects are == is a little uh.. oldschool? Maybe this is a
good case for an Optional sub_buffer_rect. That'd make intentions more explicit.
If you agree we could do that as a separate CL for this one to build on.

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay_cast.cc (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay_cast.cc:40:
OverlayCandidate::FromDrawQuad(resource_provider, quad, &candidate);
How robust is it that you're redoing a subset of the logic from
OverlayStrategyUnderlay here to decide what will be the underlay, then calling
it to do the real thing? I'm looking at CheckOverlaySupport() and wondering
about the fact you're not using that here. Maybe I'm wrong and this is all okay?

Or, is there a way to call the parent class, have it process things, then
determine the content rects from the result once you have definite answer on
what will be the underlay?

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay_cast.cc:41: found_underlay |= is_underlay;
you can just use = here if you want. you know found_underlay is false.

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay_cast.cc:51:
content_rect.Subtract(ToEnclosedRect(quad_rect));
Do you have a test verifying the use of Enclosed/Enclosing here?

And can you write gfx::ToEnclos*Rect() with the namespace?

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay_cast.cc:60: content_bounds->clear();
I suggest DCHECK its empty instead, if you want. It'd be weird if we're passing
a non-empty list to other strategies which just leave it as is.

https://codereview.chromium.org/2693023002/diff/20001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2693023002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:110: 
this could DCHECK that we don't have both partial swap and content_bounds? maybe
it needs to consider empty swaps tho? (see comments in GLRenderer)

[halliwell, 2017-02-17 14:08:45]

https://codereview.chromium.org/2693023002/diff/20001/cc/output/output_surfac...
File cc/output/output_surface_frame.h (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/output_surfac...
cc/output/output_surface_frame.h:32: // |sub_buffer_rect| and |content_bounds|
is not supported.
On 2017/02/16 18:54:19, danakj wrote:
> I think this is a little misleading. sub_buffer_rect is always provided. But
it
> may be == surface size. It may also be empty even if not using partial swap
but
> there's no damage (after doing overlays? when the context supports empty
swaps)
> 
> That said.. comparing rects are == is a little uh.. oldschool? Maybe this is a
> good case for an Optional sub_buffer_rect. That'd make intentions more
explicit.
> If you agree we could do that as a separate CL for this one to build on.

Yep, looked at this a little closer:
* Optional does sound clearer vs comparing rects.  I'll try putting together a
CL for that first.
* I think this comment is still true to the extent that content_bounds and
sub_buffer_rect can't both be provided.  But yes, there is an additional case -
sub_buffer_rect isn't just for PartialSwap, it can also be empty in the case
where CommitOverlayPlanes is used.  I'll rephrase to clarify in next patchset.

[halliwell, 2017-02-23 18:01:47]

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:421: return
context_provider->ContextCapabilities().post_sub_buffer;
On 2017/02/16 18:54:19, danakj wrote:
> Should this be false if use_swap_with_bounds_? We're entering a bit of a
strange
> tristate represented by 2 bools which have 2^2=4 states. So there is now an
> invalid/unused state represented. What if use_partial_swap_ and
> use_swap_with_bounds_ became an enum instead?

It's true that a 3-state enum represents the state better.  But I'm missing some
context here: use_partial_swap_ is state of DirectRenderer, whereas for now,
swap with bounds is only a concern of GLRenderer.  Making it 3-state requires
pushing that concept up into DirectRenderer ... I'm not clear whether that's a
significant drawback.

For this patchset, I just fixed up CanPartialSwap here to return false if using
swap with bounds.

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2638: swap_buffer_rect_ = gfx::Rect(surface_size);
On 2017/02/16 18:54:19, danakj wrote:
> Do you intentionally overload not supporting empty swaps with
swap_with_bounds_?
> See the else case below.

Correct.  The case of allow_empty_swap_ when use_partial_swap_ is false
corresponds to calling CommitOverlayPlanes instead of Swap.

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:353: bool use_swap_with_bounds_;
On 2017/02/16 18:54:19, danakj wrote:
> can you group these two use_ with the other use_ above? maybe make them all
> const?

I grouped them.  Making them all const is a bit verbose to access context caps
in initializer list.  Could fix that by grouping in a struct and passing
gpu::Capabilities to its constructor but maybe best left to another CL?

https://codereview.chromium.org/2693023002/diff/20001/cc/output/output_surfac...
File cc/output/output_surface_frame.h (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/output_surfac...
cc/output/output_surface_frame.h:32: // |sub_buffer_rect| and |content_bounds|
is not supported.
On 2017/02/17 14:08:45, halliwell wrote:
> On 2017/02/16 18:54:19, danakj wrote:
> > I think this is a little misleading. sub_buffer_rect is always provided. But
> it
> > may be == surface size. It may also be empty even if not using partial swap
> but
> > there's no damage (after doing overlays? when the context supports empty
> swaps)
> > 
> > That said.. comparing rects are == is a little uh.. oldschool? Maybe this is
a
> > good case for an Optional sub_buffer_rect. That'd make intentions more
> explicit.
> > If you agree we could do that as a separate CL for this one to build on.
> 
> Yep, looked at this a little closer:
> * Optional does sound clearer vs comparing rects.  I'll try putting together a
> CL for that first.
> * I think this comment is still true to the extent that content_bounds and
> sub_buffer_rect can't both be provided.  But yes, there is an additional case
-
> sub_buffer_rect isn't just for PartialSwap, it can also be empty in the case
> where CommitOverlayPlanes is used.  I'll rephrase to clarify in next patchset.

Ok, comment updated, and using Optional now.

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay_cast.cc (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay_cast.cc:40:
OverlayCandidate::FromDrawQuad(resource_provider, quad, &candidate);
On 2017/02/16 18:54:19, danakj wrote:
> How robust is it that you're redoing a subset of the logic from
> OverlayStrategyUnderlay here to decide what will be the underlay, then calling
> it to do the real thing? I'm looking at CheckOverlaySupport() and wondering
> about the fact you're not using that here. Maybe I'm wrong and this is all
okay?
> 
Strictly speaking, it is ok (we always return true for CheckOverlaySupport
here).  I acknowledge that does involve some assumptions (i.e. that this is used
together with OverlayCandidatesCast), which is less than ideal.

> Or, is there a way to call the parent class, have it process things, then
> determine the content rects from the result once you have definite answer on
> what will be the underlay?

It's a bit awkward, because we need to treat solid-black quads here differently
depending on whether they occlude the overlay or not.  If we run the parent
logic first, the overlay quad gets replaced with a transparent one, and it's no
longer possible to tell that it was the overlay quad in order to recover this
ordering, I think.

One option to clean it up would be to give up on code reuse with parent class
and just put all the 'Attempt' logic directly here.  That wouldn't require any
assumptions.  WDYT?

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay_cast.cc:41: found_underlay |= is_underlay;
On 2017/02/16 18:54:19, danakj wrote:
> you can just use = here if you want. you know found_underlay is false.

Done.

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay_cast.cc:51:
content_rect.Subtract(ToEnclosedRect(quad_rect));
On 2017/02/16 18:54:19, danakj wrote:
> Do you have a test verifying the use of Enclosed/Enclosing here?
> 
Test added.

> And can you write gfx::ToEnclos*Rect() with the namespace?
Done.

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay_cast.cc:60: content_bounds->clear();
On 2017/02/16 18:54:19, danakj wrote:
> I suggest DCHECK its empty instead, if you want. It'd be weird if we're
passing
> a non-empty list to other strategies which just leave it as is.

Done.

https://codereview.chromium.org/2693023002/diff/20001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2693023002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:110: 
On 2017/02/16 18:54:19, danakj wrote:
> this could DCHECK that we don't have both partial swap and content_bounds?
maybe
> it needs to consider empty swaps tho? (see comments in GLRenderer)

Done (btw, empty swap can only happen here through PartialSwap.  The special
case for empty swap in GLRenderer corresponds to calling CommitOverlayPlanes,
which is used here:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/compositor...
 )

[danakj, 2017-02-24 22:05:18]

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:421: return
context_provider->ContextCapabilities().post_sub_buffer;
On 2017/02/23 18:01:46, halliwell wrote:
> On 2017/02/16 18:54:19, danakj wrote:
> > Should this be false if use_swap_with_bounds_? We're entering a bit of a
> strange
> > tristate represented by 2 bools which have 2^2=4 states. So there is now an
> > invalid/unused state represented. What if use_partial_swap_ and
> > use_swap_with_bounds_ became an enum instead?
> 
> It's true that a 3-state enum represents the state better.  But I'm missing
some
> context here: use_partial_swap_ is state of DirectRenderer, whereas for now,
> swap with bounds is only a concern of GLRenderer.  Making it 3-state requires
> pushing that concept up into DirectRenderer ... I'm not clear whether that's a
> significant drawback.
> 
> For this patchset, I just fixed up CanPartialSwap here to return false if
using
> swap with bounds.

Ya, I think with the optional changes u did this is a lot more clear and i'm
happy where it landed

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:353: bool use_swap_with_bounds_;
On 2017/02/23 18:01:46, halliwell wrote:
> On 2017/02/16 18:54:19, danakj wrote:
> > can you group these two use_ with the other use_ above? maybe make them all
> > const?
> 
> I grouped them.  Making them all const is a bit verbose to access context caps
> in initializer list.  Could fix that by grouping in a struct and passing
> gpu::Capabilities to its constructor but maybe best left to another CL?

Ah, right, good points, what u did seems good

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay_cast.cc (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay_cast.cc:40:
OverlayCandidate::FromDrawQuad(resource_provider, quad, &candidate);
On 2017/02/23 18:01:46, halliwell wrote:
> On 2017/02/16 18:54:19, danakj wrote:
> > How robust is it that you're redoing a subset of the logic from
> > OverlayStrategyUnderlay here to decide what will be the underlay, then
calling
> > it to do the real thing? I'm looking at CheckOverlaySupport() and wondering
> > about the fact you're not using that here. Maybe I'm wrong and this is all
> okay?
> > 
> Strictly speaking, it is ok (we always return true for CheckOverlaySupport
> here).  I acknowledge that does involve some assumptions (i.e. that this is
used
> together with OverlayCandidatesCast), which is less than ideal.
> 
> > Or, is there a way to call the parent class, have it process things, then
> > determine the content rects from the result once you have definite answer on
> > what will be the underlay?
> 
> It's a bit awkward, because we need to treat solid-black quads here
differently
> depending on whether they occlude the overlay or not.  If we run the parent
> logic first, the overlay quad gets replaced with a transparent one, and it's
no
> longer possible to tell that it was the overlay quad in order to recover this
> ordering, I think.
> 
> One option to clean it up would be to give up on code reuse with parent class
> and just put all the 'Attempt' logic directly here.  That wouldn't require any
> assumptions.  WDYT?

If you reproduce the tests for it then that seems decent to me. Otherwise, some
tests and/or dchecks that verify assumptions and would go off if they were ever
violated would be good if possible. I don't have any specifically amazing
suggestions right now so LGTM and if you like any of those ideas or have another
then go for it! :)

https://codereview.chromium.org/2693023002/diff/80001/cc/output/overlay_unitt...
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/2693023002/diff/80001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:112: expected_rect_ = gfx::RectF(rect);
can u drop this overload and do the gfx::RectF promotion at the callsites?

https://codereview.chromium.org/2693023002/diff/80001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:342: color_quad->SetNew(shared_quad_state, rect,
rect, color, false);
DCHECK the color has 255 alpha

https://codereview.chromium.org/2693023002/diff/80001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:390: output_surface_.reset(new
OutputSurfaceType(provider_));
nit: = MakeUnique<> is preferred to reset(new)

https://codereview.chromium.org/2693023002/diff/80001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:1477: EXPECT_EQ(gfx::Rect(0, 0, 11, 11),
content_bounds_[0]);
fwiw u can skip the position and just pass the size if its at 0,0

https://codereview.chromium.org/2693023002/diff/80001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:1483: const gfx::Rect overlay_rect(0, 0, 255,
255);
can you relate this to the kOverlayRect in the test a bit more. like maybe
overlay_rect = kOverlayRect then inset it, or grow this and expect_eq to that
below

[halliwell, 2017-02-27 22:57:14]

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay_cast.cc (right):

https://codereview.chromium.org/2693023002/diff/20001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay_cast.cc:40:
OverlayCandidate::FromDrawQuad(resource_provider, quad, &candidate);
On 2017/02/24 22:05:18, danakj_OOO wrote:
> On 2017/02/23 18:01:46, halliwell wrote:
> > On 2017/02/16 18:54:19, danakj wrote:
> > > How robust is it that you're redoing a subset of the logic from
> > > OverlayStrategyUnderlay here to decide what will be the underlay, then
> calling
> > > it to do the real thing? I'm looking at CheckOverlaySupport() and
wondering
> > > about the fact you're not using that here. Maybe I'm wrong and this is all
> > okay?
> > > 
> > Strictly speaking, it is ok (we always return true for CheckOverlaySupport
> > here).  I acknowledge that does involve some assumptions (i.e. that this is
> used
> > together with OverlayCandidatesCast), which is less than ideal.
> > 
> > > Or, is there a way to call the parent class, have it process things, then
> > > determine the content rects from the result once you have definite answer
on
> > > what will be the underlay?
> > 
> > It's a bit awkward, because we need to treat solid-black quads here
> differently
> > depending on whether they occlude the overlay or not.  If we run the parent
> > logic first, the overlay quad gets replaced with a transparent one, and it's
> no
> > longer possible to tell that it was the overlay quad in order to recover
this
> > ordering, I think.
> > 
> > One option to clean it up would be to give up on code reuse with parent
class
> > and just put all the 'Attempt' logic directly here.  That wouldn't require
any
> > assumptions.  WDYT?
> 
> If you reproduce the tests for it then that seems decent to me. Otherwise,
some
> tests and/or dchecks that verify assumptions and would go off if they were
ever
> violated would be good if possible. I don't have any specifically amazing
> suggestions right now so LGTM and if you like any of those ideas or have
another
> then go for it! :)

Yep, good point.  I added a basic DCHECK that the parent class Attempt call
agrees with the logic here on whether an underlay is found or not.

https://codereview.chromium.org/2693023002/diff/80001/cc/output/overlay_unitt...
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/2693023002/diff/80001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:112: expected_rect_ = gfx::RectF(rect);
On 2017/02/24 22:05:18, danakj_OOO wrote:
> can u drop this overload and do the gfx::RectF promotion at the callsites?

Done.

https://codereview.chromium.org/2693023002/diff/80001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:342: color_quad->SetNew(shared_quad_state, rect,
rect, color, false);
On 2017/02/24 22:05:18, danakj_OOO wrote:
> DCHECK the color has 255 alpha

Done.

https://codereview.chromium.org/2693023002/diff/80001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:390: output_surface_.reset(new
OutputSurfaceType(provider_));
On 2017/02/24 22:05:18, danakj_OOO wrote:
> nit: = MakeUnique<> is preferred to reset(new)

Done.

https://codereview.chromium.org/2693023002/diff/80001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:1483: const gfx::Rect overlay_rect(0, 0, 255,
255);
On 2017/02/24 22:05:18, danakj_OOO wrote:
> can you relate this to the kOverlayRect in the test a bit more. like maybe
> overlay_rect = kOverlayRect then inset it, or grow this and expect_eq to that
> below

Done.

[halliwell, 2017-02-27 22:59:18]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2017-02-27 22:59:19]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2693023002/#ps100001
(title: "danakj nits")

[commit-bot: I haz the power, 2017-02-27 23:00:12]

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-02-28 00:53:10]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488236358995890,
"parent_rev": "105146afd30625c6d68e62fb456318c1ec87ef69", "commit_rev":
"dc04cafce9b167e74e2cd98761cd0f075e187671"}

[commit-bot: I haz the power, 2017-02-28 00:53:54]

Description was changed from

==========
Use SwapBuffersWithBounds on Chromecast

1) Compute bounds of non-transparent content in Cast overlay
strategy.
2) Pass these bounds through via swap call (SwapBuffersWithBounds)

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

to

==========
Use SwapBuffersWithBounds on Chromecast

1) Compute bounds of non-transparent content in Cast overlay
strategy.
2) Pass these bounds through via swap call (SwapBuffersWithBounds)

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

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

[commit-bot: I haz the power, 2017-02-28 00:53:55]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/dc04cafce9b167e74e2cd98761cd...