cc: Move filters from RenderPassDrawQuad to RenderPass

cc: Move filters from RenderPassDrawQuad to RenderPass

This CL moves filters and background filters from RenderPassDrawQuad to
RenderPass. Quads are stored in a ListContainer, which uses memcpy to
shift over elements when an element in the middle is deleted. Since
FilterOperations contain a vector, they're not safe to memcpy (and
this memcpy triggers crashes with http://crrev.com/2423483003).

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

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

[ajuma, 2016-11-30 18:24:47]

Description was changed from

==========
cc: Move filters from RenderPassDrawQuad to RenderPass

BUG=664357
==========

to

==========
cc: Move filters from RenderPassDrawQuad to RenderPass

BUG=664357
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ajuma, 2016-11-30 18:24:48]

Description was changed from

==========
cc: Move filters from RenderPassDrawQuad to RenderPass

BUG=664357
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
cc: Move filters from RenderPassDrawQuad to RenderPass

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

[ajuma, 2016-11-30 18:25:11]

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

[commit-bot: I haz the power, 2016-11-30 18:25:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ajuma, 2016-11-30 18:32:58]

Description was changed from

==========
cc: Move filters from RenderPassDrawQuad to RenderPass

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

to

==========
cc: Move filters from RenderPassDrawQuad to RenderPass

This CL moves filters and background filters from RenderPassDrawQuad to
RenderPass. Quads are stored in a ListContainer, which uses memcpy to
shift over elements when an element in the middle is deleted. Since
FilterOperations contain a vector, they're not safe to memcpy (and
this memcpy triggers crashes with http://crrev.com/2423483003).

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

[ajuma, 2016-11-30 18:39:43]

Description was changed from

==========
cc: Move filters from RenderPassDrawQuad to RenderPass

This CL moves filters and background filters from RenderPassDrawQuad to
RenderPass. Quads are stored in a ListContainer, which uses memcpy to
shift over elements when an element in the middle is deleted. Since
FilterOperations contain a vector, they're not safe to memcpy (and
this memcpy triggers crashes with http://crrev.com/2423483003).

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

to

==========
cc: Move filters from RenderPassDrawQuad to RenderPass

This CL moves filters and background filters from RenderPassDrawQuad to
RenderPass. Quads are stored in a ListContainer, which uses memcpy to
shift over elements when an element in the middle is deleted. Since
FilterOperations contain a vector, they're not safe to memcpy (and
this memcpy triggers crashes with http://crrev.com/2423483003).

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

[ajuma, 2016-11-30 18:39:44]

ajuma@chromium.org changed reviewers:
+ danakj@chromium.org, weiliangc@chromium.org

[ajuma, 2016-11-30 18:40:55]

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h#newc...
cc/quads/render_pass.h:157: std::vector<std::pair<RenderPassId,
FilterOperations>>;
I made this a vector since the number of render passes with filters is typically
very small. On the other hand, it's easy to make a page a with lots of filters,
so arguably this should be an unordered_map instead to optimize for the worst
case rather than the typical one. Opinions?

[commit-bot: I haz the power, 2016-11-30 19:56:44]

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

[commit-bot: I haz the power, 2016-11-30 19:56:45]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-12-01 01:50:47]

