Add underlays and split off common overlay functionality

Add underlays and split off common overlay functionality

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

[achaulk, 2015-03-04 20:53:34]

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

[achaulk, 2015-03-04 20:53:35]

This should work for underlays

[alexst (slow to review), 2015-03-05 14:12:01]

https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_s...
File cc/output/overlay_strategy_single_on_top.h (right):

https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_s...
cc/output/overlay_strategy_single_on_top.h:31: OverlayCandidateValidator*
capability_checker_;
Maybe move this into the base class since both subclasses use it?

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

https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
cc/output/overlay_strategy_underlay.cc:24: case DrawQuad::STREAM_VIDEO_CONTENT:
We can allow WebGL here as well, I think.

https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
cc/output/overlay_strategy_underlay.cc:44: for (auto it = quad_list.begin(); it
!= quad_list.end(); ++it) {
(auto it : quad_list) instead for simplicity?

https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
cc/output/overlay_strategy_underlay.cc:62: candidate.plane_z_order = 1;
This should be -1 for the underlay.

https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
cc/output/overlay_strategy_underlay.cc:74: opaque =
candidate_iterator->opaque_rect,
If we are punching a hole, shouldn't opaque parts become transparent so we can
see through to the underlay? Or is there something in the quad constructor that
does that?

[halliwell, 2015-03-05 18:51:34]

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

[halliwell, 2015-03-05 18:51:35]

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

https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
cc/output/overlay_strategy_underlay.cc:79: SK_ColorTRANSPARENT, true);
I tried patching this change into my build this morning ... and what's here
doesn't actually seem to punch a transparent hole through to let the underlay
show.  If I change this to SK_ColorYELLOW, say, I can see the quad is getting
drawn in the right place, but with the code as it stands, the video area is just
black.

[achaulk, 2015-03-05 18:53:04]

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

https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
cc/output/overlay_strategy_underlay.cc:79: SK_ColorTRANSPARENT, true);
On 2015/03/05 18:51:35, halliwell wrote:
> I tried patching this change into my build this morning ... and what's here
> doesn't actually seem to punch a transparent hole through to let the underlay
> show.  If I change this to SK_ColorYELLOW, say, I can see the quad is getting
> drawn in the right place, but with the code as it stands, the video area is
just
> black.

Hmm, I just took the existing VIDEO_HOLE setup for the quad. Do you need
something more?

[halliwell, 2015-03-05 18:54:30]

On 2015/03/05 18:53:04, achaulk wrote:
>
https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
> File cc/output/overlay_strategy_underlay.cc (right):
> 
>
https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
> cc/output/overlay_strategy_underlay.cc:79: SK_ColorTRANSPARENT, true);
> On 2015/03/05 18:51:35, halliwell wrote:
> > I tried patching this change into my build this morning ... and what's here
> > doesn't actually seem to punch a transparent hole through to let the
underlay
> > show.  If I change this to SK_ColorYELLOW, say, I can see the quad is
getting
> > drawn in the right place, but with the code as it stands, the video area is
> just
> > black.
> 
> Hmm, I just took the existing VIDEO_HOLE setup for the quad. Do you need
> something more?

Yeah I don't understand why it's not working :)  We currently use VIDEO_HOLE and
that works.
I'm digging into it just now ...

[alexst (slow to review), 2015-03-05 19:01:11]

On 2015/03/05 18:54:30, halliwell wrote:
> On 2015/03/05 18:53:04, achaulk wrote:
> >
>
https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
> > File cc/output/overlay_strategy_underlay.cc (right):
> > 
> >
>
https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
> > cc/output/overlay_strategy_underlay.cc:79: SK_ColorTRANSPARENT, true);
> > On 2015/03/05 18:51:35, halliwell wrote:
> > > I tried patching this change into my build this morning ... and what's
here
> > > doesn't actually seem to punch a transparent hole through to let the
> underlay
> > > show.  If I change this to SK_ColorYELLOW, say, I can see the quad is
> getting
> > > drawn in the right place, but with the code as it stands, the video area
is
> > just
> > > black.
> > 
> > Hmm, I just took the existing VIDEO_HOLE setup for the quad. Do you need
> > something more?
> 
> Yeah I don't understand why it's not working :)  We currently use VIDEO_HOLE
and
> that works.
> I'm digging into it just now ...

