cc: fix for huge filtered render surfaces.

cc: fix for huge filtered render surfaces.

Now that we're properly taking crop rects (filter region) into account
in accelerated reference filters, we may end up with a very large
size for the resulting render target. This is inefficient, and may fail
to allocate.

The fix is to apply the layer's clip_rect to the computed rect.

BUG=582357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5b2fc5f855c48e9037f07bb7589002fe156e4a3d
Cr-Commit-Position: refs/heads/master@{#374216}

[Stephen White, 2016-02-04 22:27:38]

Description was changed from

==========
cc: fix for huge filtered render surfaces.

Now that we're properly taking crop rects (filter region) into account
in accelerated reference filters, we may end up with a very large
size for the resulting render target. This is inefficient, and may fail
to allocate.

The fix is to apply the layer's clip_rect to the computed rect.

BUG=582357
==========

to

==========
cc: fix for huge filtered render surfaces.

Now that we're properly taking crop rects (filter region) into account
in accelerated reference filters, we may end up with a very large
size for the resulting render target. This is inefficient, and may fail
to allocate.

The fix is to apply the layer's clip_rect to the computed rect.

BUG=582357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Stephen White, 2016-02-04 22:41:10]

Description was changed from

==========
cc: fix for huge filtered render surfaces.

Now that we're properly taking crop rects (filter region) into account
in accelerated reference filters, we may end up with a very large
size for the resulting render target. This is inefficient, and may fail
to allocate.

The fix is to apply the layer's clip_rect to the computed rect.

BUG=582357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: fix for huge filtered render surfaces.

Now that we're properly taking crop rects (filter region) into account
in accelerated reference filters, we may end up with a very large
size for the resulting render target. This is inefficient, and may fail
to allocate.

The fix is to apply the layer's clip_rect to the computed rect.

BUG=582357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Stephen White, 2016-02-04 22:41:10]

senorblanco@chromium.org changed reviewers:
+ enne@chromium.org

[Stephen White, 2016-02-04 22:42:23]

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

[Stephen White, 2016-02-04 22:43:29]

enne@: PTAL. Thanks!