https://codereview.chromium.org/2543473004/diff/1/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/ipc/cc_param_traits.cc#n...
cc/ipc/cc_param_traits.cc:400: static size_t ReserveSizeForRenderPassWrite(const
cc::RenderPass& p) {
This needs to be updated for the filters

https://codereview.chromium.org/2543473004/diff/1/cc/ipc/struct_traits_unitte...
File cc/ipc/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/ipc/struct_traits_unitte...
cc/ipc/struct_traits_unittest.cc:558:
filters.Append(FilterOperation::CreateBlurFilter(0.f));
These are never checked that they survive?

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:169: render_pass_filters_.clear();
This isn't part of allocating.. while I appreciate sharing the same walk thru
the render pass list, I think it'd be better clarity to do this in DrawFrame
instead since the # of passes is really small.

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:179:
render_pass_filters_.push_back(std::make_pair(pass->id, pass->filters));
why copy the filters, you could store a pointer to them instead?

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:420: const FilterOperations*
DirectRenderer::FiltersForPass(RenderPassId id) const {
const&?

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:430: RenderPassId id) const {
const&

https://codereview.chromium.org/2543473004/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:914: const FilterOperations* background_filters,
if this can't be null maybe pass it by const& to here already

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

https://codereview.chromium.org/2543473004/diff/1/cc/output/overlay_unittest....
cc/output/overlay_unittest.cc:204: pass->SetAll(id, output_rect, output_rect,
gfx::Transform(),
maybe..setnew?

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h#newc...
cc/quads/render_pass.h:157: std::vector<std::pair<RenderPassId,
FilterOperations>>;
On 2016/11/30 18:40:54, ajuma wrote:
> I made this a vector since the number of render passes with filters is
typically
> very small. On the other hand, it's easy to make a page a with lots of
filters,
> so arguably this should be an unordered_map instead to optimize for the worst
> case rather than the typical one. Opinions?

You would need hundreds (or thousands?) of entries before a map would be better
for searching in. Vector is best. If you're worried about the size, you can
std::sort the vector after building it and use std::lower_bound to search in it
instead of find_if. That'd keep it fast for any size we can hit.

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
cc/surfaces/surface_aggregator.cc:614: bool in_moved_pixel_pass =
!!moved_pixel_passes_.count(remapped_pass_id);
This variable shadows the outer one with same name which is very confusing..

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
cc/surfaces/surface_aggregator.cc:623: SurfaceInfo{surface_quad->surface_id,
in_moved_pixel_pass,
aside.. this should use () not {} for the constructor
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...
tho emplace_back might be better

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
cc/surfaces/surface_aggregator.cc:629: pixel_moving_filter_passes.count(
I think this bit doesn't need to be in per-quad code anymore. This is making us
add to moved_pixel_passes_ any RenderPass with a moving-pixel filter.

We can just do that at the top of the render_pass loop if it has a moving-pixel
filter by checking render_pass.filters. And then we know we're inside a
moved_pixel_pass already for the code inside the loop.

The shadowing in_moved_pixel_pass makes it so that any RenderPass or Surface
appearing after (ie below) a background filtered RenderPass will be added to
moved_pixel_passes_, that still needs to be per-quad, and I agree we need to
prewalk the passes to find them since we go in the opposite order here.
However.. is there a reason to go in reversed order here anymore? I'm not seeing
it. If not then we don't need to prewalk we can just add a render_pass as we go
to the "has background filters" set.

https://codereview.chromium.org/2543473004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_widget_host_view_guest_unittest.cc
(right):

https://codereview.chromium.org/2543473004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_guest_unittest.cc:254:
gfx::Transform(), cc::FilterOperations(),
You could leave these out of SetNew if u want, it might make the patch smaller
and new code easier overall? SetNew is meant to be things you normally always
want to set. Then you can set individual fields after as needed.

https://codereview.chromium.org/2543473004/diff/1/services/ui/demo/bitmap_upl...
File services/ui/demo/bitmap_uploader.cc (right):

https://codereview.chromium.org/2543473004/diff/1/services/ui/demo/bitmap_upl...
services/ui/demo/bitmap_uploader.cc:87: cc::FilterOperations(),
cc::FilterOperations(),
This should use SetNew for that reason. It can set has_transparent_background
directly.

[weiliangc, 2016-12-01 19:04:31]

https://codereview.chromium.org/2543473004/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:914: const FilterOperations* background_filters,
On 2016/12/01 at 01:50:46, danakj wrote:
> if this can't be null maybe pass it by const& to here already

So it's every RP always have filters, but most times are empty v.s. RP only has
filters when they are not empty?

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h#newc...
cc/quads/render_pass.h:157: std::vector<std::pair<RenderPassId,
FilterOperations>>;
On 2016/12/01 at 01:50:46, danakj wrote:
> On 2016/11/30 18:40:54, ajuma wrote:
> > I made this a vector since the number of render passes with filters is
typically
> > very small. On the other hand, it's easy to make a page a with lots of
filters,
> > so arguably this should be an unordered_map instead to optimize for the
worst
> > case rather than the typical one. Opinions?
> 
> You would need hundreds (or thousands?) of entries before a map would be
better for searching in. Vector is best. If you're worried about the size, you
can std::sort the vector after building it and use std::lower_bound to search in
it instead of find_if. That'd keep it fast for any size we can hit.

Maybe overall just use FilterOperations* when not on render pass? Avoids copy
when building RPFilterList too.

[weiliangc, 2016-12-01 20:16:11]

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h#newc...
cc/quads/render_pass.h:157: std::vector<std::pair<RenderPassId,
FilterOperations>>;
On 2016/12/01 01:50:46, danakj wrote:
> On 2016/11/30 18:40:54, ajuma wrote:
> > I made this a vector since the number of render passes with filters is
> typically
> > very small. On the other hand, it's easy to make a page a with lots of
> filters,
> > so arguably this should be an unordered_map instead to optimize for the
worst
> > case rather than the typical one. Opinions?
> 
> You would need hundreds (or thousands?) of entries before a map would be
better
> for searching in. Vector is best. If you're worried about the size, you can
> std::sort the vector after building it and use std::lower_bound to search in
it
> instead of find_if. That'd keep it fast for any size we can hit.

Also sort of unrelated to the CL, but isn't using std::sort and std::lower_bound
the same as using std::map and std::map::lower_bound?

[weiliangc, 2016-12-01 20:23:09]

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h#newc...
cc/quads/render_pass.h:157: std::vector<std::pair<RenderPassId,
FilterOperations>>;
On 2016/12/01 20:16:11, weiliangc wrote:
> On 2016/12/01 01:50:46, danakj wrote:
> > On 2016/11/30 18:40:54, ajuma wrote:
> > > I made this a vector since the number of render passes with filters is
> > typically
> > > very small. On the other hand, it's easy to make a page a with lots of
> > filters,
> > > so arguably this should be an unordered_map instead to optimize for the
> worst
> > > case rather than the typical one. Opinions?
> > 
> > You would need hundreds (or thousands?) of entries before a map would be
> better
> > for searching in. Vector is best. If you're worried about the size, you can
> > std::sort the vector after building it and use std::lower_bound to search in
> it
> > instead of find_if. That'd keep it fast for any size we can hit.
> 
> Also sort of unrelated to the CL, but isn't using std::sort and
std::lower_bound
> the same as using std::map and std::map::lower_bound?

Wait no just realized std::map will need to be balanced, nvm disregard previous
comment :P

[danakj, 2016-12-01 21:14:34]

https://codereview.chromium.org/2543473004/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:914: const FilterOperations* background_filters,
On 2016/12/01 19:04:31, weiliangc wrote:
> On 2016/12/01 at 01:50:46, danakj wrote:
> > if this can't be null maybe pass it by const& to here already
> 
> So it's every RP always have filters, but most times are empty v.s. RP only
has
> filters when they are not empty?

No we don't call this function if it was null (we deref it on 925).

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h#newc...
cc/quads/render_pass.h:157: std::vector<std::pair<RenderPassId,
FilterOperations>>;
On 2016/12/01 20:23:09, weiliangc wrote:
> On 2016/12/01 20:16:11, weiliangc wrote:
> > On 2016/12/01 01:50:46, danakj wrote:
> > > On 2016/11/30 18:40:54, ajuma wrote:
> > > > I made this a vector since the number of render passes with filters is
> > > typically
> > > > very small. On the other hand, it's easy to make a page a with lots of
> > > filters,
> > > > so arguably this should be an unordered_map instead to optimize for the
> > worst
> > > > case rather than the typical one. Opinions?
> > > 
> > > You would need hundreds (or thousands?) of entries before a map would be
> > better
> > > for searching in. Vector is best. If you're worried about the size, you
can
> > > std::sort the vector after building it and use std::lower_bound to search
in
> > it
> > > instead of find_if. That'd keep it fast for any size we can hit.
> > 
> > Also sort of unrelated to the CL, but isn't using std::sort and
> std::lower_bound
> > the same as using std::map and std::map::lower_bound?
> 
> Wait no just realized std::map will need to be balanced, nvm disregard
previous
> comment :P

also std::map is a pointery (ie terrible) data structure

[weiliangc, 2016-12-01 21:20:54]

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:179:
render_pass_filters_.push_back(std::make_pair(pass->id, pass->filters));
On 2016/12/01 01:50:46, danakj wrote:
> why copy the filters, you could store a pointer to them instead?

I mean we store the filters pointers here, and when search RenderPass's filters,
return nullptr?

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:420: const FilterOperations*
DirectRenderer::FiltersForPass(RenderPassId id) const {
On 2016/12/01 01:50:46, danakj wrote:
> const&?

like mentioned in line 179, if we store pointers, we should return pointers
here?

[danakj, 2016-12-01 21:26:03]

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:179:
render_pass_filters_.push_back(std::make_pair(pass->id, pass->filters));
On 2016/12/01 21:20:54, weiliangc wrote:
> On 2016/12/01 01:50:46, danakj wrote:
> > why copy the filters, you could store a pointer to them instead?
> 
> I mean we store the filters pointers here, and when search RenderPass's
filters,
> return nullptr?

i'm not totally sure what you mean, but what i mean is render_pass_filters_ is a
RenderPassFilterList which is a std::vector<std::pair<RenderPassId,
FilterOperations>>. I'm suggesting  std::vector<std::pair<RenderPassId,
FilterOperations*>> with a pointer to the RenderPass's |filters|.

Alternatively we used to have a map of RenderPassId to RenderPass* for
DrawLayers but it went unused. We could have that and then we only need one
std::vector<std::pair<RenderPassId, RenderPass*>> thing to find both types of
filters.

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:420: const FilterOperations*
DirectRenderer::FiltersForPass(RenderPassId id) const {
On 2016/12/01 21:20:54, weiliangc wrote:
> On 2016/12/01 01:50:46, danakj wrote:
> > const&?
> 
> like mentioned in line 179, if we store pointers, we should return pointers
> here?

here I'm talking about the |id|

[weiliangc, 2016-12-02 17:19:13]

On 2016/12/01 at 21:26:03, danakj wrote:
> https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.cc
> File cc/output/direct_renderer.cc (right):
> 
>
https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
> cc/output/direct_renderer.cc:179:
render_pass_filters_.push_back(std::make_pair(pass->id, pass->filters));
> On 2016/12/01 21:20:54, weiliangc wrote:
> > On 2016/12/01 01:50:46, danakj wrote:
> > > why copy the filters, you could store a pointer to them instead?
> > 
> > I mean we store the filters pointers here, and when search RenderPass's
filters,
> > return nullptr?
> 
> i'm not totally sure what you mean, but what i mean is render_pass_filters_ is
a RenderPassFilterList which is a std::vector<std::pair<RenderPassId,
FilterOperations>>. I'm suggesting  std::vector<std::pair<RenderPassId,
FilterOperations*>> with a pointer to the RenderPass's |filters|.
> 
> Alternatively we used to have a map of RenderPassId to RenderPass* for
DrawLayers but it went unused. We could have that and then we only need one
std::vector<std::pair<RenderPassId, RenderPass*>> thing to find both types of
filters.
> 
^ Agreed.

>
https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
> cc/output/direct_renderer.cc:420: const FilterOperations*
DirectRenderer::FiltersForPass(RenderPassId id) const {
> On 2016/12/01 21:20:54, weiliangc wrote:
> > On 2016/12/01 01:50:46, danakj wrote:
> > > const&?
> > 
> > like mentioned in line 179, if we store pointers, we should return pointers
> > here?
> 
> here I'm talking about the |id|

I see. Sounds good.

[jbauman, 2016-12-07 01:15:46]

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

[jbauman, 2016-12-07 01:15:47]

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
cc/surfaces/surface_aggregator.cc:604: RemapPassId(render_pass->id,
surface_id));
I don't think you need this - I think you can just check do 
"bool in_moved_pixel_pass = !!moved_pixel_passes_.count(remapped_pass_id) ||
render_pass->filters.HasFilterThatMovesPixels();" below.

[ajuma, 2016-12-13 14:35:40]

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

[commit-bot: I haz the power, 2016-12-13 14:35:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ajuma, 2016-12-13 15:08:12]

Thanks for the reviews!

https://codereview.chromium.org/2543473004/diff/1/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/ipc/cc_param_traits.cc#n...
cc/ipc/cc_param_traits.cc:400: static size_t ReserveSizeForRenderPassWrite(const
cc::RenderPass& p) {
On 2016/12/01 01:50:46, danakj wrote:
> This needs to be updated for the filters

Done.

https://codereview.chromium.org/2543473004/diff/1/cc/ipc/struct_traits_unitte...
File cc/ipc/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/ipc/struct_traits_unitte...
cc/ipc/struct_traits_unittest.cc:558:
filters.Append(FilterOperation::CreateBlurFilter(0.f));
On 2016/12/01 01:50:46, danakj wrote:
> These are never checked that they survive?

Oops, added checks.

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:169: render_pass_filters_.clear();
On 2016/12/01 01:50:46, danakj wrote:
> This isn't part of allocating.. while I appreciate sharing the same walk thru
> the render pass list, I think it'd be better clarity to do this in DrawFrame
> instead since the # of passes is really small.

Done.

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:179:
render_pass_filters_.push_back(std::make_pair(pass->id, pass->filters));
On 2016/12/01 21:26:02, danakj wrote:
> On 2016/12/01 21:20:54, weiliangc wrote:
> > On 2016/12/01 01:50:46, danakj wrote:
> > > why copy the filters, you could store a pointer to them instead?
> > 
> > I mean we store the filters pointers here, and when search RenderPass's
> filters,
> > return nullptr?
> 
> i'm not totally sure what you mean, but what i mean is render_pass_filters_ is
a
> RenderPassFilterList which is a std::vector<std::pair<RenderPassId,
> FilterOperations>>. I'm suggesting  std::vector<std::pair<RenderPassId,
> FilterOperations*>> with a pointer to the RenderPass's |filters|.
> 
> Alternatively we used to have a map of RenderPassId to RenderPass* for
> DrawLayers but it went unused. We could have that and then we only need one
> std::vector<std::pair<RenderPassId, RenderPass*>> thing to find both types of
> filters.

Changed to storing a pointer to the RenderPass's filters.

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:420: const FilterOperations*
DirectRenderer::FiltersForPass(RenderPassId id) const {
On 2016/12/01 21:26:02, danakj wrote:
> On 2016/12/01 21:20:54, weiliangc wrote:
> > On 2016/12/01 01:50:46, danakj wrote:
> > > const&?
> > 
> > like mentioned in line 179, if we store pointers, we should return pointers
> > here?
> 
> here I'm talking about the |id|

Changed |id| to const&, but then changed back to pass-by-value when rebasing
since render pass ids are now ints.

https://codereview.chromium.org/2543473004/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:430: RenderPassId id) const {
On 2016/12/01 01:50:46, danakj wrote:
> const&

Done.

https://codereview.chromium.org/2543473004/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:914: const FilterOperations* background_filters,
On 2016/12/01 01:50:46, danakj wrote:
> if this can't be null maybe pass it by const& to here already

Done.

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

https://codereview.chromium.org/2543473004/diff/1/cc/output/overlay_unittest....
cc/output/overlay_unittest.cc:204: pass->SetAll(id, output_rect, output_rect,
gfx::Transform(),
On 2016/12/01 01:50:46, danakj wrote:
> maybe..setnew?

Done.

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2543473004/diff/1/cc/quads/render_pass.h#newc...
cc/quads/render_pass.h:157: std::vector<std::pair<RenderPassId,
FilterOperations>>;
On 2016/12/01 01:50:46, danakj wrote:
> On 2016/11/30 18:40:54, ajuma wrote:
> > I made this a vector since the number of render passes with filters is
> typically
> > very small. On the other hand, it's easy to make a page a with lots of
> filters,
> > so arguably this should be an unordered_map instead to optimize for the
worst
> > case rather than the typical one. Opinions?
> 
> You would need hundreds (or thousands?) of entries before a map would be
better
> for searching in. Vector is best. If you're worried about the size, you can
> std::sort the vector after building it and use std::lower_bound to search in
it
> instead of find_if. That'd keep it fast for any size we can hit.

Changed to sorting the vector and using std::lower_bound.

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
cc/surfaces/surface_aggregator.cc:604: RemapPassId(render_pass->id,
surface_id));
On 2016/12/07 01:15:47, jbauman wrote:
> I don't think you need this - I think you can just check do 
> "bool in_moved_pixel_pass = !!moved_pixel_passes_.count(remapped_pass_id) ||
> render_pass->filters.HasFilterThatMovesPixels();" below.

Done.

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
cc/surfaces/surface_aggregator.cc:614: bool in_moved_pixel_pass =
!!moved_pixel_passes_.count(remapped_pass_id);
On 2016/12/01 01:50:46, danakj wrote:
> This variable shadows the outer one with same name which is very confusing..

Renamed the outer one to |in_moved_pixel_surface|.

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
cc/surfaces/surface_aggregator.cc:623: SurfaceInfo{surface_quad->surface_id,
in_moved_pixel_pass,
On 2016/12/01 01:50:46, danakj wrote:
> aside.. this should use () not {} for the constructor
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...
> tho emplace_back might be better

Changed to using emplace_back.

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
cc/surfaces/surface_aggregator.cc:629: pixel_moving_filter_passes.count(
On 2016/12/01 01:50:46, danakj_OOO_and_slow wrote:
> I think this bit doesn't need to be in per-quad code anymore. This is making
us
> add to moved_pixel_passes_ any RenderPass with a moving-pixel filter.
>
> We can just do that at the top of the render_pass loop if it has a
moving-pixel
> filter by checking render_pass.filters. And then we know we're inside a
> moved_pixel_pass already for the code inside the loop.

Done.

> The shadowing in_moved_pixel_pass makes it so that any RenderPass or Surface
> appearing after (ie below) a background filtered RenderPass will be added to
> moved_pixel_passes_, that still needs to be per-quad, and I agree we need to
> prewalk the passes to find them since we go in the opposite order here.
> However.. is there a reason to go in reversed order here anymore? I'm not
seeing
> it. If not then we don't need to prewalk we can just add a render_pass as we
go
> to the "has background filters" set.

We need to go in reversed order since for any pass that has a (foreground)
filter, we want contributing passes to also go into moved_pixel_passes_. So we
need to visit passes before visiting their contributing passes.

Expanded one of the surface aggregator unit tests for this.

https://codereview.chromium.org/2543473004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_widget_host_view_guest_unittest.cc
(right):

https://codereview.chromium.org/2543473004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_guest_unittest.cc:254:
gfx::Transform(), cc::FilterOperations(),
On 2016/12/01 01:50:46, danakj wrote:
> You could leave these out of SetNew if u want, it might make the patch smaller
> and new code easier overall? SetNew is meant to be things you normally always
> want to set. Then you can set individual fields after as needed.

Done.

https://codereview.chromium.org/2543473004/diff/1/services/ui/demo/bitmap_upl...
File services/ui/demo/bitmap_uploader.cc (right):

https://codereview.chromium.org/2543473004/diff/1/services/ui/demo/bitmap_upl...
services/ui/demo/bitmap_uploader.cc:87: cc::FilterOperations(),
cc::FilterOperations(),
On 2016/12/01 01:50:46, danakj wrote:
> This should use SetNew for that reason. It can set has_transparent_background
> directly.

Done.

[commit-bot: I haz the power, 2016-12-13 17:06:55]

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

[commit-bot: I haz the power, 2016-12-13 17:06:56]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-12-14 23:23:57]

1 question then LGTM

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2543473004/diff/1/cc/surfaces/surface_aggrega...
cc/surfaces/surface_aggregator.cc:623: SurfaceInfo{surface_quad->surface_id,
in_moved_pixel_pass,
On 2016/12/13 15:08:11, ajuma wrote:
> On 2016/12/01 01:50:46, danakj wrote:
> > aside.. this should use () not {} for the constructor
> >
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...
> > tho emplace_back might be better
> 
> Changed to using emplace_back.

So uh, I guess that was ok cuz it was struct init not a constructor.. but this
is much more clear so thanks.

https://codereview.chromium.org/2543473004/diff/20001/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2543473004/diff/20001/cc/ipc/cc_param_traits....
cc/ipc/cc_param_traits.cc:413: base::PickleSizer sizer;
Oh this is neet, I never knew about this. I was wondering if p.filters.size() *
sizeof(FilterOperation) would be good enough tho there is a SkImageFilter* on
FilterOperations so it would miss that. Cool.

https://codereview.chromium.org/2543473004/diff/20001/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2543473004/diff/20001/cc/quads/render_pass.h#...
cc/quads/render_pass.h:154: using RenderPassFilterList =
I think we need to either mention here that this is sorted so you can find
things with std::lower_bound, and then it should be true for all uses.. or
mention it everywhere that can make sense in DirectRenderer and maybe the
overlay code. Because thats an invisible thing about this vector until we can
use some nicer flat_map type structure that encapsulates it.

https://codereview.chromium.org/2543473004/diff/20001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2543473004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:620: bool in_moved_pixel_pass =
!!moved_pixel_passes_.count(remapped_pass_id);
It could be nice to not force a lookup to happen if we already know its there
cuz we just inserted it, if writing that isnt too awkward to read?

https://codereview.chromium.org/2543473004/diff/20001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator_unittest.cc (right):

https://codereview.chromium.org/2543473004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_unittest.cc:1632: RenderPass* pass =
root_pass_list[1].get();
can u name this |filter_pass| now cuz there's 2 non-root ones.

https://codereview.chromium.org/2543473004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_unittest.cc:1633:
pass->shared_quad_state_list.front()->quad_to_target_transform.Translate(0,
Why the 0,0 change, and why is that different than not doing anything?

[ajuma, 2016-12-15 16:48:00]

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

[commit-bot: I haz the power, 2016-12-15 16:48:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ajuma, 2016-12-15 16:49:21]

https://codereview.chromium.org/2543473004/diff/20001/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2543473004/diff/20001/cc/quads/render_pass.h#...
cc/quads/render_pass.h:154: using RenderPassFilterList =
On 2016/12/14 23:23:57, danakj_OOO_and_slow wrote:
> I think we need to either mention here that this is sorted so you can find
> things with std::lower_bound, and then it should be true for all uses.. or
> mention it everywhere that can make sense in DirectRenderer and maybe the
> overlay code. Because thats an invisible thing about this vector until we can
> use some nicer flat_map type structure that encapsulates it.

Added a comment here.

https://codereview.chromium.org/2543473004/diff/20001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2543473004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:620: bool in_moved_pixel_pass =
!!moved_pixel_passes_.count(remapped_pass_id);
On 2016/12/14 23:23:57, danakj_OOO_and_slow wrote:
> It could be nice to not force a lookup to happen if we already know its there
> cuz we just inserted it, if writing that isnt too awkward to read?

Done.

https://codereview.chromium.org/2543473004/diff/20001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator_unittest.cc (right):

https://codereview.chromium.org/2543473004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_unittest.cc:1632: RenderPass* pass =
root_pass_list[1].get();
On 2016/12/14 23:23:57, danakj_OOO_and_slow wrote:
> can u name this |filter_pass| now cuz there's 2 non-root ones.

Done.

https://codereview.chromium.org/2543473004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_unittest.cc:1633:
pass->shared_quad_state_list.front()->quad_to_target_transform.Translate(0,
On 2016/12/14 23:23:57, danakj_OOO_and_slow wrote:
> Why the 0,0 change, and why is that different than not doing anything?

Oh, I changed that while debugging and forgot to revert. Thanks for catching it!

[ajuma, 2016-12-15 16:58:25]

ajuma@chromium.org changed reviewers:
+ boliu@chromium.org, junov@chromium.org, reveman@chromium.org,
sky@chromium.org, tedchoc@chromium.org, tsepez@chromium.org

[ajuma, 2016-12-15 17:00:20]

+tsepez for cc/ipc security
+boliu for android_webview/
+reveman for components/exo
+tedchoc for content/renderer/android
+sky for services/ui
+junov for third_party/WebKit/Source/platform/graphics

[sky, 2016-12-15 17:43:29]

LGTM

[Tom Sepez, 2016-12-15 17:45:50]

LGTM, but can we get out of the business of doing memcpy's and use some
supported container like std::vector down the road?

[reveman, 2016-12-15 17:56:20]

components/exo lgtm

[commit-bot: I haz the power, 2016-12-15 18:18:19]

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

[commit-bot: I haz the power, 2016-12-15 18:18:20]

Dry run: This issue passed the CQ dry run.

[boliu, 2016-12-15 18:21:55]

lgtm

[Ted C, 2016-12-15 18:47:07]

On 2016/12/15 18:21:55, boliu wrote:
> lgtm

lgtm

[Justin Novosad, 2016-12-15 21:54:38]

Webkit lgtm

[ajuma, 2016-12-15 22:17:43]

The CQ bit was checked by ajuma@chromium.org

[ajuma, 2016-12-15 22:17:43]

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/2543473004/#ps80001
(title: "Rebase again")

[commit-bot: I haz the power, 2016-12-15 22:18:55]

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, 2016-12-15 22:35:05]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481840263112810,
"parent_rev": "f7519b1eac6b7c178f575255fcdbe4dc88583cc1", "commit_rev":
"3ee7cbe1aa17bb8b6fad7ccda54b778c850b3839"}

[commit-bot: I haz the power, 2016-12-15 22:36:39]

Description was changed from

==========
cc: Move filters from RenderPassDrawQuad to RenderPass

This CL moves filters and background filters from RenderPassDrawQuad to
RenderPass. Quads are stored in a ListContainer, which uses memcpy to
shift over elements when an element in the middle is deleted. Since
FilterOperations contain a vector, they're not safe to memcpy (and
this memcpy triggers crashes with http://crrev.com/2423483003).

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

to

==========
cc: Move filters from RenderPassDrawQuad to RenderPass

This CL moves filters and background filters from RenderPassDrawQuad to
RenderPass. Quads are stored in a ListContainer, which uses memcpy to
shift over elements when an element in the middle is deleted. Since
FilterOperations contain a vector, they're not safe to memcpy (and
this memcpy triggers crashes with http://crrev.com/2423483003).

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

Review-Url: https://codereview.chromium.org/2543473004
==========

[commit-bot: I haz the power, 2016-12-15 22:36:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-15 22:38:42]

Description was changed from

==========
cc: Move filters from RenderPassDrawQuad to RenderPass

This CL moves filters and background filters from RenderPassDrawQuad to
RenderPass. Quads are stored in a ListContainer, which uses memcpy to
shift over elements when an element in the middle is deleted. Since
FilterOperations contain a vector, they're not safe to memcpy (and
this memcpy triggers crashes with http://crrev.com/2423483003).

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

Review-Url: https://codereview.chromium.org/2543473004
==========

to

==========
cc: Move filters from RenderPassDrawQuad to RenderPass

This CL moves filters and background filters from RenderPassDrawQuad to
RenderPass. Quads are stored in a ListContainer, which uses memcpy to
shift over elements when an element in the middle is deleted. Since
FilterOperations contain a vector, they're not safe to memcpy (and
this memcpy triggers crashes with http://crrev.com/2423483003).

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

Committed: https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47
Cr-Commit-Position: refs/heads/master@{#438938}
==========

[commit-bot: I haz the power, 2016-12-15 22:38:44]

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