So, I recall there being a place where transparent background is set in CC, I
wonder if alpha channel in the framebuffer is 1.0, which is why nothing is
showing through.

[alexst (slow to review), 2015-03-05 19:30:38]

On 2015/03/05 19:01:11, alexst wrote:
> On 2015/03/05 18:54:30, halliwell wrote:
> > On 2015/03/05 18:53:04, achaulk wrote:
> > >
> >
>
https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
> > > File cc/output/overlay_strategy_underlay.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
> > > cc/output/overlay_strategy_underlay.cc:79: SK_ColorTRANSPARENT, true);
> > > On 2015/03/05 18:51:35, halliwell wrote:
> > > > I tried patching this change into my build this morning ... and what's
> here
> > > > doesn't actually seem to punch a transparent hole through to let the
> > underlay
> > > > show.  If I change this to SK_ColorYELLOW, say, I can see the quad is
> > getting
> > > > drawn in the right place, but with the code as it stands, the video area
> is
> > > just
> > > > black.
> > > 
> > > Hmm, I just took the existing VIDEO_HOLE setup for the quad. Do you need
> > > something more?
> > 
> > Yeah I don't understand why it's not working :)  We currently use VIDEO_HOLE
> and
> > that works.
> > I'm digging into it just now ...
> 
> So, I recall there being a place where transparent background is set in CC, I
> wonder if alpha channel in the framebuffer is 1.0, which is why nothing is
> showing through.

This is one of the places where we set global opacity for a renderer
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

the other one is probably in the browser compositor. I wonder if there are
additional quads below the video that are setting the setting alpha.

[achaulk, 2015-03-05 19:32:23]

On 2015/03/05 19:30:38, alexst wrote:
> On 2015/03/05 19:01:11, alexst wrote:
> > On 2015/03/05 18:54:30, halliwell wrote:
> > > On 2015/03/05 18:53:04, achaulk wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
> > > > File cc/output/overlay_strategy_underlay.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
> > > > cc/output/overlay_strategy_underlay.cc:79: SK_ColorTRANSPARENT, true);
> > > > On 2015/03/05 18:51:35, halliwell wrote:
> > > > > I tried patching this change into my build this morning ... and what's
> > here
> > > > > doesn't actually seem to punch a transparent hole through to let the
> > > underlay
> > > > > show.  If I change this to SK_ColorYELLOW, say, I can see the quad is
> > > getting
> > > > > drawn in the right place, but with the code as it stands, the video
area
> > is
> > > > just
> > > > > black.
> > > > 
> > > > Hmm, I just took the existing VIDEO_HOLE setup for the quad. Do you need
> > > > something more?
> > > 
> > > Yeah I don't understand why it's not working :)  We currently use
VIDEO_HOLE
> > and
> > > that works.
> > > I'm digging into it just now ...
> > 
> > So, I recall there being a place where transparent background is set in CC,
I
> > wonder if alpha channel in the framebuffer is 1.0, which is why nothing is
> > showing through.
> 
> This is one of the places where we set global opacity for a renderer
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> 
> the other one is probably in the browser compositor. I wonder if there are
> additional quads below the video that are setting the setting alpha.

What the replacement quad should be doing is force painting black alpha=0 over
whatever is there without blending, so the background transparency shouldn't
matter

[danakj, 2015-03-05 19:39:33]

On Thu, Mar 5, 2015 at 11:32 AM, <achaulk@chromium.org> wrote:

> On 2015/03/05 19:30:38, alexst wrote:
>
>> On 2015/03/05 19:01:11, alexst wrote:
>> > On 2015/03/05 18:54:30, halliwell wrote:
>> > > On 2015/03/05 18:53:04, achaulk wrote:
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/979693005/diff/1/cc/
> output/overlay_strategy_underlay.cc
>
>> > > > File cc/output/overlay_strategy_underlay.cc (right):
>> > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/979693005/diff/1/cc/
> output/overlay_strategy_underlay.cc#newcode79
>
>> > > > cc/output/overlay_strategy_underlay.cc:79: SK_ColorTRANSPARENT,
>> true);
>> > > > On 2015/03/05 18:51:35, halliwell wrote:
>> > > > > I tried patching this change into my build this morning ... and
>> what's
>> > here
>> > > > > doesn't actually seem to punch a transparent hole through to let
>> the
>> > > underlay
>> > > > > show.  If I change this to SK_ColorYELLOW, say, I can see the
>> quad is
>> > > getting
>> > > > > drawn in the right place, but with the code as it stands, the
>> video
>>
> area
>
>> > is
>> > > > just
>> > > > > black.
>> > > >
>> > > > Hmm, I just took the existing VIDEO_HOLE setup for the quad. Do you
>> need
>> > > > something more?
>> > >
>> > > Yeah I don't understand why it's not working :)  We currently use
>>
> VIDEO_HOLE
>
>> > and
>> > > that works.
>> > > I'm digging into it just now ...
>> >
>> > So, I recall there being a place where transparent background is set in
>> CC,
>>
> I
>
>> > wonder if alpha channel in the framebuffer is 1.0, which is why nothing
>> is
>> > showing through.
>>
>
>  This is one of the places where we set global opacity for a renderer
>>
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/content/renderer/render_view_impl.cc&l=3207
>
>  the other one is probably in the browser compositor. I wonder if there are
>> additional quads below the video that are setting the setting alpha.
>>
>
> What the replacement quad should be doing is force painting black alpha=0
> over
> whatever is there without blending, so the background transparency
> shouldn't
> matter
>

That's what this |false| does:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_la...


>
> https://codereview.chromium.org/979693005/
>

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

[halliwell, 2015-03-05 23:09:25]

On 2015/03/05 19:39:33, danakj wrote:
> On Thu, Mar 5, 2015 at 11:32 AM, <mailto:achaulk@chromium.org> wrote:
> 
> > On 2015/03/05 19:30:38, alexst wrote:
> >
> >> On 2015/03/05 19:01:11, alexst wrote:
> >> > On 2015/03/05 18:54:30, halliwell wrote:
> >> > > On 2015/03/05 18:53:04, achaulk wrote:
> >> > > >
> >> > >
> >> >
> >>
> >
> > https://codereview.chromium.org/979693005/diff/1/cc/
> > output/overlay_strategy_underlay.cc
> >
> >> > > > File cc/output/overlay_strategy_underlay.cc (right):
> >> > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> > https://codereview.chromium.org/979693005/diff/1/cc/
> > output/overlay_strategy_underlay.cc#newcode79
> >
> >> > > > cc/output/overlay_strategy_underlay.cc:79: SK_ColorTRANSPARENT,
> >> true);
> >> > > > On 2015/03/05 18:51:35, halliwell wrote:
> >> > > > > I tried patching this change into my build this morning ... and
> >> what's
> >> > here
> >> > > > > doesn't actually seem to punch a transparent hole through to let
> >> the
> >> > > underlay
> >> > > > > show.  If I change this to SK_ColorYELLOW, say, I can see the
> >> quad is
> >> > > getting
> >> > > > > drawn in the right place, but with the code as it stands, the
> >> video
> >>
> > area
> >
> >> > is
> >> > > > just
> >> > > > > black.
> >> > > >
> >> > > > Hmm, I just took the existing VIDEO_HOLE setup for the quad. Do you
> >> need
> >> > > > something more?
> >> > >
> >> > > Yeah I don't understand why it's not working :)  We currently use
> >>
> > VIDEO_HOLE
> >
> >> > and
> >> > > that works.
> >> > > I'm digging into it just now ...
> >> >
> >> > So, I recall there being a place where transparent background is set in
> >> CC,
> >>
> > I
> >
> >> > wonder if alpha channel in the framebuffer is 1.0, which is why nothing
> >> is
> >> > showing through.
> >>
> >
> >  This is one of the places where we set global opacity for a renderer
> >>
> >
> > https://code.google.com/p/chromium/codesearch#chromium/
> > src/content/renderer/render_view_impl.cc&l=3207
> >
> >  the other one is probably in the browser compositor. I wonder if there are
> >> additional quads below the video that are setting the setting alpha.
> >>
> >
> > What the replacement quad should be doing is force painting black alpha=0
> > over
> > whatever is there without blending, so the background transparency
> > shouldn't
> > matter
> >
> 
> That's what this |false| does:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_la...
> 
> 
> >
> > https://codereview.chromium.org/979693005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ok, it's because the video quad has an empty 'opaque rect' (think this is
because the VideoLayer has contents_opaque == false?), which makes
DrawQuad::ShouldDrawWithBlending return true, and so
GLRenderer::DrawSolidColorQuad is an early-out.