[commit-bot: I haz the power, 2016-02-04 22:46:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1670763002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1670763002/40001

[enne (OOO), 2016-02-04 22:50:04]

I don't think you can always count on there being a clip_rect set on the quad. 
I think you also need to clip against the viewport?  Can you add a test for that
case too?

[Stephen White, 2016-02-04 22:56:32]

On 2016/02/04 22:50:04, enne wrote:
> I don't think you can always count on there being a clip_rect set on the quad.

> I think you also need to clip against the viewport?  Can you add a test for
that
> case too?

Is this reachable from web content? Clipping against the viewport is a bit
trickier
since it's in a different coordinate space (window vs target) I think, and if
it's
not possible to reach it from web content, I'm not too worried about it.

[enne (OOO), 2016-02-04 23:09:35]

On 2016/02/04 at 22:56:32, senorblanco wrote:
> On 2016/02/04 22:50:04, enne wrote:
> > I don't think you can always count on there being a clip_rect set on the
quad. 
> > I think you also need to clip against the viewport?  Can you add a test for
that
> > case too?
> 
> Is this reachable from web content? Clipping against the viewport is a bit
trickier
> since it's in a different coordinate space (window vs target) I think, and if
it's
> not possible to reach it from web content, I'm not too worried about it.

I am not sure why it wouldn't be.  I would really prefer to make this just work,
as it's a supported operation in general to drop the clip rect on any quad where
it would also be correct to just let its target surface do the clipping.

[commit-bot: I haz the power, 2016-02-04 23:47:12]

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

[commit-bot: I haz the power, 2016-02-04 23:47:12]

Dry run: This issue passed the CQ dry run.

[Stephen White, 2016-02-05 16:49:13]

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

[commit-bot: I haz the power, 2016-02-05 16:49:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1670763002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1670763002/60001

[Stephen White, 2016-02-05 16:50:26]

On 2016/02/04 23:09:35, enne wrote:
> On 2016/02/04 at 22:56:32, senorblanco wrote:
> > On 2016/02/04 22:50:04, enne wrote:
> > > I don't think you can always count on there being a clip_rect set on the
> quad. 
> > > I think you also need to clip against the viewport?  Can you add a test
for
> that
> > > case too?
> > 
> > Is this reachable from web content? Clipping against the viewport is a bit
> trickier
> > since it's in a different coordinate space (window vs target) I think, and
if
> it's
> > not possible to reach it from web content, I'm not too worried about it.
> 
> I am not sure why it wouldn't be.  I would really prefer to make this just
work,
> as it's a supported operation in general to drop the clip rect on any quad
where
> it would also be correct to just let its target surface do the clipping.

OK, figured it out. PTAL.

[commit-bot: I haz the power, 2016-02-05 17:56:24]

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

[commit-bot: I haz the power, 2016-02-05 17:56:25]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2016-02-05 18:16:44]

Thanks for the followup.  Sorry to ask for more work!  lgtm

[Stephen White, 2016-02-05 18:22:16]

On 2016/02/05 18:16:44, enne wrote:
> Thanks for the followup.  Sorry to ask for more work!  lgtm

No problem. Unfortunately, this fix doesn't seem to work with scrolling. I'm
guessing the transform is wrong -- will keep digging.

[Stephen White, 2016-02-06 00:08:42]

Here's a repro case of the viewport failure, so I don't lose it. Will work on a
cc unit test.

<style>
.composited {
  will-change: transform;
}
.blurred {
  -webkit-filter: blur(3px);
}
.bright {
  -webkit-filter: brightness(101%);
}
.circle {
  width: 1200px;
  height: 1200px;
  border-radius: 600px;
}
.green {
  background-color: green;
}
</style>
<div class="bright">
  <div class="blurred composited green circle"></div>
</div>

[Stephen White, 2016-02-08 19:13:59]

Using the draw_rect instead of the viewport_rect seems to fix it. PTAL

[Stephen White, 2016-02-08 19:14:12]

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

[commit-bot: I haz the power, 2016-02-08 19:15:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1670763002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1670763002/100001

[enne (OOO), 2016-02-08 19:34:12]

Forgive my skepticism, but can you convince me that that's the right rect? I
would have figured device_clip_rect was what you needed?

Those variables in DirectRenderer are affected by both viewport offset and
framebuffer flipping.  It looks like LayerTreePixelTest already adds a
surface_expansion_size parameter by default, but has no way to do flipping *see
layer_tree_pixel_test.cc, search for flipped_output_surface = false.  Is it
possible to run your FilterWithGiantCropRectNoClip test with a flipped
framebuffer variation? I think if that worked properly, then I'd be confident
that this was the right value.

[Stephen White, 2016-02-08 19:42:29]

On 2016/02/08 19:34:12, enne wrote:
> Forgive my skepticism, but can you convince me that that's the right rect? I
> would have figured device_clip_rect was what you needed?

Your skepticism is appreciated, actually. viewport_rect always 
seems to have 0, 0 origin, and the quad_to_target_transform doesn't 
include scroll offsets, so draw_rect seems to be the only one which has the
right combination of scrolling and viewport reduction. But I just
determined that empirically.

> Those variables in DirectRenderer are affected by both viewport offset and
> framebuffer flipping.  It looks like LayerTreePixelTest already adds a
> surface_expansion_size parameter by default, but has no way to do flipping
*see
> layer_tree_pixel_test.cc, search for flipped_output_surface = false.  Is it
> possible to run your FilterWithGiantCropRectNoClip test with a flipped
> framebuffer variation? I think if that worked properly, then I'd be confident
> that this was the right value.

Will give it a try.

[Stephen White, 2016-02-08 19:49:08]

On 2016/02/08 19:34:12, enne wrote:
> Forgive my skepticism, but can you convince me that that's the right rect? I
> would have figured device_clip_rect was what you needed?

BTW, device_clip_rect also always seems to have 0,0 origin, regardless of
scrolling
(while draw_rect includes the scroll offset). Perhaps the scroll offset should
be 
coming from the transform instead, but I'm not sure how to retrieve it.

What I really need to do is clip the computed dst_rect the same way that the 
original render surface texture was clipped (and from which quad->rect is
computed),
but I have been unable to find that.

[enne (OOO), 2016-02-08 19:54:08]

On 2016/02/08 at 19:49:08, senorblanco wrote:
> On 2016/02/08 19:34:12, enne wrote:
> > Forgive my skepticism, but can you convince me that that's the right rect? I
> > would have figured device_clip_rect was what you needed?
> 
> BTW, device_clip_rect also always seems to have 0,0 origin, regardless of
scrolling
> (while draw_rect includes the scroll offset). Perhaps the scroll offset should
be 
> coming from the transform instead, but I'm not sure how to retrieve it.
> 
> What I really need to do is clip the computed dst_rect the same way that the 
> original render surface texture was clipped (and from which quad->rect is
computed),
> but I have been unable to find that.

I would expect the clip rect for the viewport not to move during scrolling. 
It's the content that moves around and the viewport stays put.  The quad to
target transform should include the scroll here, but the clip rect should be in
device space.

[commit-bot: I haz the power, 2016-02-08 22:40:29]

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

[commit-bot: I haz the power, 2016-02-08 22:40:30]

Dry run: This issue passed the CQ dry run.

[Stephen White, 2016-02-08 22:43:12]

On 2016/02/08 19:34:12, enne wrote:
> Forgive my skepticism, but can you convince me that that's the right rect? I
> would have figured device_clip_rect was what you needed?
> 
> Those variables in DirectRenderer are affected by both viewport offset and
> framebuffer flipping.  It looks like LayerTreePixelTest already adds a
> surface_expansion_size parameter by default, but has no way to do flipping
*see
> layer_tree_pixel_test.cc, search for flipped_output_surface = false.  Is it
> possible to run your FilterWithGiantCropRectNoClip test with a flipped
> framebuffer variation? I think if that worked properly, then I'd be confident
> that this was the right value.

OK, I'm not sure I'm doing this correctly, but I set flipped_output_surface to
'true' manually in layer_tree_pixel_test.cc, and ran
FilterWithGiantCropRectNoClip.GL and
it passed. Does that give any confidence?

[Stephen White, 2016-02-08 22:43:16]

On 2016/02/08 19:34:12, enne wrote:
> Forgive my skepticism, but can you convince me that that's the right rect? I
> would have figured device_clip_rect was what you needed?
> 
> Those variables in DirectRenderer are affected by both viewport offset and
> framebuffer flipping.  It looks like LayerTreePixelTest already adds a
> surface_expansion_size parameter by default, but has no way to do flipping
*see
> layer_tree_pixel_test.cc, search for flipped_output_surface = false.  Is it
> possible to run your FilterWithGiantCropRectNoClip test with a flipped
> framebuffer variation? I think if that worked properly, then I'd be confident
> that this was the right value.

OK, I'm not sure I'm doing this correctly, but I set flipped_output_surface to
'true' manually in layer_tree_pixel_test.cc, and ran
FilterWithGiantCropRectNoClip.GL and
it passed. Does that give any confidence?

[Stephen White, 2016-02-08 22:49:01]

For posterity, it looks like current_draw_rect_ gets set from  comes from
RenderPass::output_rect.

For RenderSurfaces, this seems to get set in
RenderSurfaceImpl::AppendRenderPasses() to RenderSurfaceImpl::content_rect_,
which was
clipped to the visible rect in
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre....

If that's any help in determining if it's in the right coordinate space.

[enne (OOO), 2016-02-08 22:54:30]

Thanks for investigating.  Sorry that my memory for which rect is which here is
not as fresh as I want it to be.

Yeah, current_draw_rect_ comes from render surface content rect, then that's
exactly what I would expect it to be.  If it works when flipped, then great.  :)

lgtm

[Stephen White, 2016-02-08 23:15:51]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2016-02-08 23:20:28]

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

