Make software compositing work on Mac.

Make software compositing work on Mac.

Rename the FramebufferMemoryManager class to
SoftwareFrameMemoryManager. Pull out common members
to Aura and Mac and move them to SoftwareFramebufferManager.
Rename the FramebufferHolder to SoftwareFrame.

This is still disabled unless --enable-software-compositing is passed
in and has not been thoroughly tested.

BUG=286038
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230900

[ccameron, 2013-10-03 21:46:00]

This more-or-less works. We don't successfully switch to software compositor
when the GPU process is killed 3 times (the renderer crashes instead). We also
can't load Flash (or any other plugins) for some reason.

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (left):

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:2997: void
RenderWidgetHostViewAura::ReleaseCurrentFrame() {
This is a functional change in that we now ignore current_surface_ and
unconditionally reclaim frames.

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:3183:
framebuffer_holder_ = NULL;
The SoftwareFramebuffer destructor un-sets is back-pointer to the
RenderWidgetHost and view, so any callbacks that come in during tear-down will
not reach the (now-dead) view.

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1497:
software_framebuffer_->SwapToNewFrame(
This is a functional change, in that we have fused FrameMemoryManager::AddFrame
and SwapToNewFrame. If this frame is to a backgrounded tab, it may be instantly
evicted.

In that case, this code acts more like the code in ShouldSkipFrame instead of
proceeding through, then deleting the frame at the end of the function.

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/software_framebuffer.h (right):

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/software_framebuffer.h:52: 
This part of the interface seems a bit sloppy. I included these functions as-is
from Aura to separate functional and aesthetic changes (this is largely
aesthetic).

I'd like to look into what can be done to clean this up in a follow-on change.

[slavi, 2013-10-04 22:12:04]

lgtm

[ccameron, 2013-10-07 17:53:32]

Adding sail for the Mac specific parts, piman for content OWNER.

[ccameron, 2013-10-07 18:16:21]

Switching to kbr for RWHVMac parts.

[Ken Russell (switch to Gerrit), 2013-10-07 18:33:07]

Looks like good progress. LGTM

[piman, 2013-10-08 02:58:38]

Can we have unit tests or browser tests for the code that you're changing?

Other than that, it seems reasonable, couple of q's. Also, I'd like jbauman@ to
take a look as well.

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1497:
software_framebuffer_->SwapToNewFrame(
On 2013/10/03 21:46:00, ccameron1 wrote:
> This is a functional change, in that we have fused
FrameMemoryManager::AddFrame
> and SwapToNewFrame. If this frame is to a backgrounded tab, it may be
instantly
> evicted.
> 
> In that case, this code acts more like the code in ShouldSkipFrame instead of
> proceeding through, then deleting the frame at the end of the function.

Can the eviction cause re-entrancy? Is that ok?
Can I haz tests?

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/software_framebuffer.cc (right):

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/software_framebuffer.cc:149:
RenderWidgetHostImpl::SendSwapCompositorFrameAck(
I would really prefer if the message sending logic stayed in the
RenderWidgetHostViewAura, and SoftwareFramebuffer wouldn't skip it and talked to
the RWHImpl directly. You could easily have a
SendSoftwareFrameAck(software_frame_id, output_surface_id) on
SoftwareFramebufferClient, as well as a similar SendReclaimSoftwareFrames. You
can keep the frame tracking logic here.
This makes it a lot clearer in the RWHVAura/Mac what's going on, and has a clear
separation of responsibilites: the SoftwareFramebuffer manages frames coming in
and out, and manages memory with the FrameMemoryManager, the RWHV* take care of
feeding the software frame to the compositor, and talking to the renderer.
As it is with this CL, the latter is distributed in several files, and hard to
follow.

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/software_framebuffer.h (right):

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/software_framebuffer.h:27: class
SoftwareFramebuffer
Naming nitpicking... Just trying to convey the right info in here and in the
callers. This class doesn't represent a single frame buffer, but more of a
helper class that manages the software frame buffers. Maybe
SoftwareFrameManager?

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/software_framebuffer.h:28: : public
base::SupportsWeakPtr<SoftwareFramebuffer>,
We try to avoid SupportsWeakPtr if we can, replacing with a WeakPtrFactory
member at the end of the class (which is more correct most of the time, because
of destruction order).

[ccameron, 2013-10-22 07:15:54]

I've updated the patch to cut along the boundary suggested in the review
feedback.

The common software framebuffer code is now in software_frame_manager.h/cc. Of
note is that the separation will let me add unit tests for the
SoftwareFrameManager(Client) and SoftwareFrameMemoryManager (eviction)
functions.

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1497:
software_framebuffer_->SwapToNewFrame(
On 2013/10/08 02:58:38, piman wrote:
> On 2013/10/03 21:46:00, ccameron1 wrote:
> > This is a functional change, in that we have fused
> FrameMemoryManager::AddFrame
> > and SwapToNewFrame. If this frame is to a backgrounded tab, it may be
> instantly
> > evicted.
> > 
> > In that case, this code acts more like the code in ShouldSkipFrame instead
of
> > proceeding through, then deleting the frame at the end of the function.
> 
> Can the eviction cause re-entrancy? Is that ok?
> Can I haz tests?

The potentialy re-entrancy seemed too iffy... changed this back to two steps.

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/software_framebuffer.cc (right):

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/software_framebuffer.cc:149:
RenderWidgetHostImpl::SendSwapCompositorFrameAck(
On 2013/10/08 02:58:38, piman wrote:
> I would really prefer if the message sending logic stayed in the
> RenderWidgetHostViewAura, and SoftwareFramebuffer wouldn't skip it and talked
to
> the RWHImpl directly. You could easily have a
> SendSoftwareFrameAck(software_frame_id, output_surface_id) on
> SoftwareFramebufferClient

I did essentially this in the new version. The callbacks ended up being
SoftwareFrameWasFreed and CurrentSoftwareFrameWasEvicted (the latter does some
extra cleanup for Aura, but not Mac).

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/software_framebuffer.h (right):

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/software_framebuffer.h:27: class
SoftwareFramebuffer
On 2013/10/08 02:58:38, piman wrote:
> Naming nitpicking... Just trying to convey the right info in here and in the
> callers. This class doesn't represent a single frame buffer, but more of a
> helper class that manages the software frame buffers. Maybe
> SoftwareFrameManager?

I changed this from MemoryHolder/RWHVAura/FrameMemoryManager to
SoftwareFrame/SoftwareFrameManager/SoftwareFrameMemoryManager.

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/software_framebuffer.h:28: : public
base::SupportsWeakPtr<SoftwareFramebuffer>,
On 2013/10/08 02:58:38, piman wrote:
> We try to avoid SupportsWeakPtr if we can, replacing with a WeakPtrFactory
> member at the end of the class (which is more correct most of the time,
because
> of destruction order).

Changed this to a WeakPtrFactory hanging off of the SoftwareFrameManagerClient
(== RWHVAura).

https://codereview.chromium.org/25942002/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/software_framebuffer.h:52: 
On 2013/10/03 21:46:00, ccameron1 wrote:
> This part of the interface seems a bit sloppy. I included these functions
as-is
> from Aura to separate functional and aesthetic changes (this is largely
> aesthetic).
> 
> I'd like to look into what can be done to clean this up in a follow-on change.

This is gone now...

[ccameron, 2013-10-22 09:38:03]

Oh, and...

On 2013/10/08 02:58:38, piman wrote:
> Can we have unit tests or browser tests for the code that you're changing?

The RWHVAura behavior should be the same as before now (no more tricky
reentrancy stuff, etc).

I'm working on another CL which adds unit tests for the now-separated-out
SoftwareFrameManager to verify the eviction behavior.

[piman, 2013-10-22 22:32:47]

Mostly nits, but one thing - I don't see SoftwareFrameWasFreed being called -
should it be called from DiscardCurrentFrame?

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_mac.mm:1680:
render_widget_host_->GetProcess()->ReceivedBadMessage();
That's quite extreme :) Do we really need to kill the renderer, or can we just
ignore the message?

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
File content/browser/renderer_host/software_frame_manager.cc (right):

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.cc:91:
MaxNumberOfSavedFrames()) {
nit: MaxNumberOfSavedFrames() is a constant, right? On linux at least, it opens
and parses a file in /proc - I don't think we mean to do that every frame. Can
we cache this value at initialization?

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
File content/browser/renderer_host/software_frame_manager.h (right):

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.h:26: // Called immediately
after CurrentSoftwareFrameWasFreed if the frame
nit: "CurrentSoftwareFrameWasFreed" doesn't exist, I assume you meant
"SoftwareFrameWasFreed".

That said, I don't see SoftwareFrameWasFreed being called at all in the
implementation - did I miss it?

Lastly, is it worth having 2 methods? Maybe we can pass a bool in
SoftwareFrameWasFreed? I could imagine the implementation wanting to do
something in SoftwareFrameWasFreed based on whether it was an eviction or not,
but as it is, it has to wait to see whether CurrentSoftwareFrameWasEvicted is
called or not, which is awkward.

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.h:33:
SoftwareFrameManager(base::WeakPtr<SoftwareFrameManagerClient> client);
nit: explicit

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.h:36: // Swap to a new
frame from shared memory. This frame is guaranteed to
nit: Swaps

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.h:55: // be evicted.
nit: maybe make it private and make SoftwareFrameMemoryManager a friend

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.h:62:
scoped_refptr<SoftwareFrame> current_frame_;
nit: DISALLOW_COPY_AND_ASSIGN

[ccameron, 2013-10-23 01:05:57]

Thanks!!

I cleared up the "evict" versus "free" behavior (see comments below). And I
finished up the unit test CL, so I threw that in. The test should help clarify
some of the evict v free behavior.

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_mac.mm:1680:
render_widget_host_->GetProcess()->ReceivedBadMessage();
On 2013/10/22 22:32:48, piman wrote:
> That's quite extreme :) Do we really need to kill the renderer, or can we just
> ignore the message?

I'd rather blow up the renderer than silently accept bad input. If a renderer
starts misbehaving, I don't want to miss it.

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
File content/browser/renderer_host/software_frame_manager.cc (right):

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.cc:91:
MaxNumberOfSavedFrames()) {
On 2013/10/22 22:32:48, piman wrote:
> nit: MaxNumberOfSavedFrames() is a constant, right? On linux at least, it
opens
> and parses a file in /proc - I don't think we mean to do that every frame. Can
> we cache this value at initialization?

Done.

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
File content/browser/renderer_host/software_frame_manager.h (right):

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.h:26: // Called immediately
after CurrentSoftwareFrameWasFreed if the frame
On 2013/10/22 22:32:48, piman wrote:
> nit: "CurrentSoftwareFrameWasFreed" doesn't exist, I assume you meant
> "SoftwareFrameWasFreed".

Done

> That said, I don't see SoftwareFrameWasFreed being called at all in the
> implementation - did I miss it?

It is called in the dtor for SoftwareFrame -- the call is made when the memory
for a frame is actually freed.

> Lastly, is it worth having 2 methods? Maybe we can pass a bool in
> SoftwareFrameWasFreed? I could imagine the implementation wanting to do
> something in SoftwareFrameWasFreed based on whether it was an eviction or not,
> but as it is, it has to wait to see whether CurrentSoftwareFrameWasEvicted is
> called or not, which is awkward.

There are two separate calls here here.

When we hit memory pressure, we go tell the Client "hey, if you're holding on to
any references to this frame, drop them, so we can free up its memory". The
references, in practice, are textures in the browser compositor. The mechanism
by which this is done is the mailbox destructor callback, ReleaseMailbox, which
has the scoped_refptr to the SoftwareFrame bound in it.

The second call is the one that we make when the memory for the frame is
actually freed. When that call is made, we go tell the Renderer "hey, we're done
with this shared memory now" -- you can free it or scribble on it or whatever.
When we swap frames, this is made when the last reference to the frame being
used is removed. During eviction, this call is made only once all references to
the frame are killed off.

I renamed the "evict" method to "release your references to the software frame"
to disambiguate a bit better, and added comments.

I added a test (EvictVersusFree) to demonstrate this, and to verify that the
callback holds on to the frame until it is ditched (even if there are multiple
intervening swaps).

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.h:33:
SoftwareFrameManager(base::WeakPtr<SoftwareFrameManagerClient> client);
On 2013/10/22 22:32:48, piman wrote:
> nit: explicit

Done.

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.h:36: // Swap to a new
frame from shared memory. This frame is guaranteed to
On 2013/10/22 22:32:48, piman wrote:
> nit: Swaps

Done.

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.h:55: // be evicted.
On 2013/10/22 22:32:48, piman wrote:
> nit: maybe make it private and make SoftwareFrameMemoryManager a friend

Done.

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.h:62:
scoped_refptr<SoftwareFrame> current_frame_;
On 2013/10/22 22:32:48, piman wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

[piman, 2013-10-23 23:41:39]

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_mac.mm:1680:
render_widget_host_->GetProcess()->ReceivedBadMessage();
On 2013/10/23 01:05:58, ccameron1 wrote:
> On 2013/10/22 22:32:48, piman wrote:
> > That's quite extreme :) Do we really need to kill the renderer, or can we
just
> > ignore the message?
> 
> I'd rather blow up the renderer than silently accept bad input. If a renderer
> starts misbehaving, I don't want to miss it.

So, you're not accepting input, you're just dropping the frame.
My main concern is that this generates a crash, that is virtually impossible to
attribute. We usually only do this for straight security issues (e.g. renderer
sent a shm whose size doesn't match the explicit size).

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
File content/browser/renderer_host/software_frame_manager.h (right):

https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/software_frame_manager.h:26: // Called immediately
after CurrentSoftwareFrameWasFreed if the frame
On 2013/10/23 01:05:58, ccameron1 wrote:
> On 2013/10/22 22:32:48, piman wrote:
> > nit: "CurrentSoftwareFrameWasFreed" doesn't exist, I assume you meant
> > "SoftwareFrameWasFreed".
> 
> Done
> 
> > That said, I don't see SoftwareFrameWasFreed being called at all in the
> > implementation - did I miss it?
> 
> It is called in the dtor for SoftwareFrame -- the call is made when the memory
> for a frame is actually freed.

Ok, sorry, I had missed that.

> 
> > Lastly, is it worth having 2 methods? Maybe we can pass a bool in
> > SoftwareFrameWasFreed? I could imagine the implementation wanting to do
> > something in SoftwareFrameWasFreed based on whether it was an eviction or
not,
> > but as it is, it has to wait to see whether CurrentSoftwareFrameWasEvicted
is
> > called or not, which is awkward.
> 
> There are two separate calls here here.
> 
> When we hit memory pressure, we go tell the Client "hey, if you're holding on
to
> any references to this frame, drop them, so we can free up its memory". The
> references, in practice, are textures in the browser compositor. The mechanism
> by which this is done is the mailbox destructor callback, ReleaseMailbox,
which
> has the scoped_refptr to the SoftwareFrame bound in it.
> 
> The second call is the one that we make when the memory for the frame is
> actually freed. When that call is made, we go tell the Renderer "hey, we're
done
> with this shared memory now" -- you can free it or scribble on it or whatever.
> When we swap frames, this is made when the last reference to the frame being
> used is removed. During eviction, this call is made only once all references
to
> the frame are killed off.
> 
> I renamed the "evict" method to "release your references to the software
frame"
> to disambiguate a bit better, and added comments.
> 
> I added a test (EvictVersusFree) to demonstrate this, and to verify that the
> callback holds on to the frame until it is ditched (even if there are multiple
> intervening swaps).

Ok, thanks for the explanation. The previous documentation suggested we call
SoftwareFrameWasFreed first, and then CurrentSoftwareFrameWasEvicted. The new
documentation is clearer.

https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer...
File content/browser/renderer_host/software_frame_manager_unittest.cc (right):

https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer...
content/browser/renderer_host/software_frame_manager_unittest.cc:18: }
Ok, so, rather than duplicate the logic, which is fragile, can we make
max_number_of_saved_frames a member of SoftwareFrameMemoryManager, set at
initialization, that we can query?

https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer...
content/browser/renderer_host/software_frame_manager_unittest.cc:50:
std::vector<std::pair<uint32,unsigned> > freed_frames_;
nit: private + accessors

https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer...
content/browser/renderer_host/software_frame_manager_unittest.cc:55:
weak_ptr_factory_;
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer...
content/browser/renderer_host/software_frame_manager_unittest.cc:70:
std::vector<FakeSoftwareFrameManagerClient*> clients_;
nit: private+accessors, DISALLOW_COPY_AND_ASSIGN

[piman, 2013-10-24 01:00:33]

On Wed, Oct 23, 2013 at 4:41 PM, <piman@chromium.org> wrote:

>
> https://codereview.chromium.**org/25942002/diff/84001/**
>
content/browser/renderer_host/**render_widget_host_view_mac.mm<https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_host/render_widget_host_view_mac.mm>
> File
content/browser/renderer_host/**render_widget_host_view_mac.mm<http://render_widget_host_view_mac.mm>
> (right):
>
> https://codereview.chromium.**org/25942002/diff/84001/**
> content/browser/renderer_host/**render_widget_host_view_mac.**
>
mm#newcode1680<https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_host/render_widget_host_view_mac.mm#newcode1680>
>
content/browser/renderer_host/**render_widget_host_view_mac.**mm:1680<http://render_widget_host_view_mac.mm:1680>
> :
> render_widget_host_->**GetProcess()->**ReceivedBadMessage();
> On 2013/10/23 01:05:58, ccameron1 wrote:
>
>> On 2013/10/22 22:32:48, piman wrote:
>> > That's quite extreme :) Do we really need to kill the renderer, or
>>
> can we just
>
>> > ignore the message?
>>
>
>  I'd rather blow up the renderer than silently accept bad input. If a
>>
> renderer
>
>> starts misbehaving, I don't want to miss it.
>>
>
> So, you're not accepting input, you're just dropping the frame.
> My main concern is that this generates a crash, that is virtually
> impossible to attribute. We usually only do this for straight security
> issues (e.g. renderer sent a shm whose size doesn't match the explicit
> size).


FYI - https://codereview.chromium.org/35883006/
If you leave it as is, you may want to add some UMA for it.


>
>
> https://codereview.chromium.**org/25942002/diff/84001/**
>
content/browser/renderer_host/**software_frame_manager.h<https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_host/software_frame_manager.h>
> File content/browser/renderer_host/**software_frame_manager.h (right):
>
> https://codereview.chromium.**org/25942002/diff/84001/**
>
content/browser/renderer_host/**software_frame_manager.h#**newcode26<https://codereview.chromium.org/25942002/diff/84001/content/browser/renderer_host/software_frame_manager.h#newcode26>
> content/browser/renderer_host/**software_frame_manager.h:26: // Called
> immediately after CurrentSoftwareFrameWasFreed if the frame
> On 2013/10/23 01:05:58, ccameron1 wrote:
>
>> On 2013/10/22 22:32:48, piman wrote:
>> > nit: "CurrentSoftwareFrameWasFreed" doesn't exist, I assume you
>>
> meant
>
>> > "SoftwareFrameWasFreed".
>>
>
>  Done
>>
>
>  > That said, I don't see SoftwareFrameWasFreed being called at all in
>>
> the
>
>> > implementation - did I miss it?
>>
>
>  It is called in the dtor for SoftwareFrame -- the call is made when
>>
> the memory
>
>> for a frame is actually freed.
>>
>
> Ok, sorry, I had missed that.
>
>
>
>  > Lastly, is it worth having 2 methods? Maybe we can pass a bool in
>> > SoftwareFrameWasFreed? I could imagine the implementation wanting to
>>
> do
>
>> > something in SoftwareFrameWasFreed based on whether it was an
>>
> eviction or not,
>
>> > but as it is, it has to wait to see whether
>>
> CurrentSoftwareFrameWasEvicted is
>
>> > called or not, which is awkward.
>>
>
>  There are two separate calls here here.
>>
>
>  When we hit memory pressure, we go tell the Client "hey, if you're
>>
> holding on to
>
>> any references to this frame, drop them, so we can free up its
>>
> memory". The
>
>> references, in practice, are textures in the browser compositor. The
>>
> mechanism
>
>> by which this is done is the mailbox destructor callback,
>>
> ReleaseMailbox, which
>
>> has the scoped_refptr to the SoftwareFrame bound in it.
>>
>
>  The second call is the one that we make when the memory for the frame
>>
> is
>
>> actually freed. When that call is made, we go tell the Renderer "hey,
>>
> we're done
>
>> with this shared memory now" -- you can free it or scribble on it or
>>
> whatever.
>
>> When we swap frames, this is made when the last reference to the frame
>>
> being
>
>> used is removed. During eviction, this call is made only once all
>>
> references to
>
>> the frame are killed off.
>>
>
>  I renamed the "evict" method to "release your references to the
>>
> software frame"
>
>> to disambiguate a bit better, and added comments.
>>
>
>  I added a test (EvictVersusFree) to demonstrate this, and to verify
>>
> that the
>
>> callback holds on to the frame until it is ditched (even if there are
>>
> multiple
>
>> intervening swaps).
>>
>
> Ok, thanks for the explanation. The previous documentation suggested we
> call SoftwareFrameWasFreed first, and then
> CurrentSoftwareFrameWasEvicted**. The new documentation is clearer.
>
> https://codereview.chromium.**org/25942002/diff/394001/**
>
content/browser/renderer_host/**software_frame_manager_**unittest.cc<https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer_host/software_frame_manager_unittest.cc>
> File content/browser/renderer_host/**software_frame_manager_**unittest.cc
> (right):
>
> https://codereview.chromium.**org/25942002/diff/394001/**
> content/browser/renderer_host/**software_frame_manager_**
>
unittest.cc#newcode18<https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer_host/software_frame_manager_unittest.cc#newcode18>
> content/browser/renderer_host/**software_frame_manager_**unittest.cc:18: }
> Ok, so, rather than duplicate the logic, which is fragile, can we make
> max_number_of_saved_frames a member of SoftwareFrameMemoryManager, set
> at initialization, that we can query?
>
> https://codereview.chromium.**org/25942002/diff/394001/**
> content/browser/renderer_host/**software_frame_manager_**
>
unittest.cc#newcode50<https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer_host/software_frame_manager_unittest.cc#newcode50>
> content/browser/renderer_host/**software_frame_manager_**unittest.cc:50:
> std::vector<std::pair<uint32,**unsigned> > freed_frames_;
> nit: private + accessors
>
> https://codereview.chromium.**org/25942002/diff/394001/**
> content/browser/renderer_host/**software_frame_manager_**
>
unittest.cc#newcode55<https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer_host/software_frame_manager_unittest.cc#newcode55>
> content/browser/renderer_host/**software_frame_manager_**unittest.cc:55:
> weak_ptr_factory_;
> nit: DISALLOW_COPY_AND_ASSIGN
>
> https://codereview.chromium.**org/25942002/diff/394001/**
> content/browser/renderer_host/**software_frame_manager_**
>
unittest.cc#newcode70<https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer_host/software_frame_manager_unittest.cc#newcode70>
> content/browser/renderer_host/**software_frame_manager_**unittest.cc:70:
> std::vector<**FakeSoftwareFrameManagerClient***> clients_;
> nit: private+accessors, DISALLOW_COPY_AND_ASSIGN
>
>
https://codereview.chromium.**org/25942002/<https://codereview.chromium.org/2...
>

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

[ccameron, 2013-10-24 17:56:41]

Thanks!!

I added the UMA message and kept the ReceivedBadMessage.

https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer...
File content/browser/renderer_host/software_frame_manager_unittest.cc (right):

https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer...
content/browser/renderer_host/software_frame_manager_unittest.cc:18: }
On 2013/10/23 23:41:40, piman wrote:
> Ok, so, rather than duplicate the logic, which is fragile, can we make
> max_number_of_saved_frames a member of SoftwareFrameMemoryManager, set at
> initialization, that we can query?

I'd duplicated this in preference to putting SoftwareFrameMemoryManager in the
header, but that was probably a bit much. I've added it as a method on
SoftwareFrameMemoryManager.

https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer...
content/browser/renderer_host/software_frame_manager_unittest.cc:50:
std::vector<std::pair<uint32,unsigned> > freed_frames_;
On 2013/10/23 23:41:40, piman wrote:
> nit: private + accessors

Done.

https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer...
content/browser/renderer_host/software_frame_manager_unittest.cc:55:
weak_ptr_factory_;
On 2013/10/23 23:41:40, piman wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/25942002/diff/394001/content/browser/renderer...
content/browser/renderer_host/software_frame_manager_unittest.cc:70:
std::vector<FakeSoftwareFrameManagerClient*> clients_;
On 2013/10/23 23:41:40, piman wrote:
> nit: private+accessors, DISALLOW_COPY_AND_ASSIGN

Made this protected instead (since all tests access it more-or-less-directly).

[piman, 2013-10-24 20:33:16]

lgtm

[ccameron, 2013-10-24 21:16:47]

Thanks!!

[commit-bot: I haz the power, 2013-10-24 22:53:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/25942002/654001

[commit-bot: I haz the power, 2013-10-25 01:48:53]

Change committed as 230900