cc: Change the restrictions on overlays for video

cc: Change the restrictions on overlays for video

- Allow positively scaled transformations. Overlay hardware can handle this
- Allow the STREAM_VIDEO_QUAD type. Tests added for that type

TEST=manual test, video in overlay

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

[achaulk, 2015-01-19 18:05:49]

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

[danakj, 2015-01-21 23:30:33]

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
File cc/output/overlay_strategy_single_on_top.cc (right):

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:27: if (draw_quad->material ==
DrawQuad::TEXTURE_CONTENT) {
how about a switch instead of an if/elseif/else? then if a new quad type happens
we'll have to come visit here and see if it makes sense to handle it.

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:29: if (!quad) {
The material cast will DCHECK if it's not a texture quad. You don't need to
handle null here

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:36: if (!quad) {
ditto

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:70:
OverlayCandidate::GetOverlayTransform(quad.quadTransform(), false);
use a temp var to give boolean literal a name

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:88: quad_info->transform =
gfx::OVERLAY_TRANSFORM_FLIP_HORIZONTAL;
This all gets very nested, it'd be nice to do this in a way with less nesting.
One way that comes to mind is to break the TEXTURE and the STREAM cases out into
individual helper functions?

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:158: if
(rect.Intersects(overlap_rect) && QuadBlocksOverlay(*overlap_iter)) {
This seems like a 3rd change, allowing overlapping non-opaque solid color quads
above an overlay? Can you split this out into its own CL?

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:177: main_image.plane_z_order = 0;
Unrelated change?

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
File cc/output/overlay_strategy_single_on_top.h (right):

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.h:30: // Returns true if |draw_quad|
will block quads underheath from becoming an
underneath

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.h:32: bool QuadBlocksOverlay(const
DrawQuad* draw_quad);
nit: start with a verb: DoesQuadBlockOverlay

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

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:492:
pass->shared_quad_state_list.back()->content_to_target_transform.Scale(2.f,
Seems like there's some new corner cases here, should test negative scale too?

[achaulk, 2015-01-23 20:10:56]

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
File cc/output/overlay_strategy_single_on_top.cc (right):

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:27: if (draw_quad->material ==
DrawQuad::TEXTURE_CONTENT) {
On 2015/01/21 23:30:33, danakj wrote:
> how about a switch instead of an if/elseif/else? then if a new quad type
happens
> we'll have to come visit here and see if it makes sense to handle it.

Done.

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:70:
OverlayCandidate::GetOverlayTransform(quad.quadTransform(), false);
On 2015/01/21 23:30:33, danakj wrote:
> use a temp var to give boolean literal a name

Done.

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:88: quad_info->transform =
gfx::OVERLAY_TRANSFORM_FLIP_HORIZONTAL;
On 2015/01/21 23:30:33, danakj wrote:
> This all gets very nested, it'd be nice to do this in a way with less nesting.
> One way that comes to mind is to break the TEXTURE and the STREAM cases out
into
> individual helper functions?

Done.

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:158: if
(rect.Intersects(overlap_rect) && QuadBlocksOverlay(*overlap_iter)) {
On 2015/01/21 23:30:33, danakj wrote:
> This seems like a 3rd change, allowing overlapping non-opaque solid color
quads
> above an overlay? Can you split this out into its own CL?

Done.

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:177: main_image.plane_z_order = 0;
On 2015/01/21 23:30:33, danakj wrote:
> Unrelated change?

Just making it explicit, the ctor does this too

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
File cc/output/overlay_strategy_single_on_top.h (right):

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.h:32: bool QuadBlocksOverlay(const
DrawQuad* draw_quad);
On 2015/01/21 23:30:33, danakj wrote:
> nit: start with a verb: DoesQuadBlockOverlay

Done.

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

https://codereview.chromium.org/855403002/diff/20001/cc/output/overlay_unitte...
cc/output/overlay_unittest.cc:492:
pass->shared_quad_state_list.back()->content_to_target_transform.Scale(2.f,
On 2015/01/21 23:30:33, danakj wrote:
> Seems like there's some new corner cases here, should test negative scale too?

Done.

[achaulk, 2015-01-29 23:36:26]

ping

[danakj, 2015-02-03 18:41:35]

lgtm

https://codereview.chromium.org/855403002/diff/40001/cc/output/overlay_strate...
File cc/output/overlay_strategy_single_on_top.cc (right):

https://codereview.chromium.org/855403002/diff/40001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.cc:165: main_image.plane_z_order = 0;
there's no need to init things that the constructor does. 

can you remove the rgba one too?

https://codereview.chromium.org/855403002/diff/40001/cc/output/overlay_strate...
File cc/output/overlay_strategy_single_on_top.h (right):

https://codereview.chromium.org/855403002/diff/40001/cc/output/overlay_strate...
cc/output/overlay_strategy_single_on_top.h:17: class TextureDrawQuad;
alphabetical

[achaulk, 2015-02-03 19:39:09]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2015-02-03 19:39:52]

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

[commit-bot: I haz the power, 2015-02-03 19:43:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-03 19:46:10]

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