Maybe time to discuss this offline, I'm not sure how directly it's connected to
this code review any more? :)

[halliwell, 2015-03-06 18:47:44]

On 2015/03/05 23:09:25, halliwell wrote:
> On 2015/03/05 19:39:33, danakj wrote:
> > On Thu, Mar 5, 2015 at 11:32 AM, <mailto:achaulk@chromium.org> wrote:
> > 
> > > On 2015/03/05 19:30:38, alexst wrote:
> > >
> > >> On 2015/03/05 19:01:11, alexst wrote:
> > >> > On 2015/03/05 18:54:30, halliwell wrote:
> > >> > > On 2015/03/05 18:53:04, achaulk wrote:
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > > https://codereview.chromium.org/979693005/diff/1/cc/
> > > output/overlay_strategy_underlay.cc
> > >
> > >> > > > File cc/output/overlay_strategy_underlay.cc (right):
> > >> > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > > https://codereview.chromium.org/979693005/diff/1/cc/
> > > output/overlay_strategy_underlay.cc#newcode79
> > >
> > >> > > > cc/output/overlay_strategy_underlay.cc:79: SK_ColorTRANSPARENT,
> > >> true);
> > >> > > > On 2015/03/05 18:51:35, halliwell wrote:
> > >> > > > > I tried patching this change into my build this morning ... and
> > >> what's
> > >> > here
> > >> > > > > doesn't actually seem to punch a transparent hole through to let
> > >> the
> > >> > > underlay
> > >> > > > > show.  If I change this to SK_ColorYELLOW, say, I can see the
> > >> quad is
> > >> > > getting
> > >> > > > > drawn in the right place, but with the code as it stands, the
> > >> video
> > >>
> > > area
> > >
> > >> > is
> > >> > > > just
> > >> > > > > black.
> > >> > > >
> > >> > > > Hmm, I just took the existing VIDEO_HOLE setup for the quad. Do you
> > >> need
> > >> > > > something more?
> > >> > >
> > >> > > Yeah I don't understand why it's not working :)  We currently use
> > >>
> > > VIDEO_HOLE
> > >
> > >> > and
> > >> > > that works.
> > >> > > I'm digging into it just now ...
> > >> >
> > >> > So, I recall there being a place where transparent background is set in
> > >> CC,
> > >>
> > > I
> > >
> > >> > wonder if alpha channel in the framebuffer is 1.0, which is why nothing
> > >> is
> > >> > showing through.
> > >>
> > >
> > >  This is one of the places where we set global opacity for a renderer
> > >>
> > >
> > > https://code.google.com/p/chromium/codesearch#chromium/
> > > src/content/renderer/render_view_impl.cc&l=3207
> > >
> > >  the other one is probably in the browser compositor. I wonder if there
are
> > >> additional quads below the video that are setting the setting alpha.
> > >>
> > >
> > > What the replacement quad should be doing is force painting black alpha=0
> > > over
> > > whatever is there without blending, so the background transparency
> > > shouldn't
> > > matter
> > >
> > 
> > That's what this |false| does:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_la...
> > 
> > 
> > >
> > > https://codereview.chromium.org/979693005/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Ok, it's because the video quad has an empty 'opaque rect' (think this is
> because the VideoLayer has contents_opaque == false?), which makes
> DrawQuad::ShouldDrawWithBlending return true, and so
> GLRenderer::DrawSolidColorQuad is an early-out.
> 
> Maybe time to discuss this offline, I'm not sure how directly it's connected
to
> this code review any more? :)

