Enable partial texture upload support for browser compositor with Surfaces

Enable partial texture upload support for browser compositor with Surfaces

This requires that previously-used textures be returned to the browser compositor before a commit, so they can be uploaded to. This saves memory by reducing the number of textures that need to be created.

Also avoid releasing old textures from renderers on SubmitFrame, as that can be unnecessarily expensive if both the old and new frames reference the same textures.

BUG=446588, 446417, 446414
Committed: https://crrev.com/9bfb1a5808e5939e5b2ce272c0103902a252fcf6
Cr-Commit-Position: refs/heads/master@{#310737}

[jbauman, 2015-01-08 21:01:13]

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

[danakj, 2015-01-08 21:04:34]

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

[danakj, 2015-01-08 21:04:34]

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

https://codereview.chromium.org/843633003/diff/20001/cc/output/output_surface...
cc/output/output_surface.h:120: virtual void ForceReclaimResources() {}
Question.. what about hardware overlays? Does this flag being true also mean the
GLRenderer should avoid using hardware overlays for content from the UI
compositor?

https://codereview.chromium.org/843633003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/843633003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:293: DCHECK(output_surface_);
nit: no need to dcheck, we'll crash if it's null on the next line.

why is this at begin commit instead of commitcomplete? (you want this to be
before the pending tree starts to update things right?)

[jbauman, 2015-01-08 21:13:09]

Thanks for looking at this.

On 2015/01/08 21:04:34, danakj wrote:
>
https://codereview.chromium.org/843633003/diff/20001/cc/output/output_surface.h
> File cc/output/output_surface.h (right):
> 
>
https://codereview.chromium.org/843633003/diff/20001/cc/output/output_surface...
> cc/output/output_surface.h:120: virtual void ForceReclaimResources() {}
> Question.. what about hardware overlays? Does this flag being true also mean
the
> GLRenderer should avoid using hardware overlays for content from the UI
> compositor?

Good question - it seems like that's what will need to happen. What happens now?
I feel like it might hit a DCHECK currently if that happens and it attempts to
do a partial swap.

> 
>
https://codereview.chromium.org/843633003/diff/20001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/843633003/diff/20001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_impl.cc:293: DCHECK(output_surface_);
> nit: no need to dcheck, we'll crash if it's null on the next line.
> 
> why is this at begin commit instead of commitcomplete? (you want this to be
> before the pending tree starts to update things right?)

With impl-side painting disabled, partial texture updates happen in
ResourceUpdateController::Finalize, which happens before CommitComplete (at
least with the SingleThreadedProxy). It looks to me like impl-side painting
doesn't support partial-texture updates, so those memory-usage problems can be
fixed as they happen.

[danakj, 2015-01-08 21:17:15]

On 2015/01/08 21:13:09, jbauman wrote:
> Thanks for looking at this.
> 
> On 2015/01/08 21:04:34, danakj wrote:
> >
>
https://codereview.chromium.org/843633003/diff/20001/cc/output/output_surface.h
> > File cc/output/output_surface.h (right):
> > 
> >
>
https://codereview.chromium.org/843633003/diff/20001/cc/output/output_surface...
> > cc/output/output_surface.h:120: virtual void ForceReclaimResources() {}
> > Question.. what about hardware overlays? Does this flag being true also mean
> the
> > GLRenderer should avoid using hardware overlays for content from the UI
> > compositor?
> 
> Good question - it seems like that's what will need to happen. What happens
now?
> I feel like it might hit a DCHECK currently if that happens and it attempts to
> do a partial swap.

With a delegated renderer it won't reuse resources that are InUse, which overlay
textures would be - they aren't returned when other textures are. Most textures
are returned when they won't be drawn with anymore, overlay textures have to
wait until the next swap. I /think/ GLRenderer is involved in keeping them in
use/holding a ref on them.. or maybe it's moved.

> > 
> >
>
https://codereview.chromium.org/843633003/diff/20001/cc/trees/layer_tree_host...
> > File cc/trees/layer_tree_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/843633003/diff/20001/cc/trees/layer_tree_host...
> > cc/trees/layer_tree_host_impl.cc:293: DCHECK(output_surface_);
> > nit: no need to dcheck, we'll crash if it's null on the next line.
> > 
> > why is this at begin commit instead of commitcomplete? (you want this to be
> > before the pending tree starts to update things right?)
> 
> With impl-side painting disabled, partial texture updates happen in
> ResourceUpdateController::Finalize, which happens before CommitComplete (at
> least with the SingleThreadedProxy). It looks to me like impl-side painting
> doesn't support partial-texture updates, so those memory-usage problems can be
> fixed as they happen.

Ah so this is for non-implside. Mind putting it inside a if (!implside), maybe
with comment saying all of this stuff here, so we know to move/remove it later?

[jbauman, 2015-01-09 00:23:34]

On 2015/01/08 21:17:15, danakj wrote:
> On 2015/01/08 21:13:09, jbauman wrote:
> > Thanks for looking at this.
> > 
> > On 2015/01/08 21:04:34, danakj wrote:
> > >
> >
>
https://codereview.chromium.org/843633003/diff/20001/cc/output/output_surface.h
> > > File cc/output/output_surface.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/843633003/diff/20001/cc/output/output_surface...
> > > cc/output/output_surface.h:120: virtual void ForceReclaimResources() {}
> > > Question.. what about hardware overlays? Does this flag being true also
mean
> > the
> > > GLRenderer should avoid using hardware overlays for content from the UI
> > > compositor?
> > 
> > Good question - it seems like that's what will need to happen. What happens
> now?
> > I feel like it might hit a DCHECK currently if that happens and it attempts
to
> > do a partial swap.
> 
> With a delegated renderer it won't reuse resources that are InUse, which
overlay
> textures would be - they aren't returned when other textures are. Most
textures
> are returned when they won't be drawn with anymore, overlay textures have to
> wait until the next swap. I /think/ GLRenderer is involved in keeping them in
> use/holding a ref on them.. or maybe it's moved.

I don't think the code is smart enough now to avoid doing partial texture
updates for textures that are in use - as a test I originally tried to only set
"capabilities_.allow_partial_texture_updates = true" for the delegated renderer,
and that caused a DCHECK because the texture was in use in
ResourceProvider::SetPixels.
> 
> > > 
> > >
> >
>
https://codereview.chromium.org/843633003/diff/20001/cc/trees/layer_tree_host...
> > > File cc/trees/layer_tree_host_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/843633003/diff/20001/cc/trees/layer_tree_host...
> > > cc/trees/layer_tree_host_impl.cc:293: DCHECK(output_surface_);
> > > nit: no need to dcheck, we'll crash if it's null on the next line.
> > > 
> > > why is this at begin commit instead of commitcomplete? (you want this to
be
> > > before the pending tree starts to update things right?)
> > 
> > With impl-side painting disabled, partial texture updates happen in
> > ResourceUpdateController::Finalize, which happens before CommitComplete (at
> > least with the SingleThreadedProxy). It looks to me like impl-side painting
> > doesn't support partial-texture updates, so those memory-usage problems can
be
> > fixed as they happen.
> 
> Ah so this is for non-implside. Mind putting it inside a if (!implside), maybe
> with comment saying all of this stuff here, so we know to move/remove it
later?

Will do.

[danakj, 2015-01-09 00:33:00]

On Thu, Jan 8, 2015 at 4:23 PM, <jbauman@chromium.org> wrote:

> On 2015/01/08 21:17:15, danakj wrote:
>
>> On 2015/01/08 21:13:09, jbauman wrote:
>> > Thanks for looking at this.
>> >
>> > On 2015/01/08 21:04:34, danakj wrote:
>> > >
>> >
>>
>
> https://codereview.chromium.org/843633003/diff/20001/cc/
> output/output_surface.h
>
>> > > File cc/output/output_surface.h (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/843633003/diff/20001/cc/
> output/output_surface.h#newcode120
>
>> > > cc/output/output_surface.h:120: virtual void ForceReclaimResources()
>> {}
>> > > Question.. what about hardware overlays? Does this flag being true
>> also
>>
> mean
>
>> > the
>> > > GLRenderer should avoid using hardware overlays for content from the
>> UI
>> > > compositor?
>> >
>> > Good question - it seems like that's what will need to happen. What
>> happens
>> now?
>> > I feel like it might hit a DCHECK currently if that happens and it
>> attempts
>>
> to
>
>> > do a partial swap.
>>
>
>  With a delegated renderer it won't reuse resources that are InUse, which
>>
> overlay
>
>> textures would be - they aren't returned when other textures are. Most
>>
> textures
>
>> are returned when they won't be drawn with anymore, overlay textures have
>> to
>> wait until the next swap. I /think/ GLRenderer is involved in keeping
>> them in
>> use/holding a ref on them.. or maybe it's moved.
>>
>
> I don't think the code is smart enough now to avoid doing partial texture
> updates for textures that are in use - as a test I originally tried to
> only set
> "capabilities_.allow_partial_texture_updates = true" for the delegated
> renderer,
> and that caused a DCHECK because the texture was in use in
> ResourceProvider::SetPixels.


Sounds about right, we have code to deal with non-partial updates I guess,
but partial updates always assumed non-delegating. Is that something you
wanna address with this patch then or what's the plan?


> https://codereview.chromium.org/843633003/diff/20001/cc/
> trees/layer_tree_host_impl.cc
>
>> > > File cc/trees/layer_tree_host_impl.cc (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/843633003/diff/20001/cc/
> trees/layer_tree_host_impl.cc#newcode293
>
>> > > cc/trees/layer_tree_host_impl.cc:293: DCHECK(output_surface_);
>> > > nit: no need to dcheck, we'll crash if it's null on the next line.
>> > >
>> > > why is this at begin commit instead of commitcomplete? (you want this
>> to
>>
> be
>
>> > > before the pending tree starts to update things right?)
>> >
>> > With impl-side painting disabled, partial texture updates happen in
>> > ResourceUpdateController::Finalize, which happens before
>> CommitComplete (at
>> > least with the SingleThreadedProxy). It looks to me like impl-side
>> painting
>> > doesn't support partial-texture updates, so those memory-usage problems
>> can
>>
> be
>
>> > fixed as they happen.
>>
>
>  Ah so this is for non-implside. Mind putting it inside a if (!implside),
>> maybe
>> with comment saying all of this stuff here, so we know to move/remove it
>>
> later?
>
> Will do.
>
> https://codereview.chromium.org/843633003/
>

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

[jbauman, 2015-01-09 01:06:58]

PTAL

On 2015/01/09 00:33:00, danakj wrote:
> On Thu, Jan 8, 2015 at 4:23 PM, <mailto:jbauman@chromium.org> wrote:
> > I don't think the code is smart enough now to avoid doing partial texture
> > updates for textures that are in use - as a test I originally tried to
> > only set
> > "capabilities_.allow_partial_texture_updates = true" for the delegated
> > renderer,
> > and that caused a DCHECK because the texture was in use in
> > ResourceProvider::SetPixels.
> 
> 
> Sounds about right, we have code to deal with non-partial updates I guess,
> but partial updates always assumed non-delegating. Is that something you
> wanna address with this patch then or what's the plan?
> 

allow_overlay is always false on tile textures, so this patch should be fine. If
we want to allow turning tile textures into overlays, then we'll need to address
the issue then.
> 
> > https://codereview.chromium.org/843633003/diff/20001/cc/
> > trees/layer_tree_host_impl.cc
> >
> >> > > File cc/trees/layer_tree_host_impl.cc (right):
> >> > >
> >> > >
> >> >
> >>
> >
> > https://codereview.chromium.org/843633003/diff/20001/cc/
> > trees/layer_tree_host_impl.cc#newcode293
> >
> >> > > cc/trees/layer_tree_host_impl.cc:293: DCHECK(output_surface_);
> >> > > nit: no need to dcheck, we'll crash if it's null on the next line.
> >> > >
> >> > > why is this at begin commit instead of commitcomplete? (you want this
> >> to
> >>
> > be
> >
> >> > > before the pending tree starts to update things right?)
> >> >
> >> > With impl-side painting disabled, partial texture updates happen in
> >> > ResourceUpdateController::Finalize, which happens before
> >> CommitComplete (at
> >> > least with the SingleThreadedProxy). It looks to me like impl-side
> >> painting
> >> > doesn't support partial-texture updates, so those memory-usage problems
> >> can
> >>
> > be
> >
> >> > fixed as they happen.
> >>
> >
> >  Ah so this is for non-implside. Mind putting it inside a if (!implside),
> >> maybe
> >> with comment saying all of this stuff here, so we know to move/remove it
> >>
> > later?
> >
> > Will do.

Fixed.

[danakj, 2015-01-09 01:08:48]

On Thu, Jan 8, 2015 at 5:06 PM, <jbauman@chromium.org> wrote:

> PTAL
>
> On 2015/01/09 00:33:00, danakj wrote:
>
>> On Thu, Jan 8, 2015 at 4:23 PM, <mailto:jbauman@chromium.org> wrote:
>> > I don't think the code is smart enough now to avoid doing partial
>> texture
>> > updates for textures that are in use - as a test I originally tried to
>> > only set
>> > "capabilities_.allow_partial_texture_updates = true" for the delegated
>> > renderer,
>> > and that caused a DCHECK because the texture was in use in
>> > ResourceProvider::SetPixels.
>>
>
>
>  Sounds about right, we have code to deal with non-partial updates I guess,
>> but partial updates always assumed non-delegating. Is that something you
>> wanna address with this patch then or what's the plan?
>>
>
>
> allow_overlay is always false on tile textures, so this patch should be
> fine. If
> we want to allow turning tile textures into overlays, then we'll need to
> address
> the issue then.
>

Ah okay cool :)


>
>  > https://codereview.chromium.org/843633003/diff/20001/cc/
>> > trees/layer_tree_host_impl.cc
>> >
>> >> > > File cc/trees/layer_tree_host_impl.cc (right):
>> >> > >
>> >> > >
>> >> >
>> >>
>> >
>> > https://codereview.chromium.org/843633003/diff/20001/cc/
>> > trees/layer_tree_host_impl.cc#newcode293
>> >
>> >> > > cc/trees/layer_tree_host_impl.cc:293: DCHECK(output_surface_);
>> >> > > nit: no need to dcheck, we'll crash if it's null on the next line.
>> >> > >
>> >> > > why is this at begin commit instead of commitcomplete? (you want
>> this
>> >> to
>> >>
>> > be
>> >
>> >> > > before the pending tree starts to update things right?)
>> >> >
>> >> > With impl-side painting disabled, partial texture updates happen in
>> >> > ResourceUpdateController::Finalize, which happens before
>> >> CommitComplete (at
>> >> > least with the SingleThreadedProxy). It looks to me like impl-side
>> >> painting
>> >> > doesn't support partial-texture updates, so those memory-usage
>> problems
>> >> can
>> >>
>> > be
>> >
>> >> > fixed as they happen.
>> >>
>> >
>> >  Ah so this is for non-implside. Mind putting it inside a if
>> (!implside),
>> >> maybe
>> >> with comment saying all of this stuff here, so we know to move/remove
>> it
>> >>
>> > later?
>> >
>> > Will do.
>>
>
> Fixed.
>
>
> https://codereview.chromium.org/843633003/
>

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

[danakj, 2015-01-09 01:11:12]

https://codereview.chromium.org/843633003/diff/40001/cc/output/delegating_ren...
File cc/output/delegating_renderer.cc (right):

https://codereview.chromium.org/843633003/diff/40001/cc/output/delegating_ren...
cc/output/delegating_renderer.cc:44: capabilities_.allow_partial_texture_updates
=
Thanks, this LGTM for non-impl-side painting. (I didn't really look at the
cc/surfaces/ parts, james can review those?)

What happens when implside gets turned on in the browser and this capability is
present? Should we only turn this allow_ptt_updates when implside is off? What's
the right way to stage this?

[jamesr, 2015-01-09 01:14:45]

lgtm w/ slightly more descriptive name

https://codereview.chromium.org/843633003/diff/40001/content/browser/composit...
File content/browser/compositor/surface_display_output_surface.cc (right):

https://codereview.chromium.org/843633003/diff/40001/content/browser/composit...
content/browser/compositor/surface_display_output_surface.cc:81:
scoped_ptr<cc::CompositorFrame> frame_copy(new cc::CompositorFrame());
what is this a copy of? this looks more like a completely empty frame

[jbauman, 2015-01-09 01:25:10]

On 2015/01/09 01:11:12, danakj wrote:
>
https://codereview.chromium.org/843633003/diff/40001/cc/output/delegating_ren...
> File cc/output/delegating_renderer.cc (right):
> 
>
https://codereview.chromium.org/843633003/diff/40001/cc/output/delegating_ren...
> cc/output/delegating_renderer.cc:44:
capabilities_.allow_partial_texture_updates
> =
> Thanks, this LGTM for non-impl-side painting. (I didn't really look at the
> cc/surfaces/ parts, james can review those?)
> 
> What happens when implside gets turned on in the browser and this capability
is
> present? Should we only turn this allow_ptt_updates when implside is off?
What's
> the right way to stage this?

I don't believe impl-side painting ever uses this flag (or does partial texture
updates). So everything should just work when impl-side painting is used.

[danakj, 2015-01-09 01:34:26]

On Jan 8, 2015 5:25 PM, <jbauman@chromium.org> wrote:
>
> On 2015/01/09 01:11:12, danakj wrote:
>
>
https://codereview.chromium.org/843633003/diff/40001/cc/output/delegating_ren...
>>
>> File cc/output/delegating_renderer.cc (right):
>
>
>
>
https://codereview.chromium.org/843633003/diff/40001/cc/output/delegating_ren...
>>
>> cc/output/delegating_renderer.cc:44:
>
> capabilities_.allow_partial_texture_updates
>>
>> =
>> Thanks, this LGTM for non-impl-side painting. (I didn't really look at
the
>> cc/surfaces/ parts, james can review those?)
>
>
>> What happens when implside gets turned on in the browser and this
capability
>
> is
>>
>> present? Should we only turn this allow_ptt_updates when implside is off?
>
> What's
>>
>> the right way to stage this?
>
>
> I don't believe impl-side painting ever uses this flag (or does partial
texture
> updates). So everything should just work when impl-side painting is used.

Hm right. I'm remembering the future there. Cool then.

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

[jbauman, 2015-01-09 07:41:29]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2015-01-09 07:42:28]

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

[commit-bot: I haz the power, 2015-01-09 08:16:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-09 08:17:24]

Patchset 4 (id:??) landed as
https://crrev.com/9bfb1a5808e5939e5b2ce272c0103902a252fcf6
Cr-Commit-Position: refs/heads/master@{#310737}