[commit-bot: I haz the power, 2016-02-08 23:31:24]

Description was changed from

==========
cc: fix for huge filtered render surfaces.

Now that we're properly taking crop rects (filter region) into account
in accelerated reference filters, we may end up with a very large
size for the resulting render target. This is inefficient, and may fail
to allocate.

The fix is to apply the layer's clip_rect to the computed rect.

BUG=582357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: fix for huge filtered render surfaces.

Now that we're properly taking crop rects (filter region) into account
in accelerated reference filters, we may end up with a very large
size for the resulting render target. This is inefficient, and may fail
to allocate.

The fix is to apply the layer's clip_rect to the computed rect.

BUG=582357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-02-08 23:31:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-08 23:33:32]

Description was changed from

==========
cc: fix for huge filtered render surfaces.

Now that we're properly taking crop rects (filter region) into account
in accelerated reference filters, we may end up with a very large
size for the resulting render target. This is inefficient, and may fail
to allocate.

The fix is to apply the layer's clip_rect to the computed rect.

BUG=582357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: fix for huge filtered render surfaces.

Now that we're properly taking crop rects (filter region) into account
in accelerated reference filters, we may end up with a very large
size for the resulting render target. This is inefficient, and may fail
to allocate.

The fix is to apply the layer's clip_rect to the computed rect.

BUG=582357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5b2fc5f855c48e9037f07bb7589002fe156e4a3d
Cr-Commit-Position: refs/heads/master@{#374216}
==========

[commit-bot: I haz the power, 2016-02-08 23:33:34]

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