If you look at video_compositor.cc, IsOpaque returns false for my video frame
(VideoFrame::NATIVE_TEXTURE).  This leads to the VideoLayerImpl being non-opaque
(via WebMediaPlayerImpl), and as I mentioned in my last comment in the review,
the video quad gets an empty opaque rect, and ends up not being drawn at all. 
For my local testing just now, I've changed overlay_strategy_underlay.cc to set
opaque_rect == visible_rect when constructing the transparent quad ... with
this, everything appears to work.

[achaulk, 2015-03-18 16:31:27]

dana, PTAL

All of the rejection cases are shared with the singleoverlayontop, since they
now come from the common base class, so I didn't dup any of those tests

https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_s...
File cc/output/overlay_strategy_single_on_top.h (right):

https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_s...
cc/output/overlay_strategy_single_on_top.h:31: OverlayCandidateValidator*
capability_checker_;
On 2015/03/05 14:12:00, alexst wrote:
> Maybe move this into the base class since both subclasses use it?

I didn't because the base class didn't actually use it, but sure

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

https://codereview.chromium.org/979693005/diff/1/cc/output/overlay_strategy_u...
cc/output/overlay_strategy_underlay.cc:24: case DrawQuad::STREAM_VIDEO_CONTENT:
On 2015/03/05 14:12:00, alexst wrote:
> We can allow WebGL here as well, I think.

Done.

[danakj, 2015-03-18 16:36:52]

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
File cc/output/overlay_strategy_common.cc (right):

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
cc/output/overlay_strategy_common.cc:50: // Ignore transparent solid color
quads.
I missed this when this moved to a helper function, but this function doesn't
"ignore" anything, can you fix this comment?

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
File cc/output/overlay_strategy_common.h (left):

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
cc/output/overlay_strategy_common.h:43:
DISALLOW_COPY_AND_ASSIGN(OverlayStrategySingleOnTop);
keep this in the base class please

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
File cc/output/overlay_strategy_common.h (right):

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
cc/output/overlay_strategy_common.h:32: // Returns true if |draw_qiad| is of a
known quad type and contains an
"draw_qiad"

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay.h (right):

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay.h:21: // it. The video is "underlaid"
through the same mechanism as the video hole -
The video hole mechanism was explicitly kept behind a VIDEO_HOLE flag and not
meant to ship in chrome itself. Why is this different?

[achaulk, 2015-03-18 16:51:34]

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
File cc/output/overlay_strategy_common.cc (right):

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
cc/output/overlay_strategy_common.cc:50: // Ignore transparent solid color
quads.
On 2015/03/18 16:36:51, danakj wrote:
> I missed this when this moved to a helper function, but this function doesn't
> "ignore" anything, can you fix this comment?

Done.

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
File cc/output/overlay_strategy_common.h (left):

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
cc/output/overlay_strategy_common.h:43:
DISALLOW_COPY_AND_ASSIGN(OverlayStrategySingleOnTop);
On 2015/03/18 16:36:51, danakj wrote:
> keep this in the base class please

Done.

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
File cc/output/overlay_strategy_common.h (right):

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
cc/output/overlay_strategy_common.h:32: // Returns true if |draw_qiad| is of a
known quad type and contains an
On 2015/03/18 16:36:51, danakj wrote:
> "draw_qiad"

Acknowledged.

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay.h (right):

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay.h:21: // it. The video is "underlaid"
through the same mechanism as the video hole -
On 2015/03/18 16:36:51, danakj wrote:
> The video hole mechanism was explicitly kept behind a VIDEO_HOLE flag and not
> meant to ship in chrome itself. Why is this different?

The plan AFAIK is to do as much of what the VIDEO_HOLE feature needed to do
inside here as we can instead, so platforms that need it can add this strategy
to their list and platforms that don't can ignore it

[alexst (slow to review), 2015-03-18 17:47:30]

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay.h (right):

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay.h:21: // it. The video is "underlaid"
through the same mechanism as the video hole -
On 2015/03/18 16:36:51, danakj wrote:
> The video hole mechanism was explicitly kept behind a VIDEO_HOLE flag and not
> meant to ship in chrome itself. Why is this different?

I would prefer we strip out video hole references from here because they bring
out bad associations.

This is simply a way to fit a current scene to available hardware. In a more
general case, we have a stack of M quads and N available overlay layers to put
them into. We can split up those M quads in some fashion across N layers and
then composite those N layers in hardware. When one of the quads goes to a layer
below, we need a transparency in the layer above so the final result looks the
same. When the top most layer is the main framebuffer, we need to explicitly add
the black quad, but you can imagine a case with 3 layers stacked as
|framebuffer|video|controls| and controls would be placed into a new small
buffer which wouldn't require any black quads added, just its color cleared.

[achaulk, 2015-03-18 19:40:39]

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay.h (right):

https://codereview.chromium.org/979693005/diff/140001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay.h:21: // it. The video is "underlaid"
through the same mechanism as the video hole -
On 2015/03/18 17:47:29, alexst wrote:
> On 2015/03/18 16:36:51, danakj wrote:
> > The video hole mechanism was explicitly kept behind a VIDEO_HOLE flag and
not
> > meant to ship in chrome itself. Why is this different?
> 
> I would prefer we strip out video hole references from here because they bring
> out bad associations.
> 
> This is simply a way to fit a current scene to available hardware. In a more
> general case, we have a stack of M quads and N available overlay layers to put
> them into. We can split up those M quads in some fashion across N layers and
> then composite those N layers in hardware. When one of the quads goes to a
layer
> below, we need a transparency in the layer above so the final result looks the
> same. When the top most layer is the main framebuffer, we need to explicitly
add
> the black quad, but you can imagine a case with 3 layers stacked as
> |framebuffer|video|controls| and controls would be placed into a new small
> buffer which wouldn't require any black quads added, just its color cleared.

Sure, I can remove the video hole mention

[danakj, 2015-03-18 21:30:14]

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay.cc (right):

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay.cc:59: SolidColorDrawQuad* replacement = new
(candidate_quad) SolidColorDrawQuad();
I think we should do this by adding a replace type method of the QuadList
allocator. This is pretty unclear why it would always work, and doesn't feel
reliable for all time.

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay.h (right):

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay.h:20: // The underlay strategy looks for a
video quad without regard to quads above
Can you mention this is only valid for a quad that is drawn opaquely so that the
hole punching does not modify the visual result?

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

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:786:
EXPECT_EQ(pass_list[0]->quad_list.back()->material, DrawQuad::SOLID_COLOR);
What if you just removed that quad, you'd still pass right? Can you verify the
color of it too to differentiate? And maybe the # of quads to show you replaced
not remove/added?

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:794:
CreateFullscreenCheckeredQuad(resource_provider_.get(),
Can you use a non-opaque quad on top?

[alexst (slow to review), 2015-03-18 21:37:58]

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay.h (right):

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay.h:12: #include
"cc/output/overlay_processor.h"
Do you need this include in the header?

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay.h:22: // for the video quad. Video can then
be blended in by the hardware on top of
..blended in by the hardware below the scene?

Since it's an underlay

[achaulk, 2015-03-23 19:16:03]

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay.cc (right):

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay.cc:59: SolidColorDrawQuad* replacement = new
(candidate_quad) SolidColorDrawQuad();
On 2015/03/18 21:30:14, danakj wrote:
> I think we should do this by adding a replace type method of the QuadList
> allocator. This is pretty unclear why it would always work, and doesn't feel
> reliable for all time.

Yeah, that's probably a good idea. It does depend on the quad list
implementation

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
File cc/output/overlay_strategy_underlay.h (right):

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay.h:12: #include
"cc/output/overlay_processor.h"
On 2015/03/18 21:37:58, alexst wrote:
> Do you need this include in the header?

Nope, removed

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay.h:20: // The underlay strategy looks for a
video quad without regard to quads above
On 2015/03/18 21:30:14, danakj wrote:
> Can you mention this is only valid for a quad that is drawn opaquely so that
the
> hole punching does not modify the visual result?

Done.

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_strat...
cc/output/overlay_strategy_underlay.h:22: // for the video quad. Video can then
be blended in by the hardware on top of
On 2015/03/18 21:37:58, alexst wrote:
> ..blended in by the hardware below the scene?
> 
> Since it's an underlay
> 

Sure

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

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:786:
EXPECT_EQ(pass_list[0]->quad_list.back()->material, DrawQuad::SOLID_COLOR);
On 2015/03/18 21:30:14, danakj wrote:
> What if you just removed that quad, you'd still pass right? Can you verify the
> color of it too to differentiate? And maybe the # of quads to show you
replaced
> not remove/added?

Oh right, didn't notice the other quad was also solid color

https://codereview.chromium.org/979693005/diff/160001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:794:
CreateFullscreenCheckeredQuad(resource_provider_.get(),
On 2015/03/18 21:30:14, danakj wrote:
> Can you use a non-opaque quad on top?

Done.

[alexst (slow to review), 2015-03-23 20:45:51]

lgtm

[danakj, 2015-03-24 17:48:10]

(In the future, please try to upload a rebase as a separate patch set. Looking
at just your changes now includes a bunch of random rebase stuff.)

[achaulk, 2015-03-24 17:48:55]

Yeah sorry about that. I thought I had uploaded it before I rebased that branch

[danakj, 2015-03-24 17:55:15]

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

https://codereview.chromium.org/979693005/diff/200001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:785:
ASSERT_EQ(pass_list[0]->quad_list.back()->material, DrawQuad::SOLID_COLOR);
EXPECT is nicer than ASSERT unless you're guarding crashes

https://codereview.chromium.org/979693005/diff/200001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:785:
ASSERT_EQ(pass_list[0]->quad_list.back()->material, DrawQuad::SOLID_COLOR);
Can you verify the pass_list[0]->quad_list size?

Did you mean to check the quad's color is transparent? The comment changed but
not the code.

https://codereview.chromium.org/979693005/diff/200001/cc/quads/list_container.h
File cc/quads/list_container.h (right):

https://codereview.chromium.org/979693005/diff/200001/cc/quads/list_container...
cc/quads/list_container.h:214: DerivedElementType*
ReplaceExistingElement(Iterator at) {
Can you add unit tests for this method?

[achaulk, 2015-03-27 17:17:47]

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

https://codereview.chromium.org/979693005/diff/200001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:785:
ASSERT_EQ(pass_list[0]->quad_list.back()->material, DrawQuad::SOLID_COLOR);
On 2015/03/24 17:55:15, danakj wrote:
> Can you verify the pass_list[0]->quad_list size?
> 
> Did you mean to check the quad's color is transparent? The comment changed but
> not the code.

Ah, I initially did, and changed it to an assert because of the MaterialCast,
but then I changed the solid color quad in the test to a checkered one since
that's a simpler way of doing it. I'll clean it up

https://codereview.chromium.org/979693005/diff/200001/cc/quads/list_container.h
File cc/quads/list_container.h (right):

https://codereview.chromium.org/979693005/diff/200001/cc/quads/list_container...
cc/quads/list_container.h:214: DerivedElementType*
ReplaceExistingElement(Iterator at) {
On 2015/03/24 17:55:15, danakj wrote:
> Can you add unit tests for this method?

Done.

[achaulk, 2015-04-01 16:13:16]

ping

[danakj, 2015-04-01 16:14:41]

LGTM

(I had looked when you posted your last comment but there was no changes. You
can publish a PTAL comment after you update the CL to avoid long waits and pings
hopefully)

[achaulk, 2015-04-01 16:20:57]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-04-01 16:20:58]

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/979693005/#ps240001
(title: "overlay test changes")

[commit-bot: I haz the power, 2015-04-01 16:21:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/979693005/240001

[commit-bot: I haz the power, 2015-04-01 17:26:25]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-04-01 17:27:36]

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