content: Cleanup GpuMemoryBufferImpl destruction.

content: Cleanup GpuMemoryBufferImpl destruction.

Use an internal callback instead of requiring the client to
pass a scoped_ptr<> to a "Delete" function.

BUG=416873
Committed: https://crrev.com/fdf0a9f6d6f29d68a0a55507f912bd82688303b1
Cr-Commit-Position: refs/heads/master@{#296411}

[reveman, 2014-09-24 01:15:45]

reveman@chromium.org changed reviewers:
+ alexst@chromium.org, piman@chromium.org

[reveman, 2014-09-24 01:15:46]

I think this is a nicer way to handle deletion of these buffers.

I'm hoping to give the compositor the ability to manage gfx:GpuMemoryBuffer
instances on its own. That will allow us to allocate, map, unmap and destroy
buffers on worker threads. Allowing the compositor to just delete the instance
it was given ownership of when done using it rather than pass the ownership back
through some API is cleaner and less vulnerable to bugs imo.

I think this will also allow us to handle deletion a bit more consistently
between the renderer and browser.

Please take a look.

[piman, 2014-09-24 03:19:11]

https://codereview.chromium.org/600693002/diff/1/content/common/gpu/client/gp...
File content/common/gpu/client/gpu_memory_buffer_impl.h (right):

https://codereview.chromium.org/600693002/diff/1/content/common/gpu/client/gp...
content/common/gpu/client/gpu_memory_buffer_impl.h:55: const
DestructionCallback& callback);
nit: I think it should be documented explicitly that the callback is called on
the thread that destroys the GpuMemoryBufferImpl, not the thread that called
CreateFromHandle

https://codereview.chromium.org/600693002/diff/1/content/common/gpu/client/gp...
File content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc (right):

https://codereview.chromium.org/600693002/diff/1/content/common/gpu/client/gp...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:23:
callback_.Run();
I think it would be nice if the callback_ was managed by the base
GpuMemoryBufferImpl.

The way I would do it is by replacing the
GpuMemoryBufferImpl*::InitializeFromHandle pattern by a static
GpuMemoryBufferImpl*::CreateForHandle, that would validate/map/lookup/anything,
fail if needed, and if successful, pass the looked up data and the callback to
the constructor (that would then delegate to the base GpuMemoryBufferImpl
constructor).

That way we know that callback_ is called consistently, we avoid doing
allocations in case of failure, and we don't have to worry about invalid states
in the GpuMemoryBufferImpl* implementation.

WDYT?

https://codereview.chromium.org/600693002/diff/1/content/renderer/render_thre...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/600693002/diff/1/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:1313:
make_scoped_refptr(thread_safe_sender()),
nit: you don't need make_scoped_refptr, Bind figures out whether to refcount or
not based on the presence of AddRef/Release, not on the fact that it's a refptr.
Similarly, you don't actually need a scoped_refptr in the callback.

https://codereview.chromium.org/600693002/diff/1/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:1315:
handle.global_id)).PassAs<gfx::GpuMemoryBuffer>();
Should DeletedGpuMemoryBuffer be called if CreateFromHandle fails?

[reveman, 2014-09-24 06:03:38]

PTAL

https://codereview.chromium.org/600693002/diff/1/content/common/gpu/client/gp...
File content/common/gpu/client/gpu_memory_buffer_impl.h (right):

https://codereview.chromium.org/600693002/diff/1/content/common/gpu/client/gp...
content/common/gpu/client/gpu_memory_buffer_impl.h:55: const
DestructionCallback& callback);
On 2014/09/24 03:19:10, piman (Very slow to review) wrote:
> nit: I think it should be documented explicitly that the callback is called on
> the thread that destroys the GpuMemoryBufferImpl, not the thread that called
> CreateFromHandle

Done.

https://codereview.chromium.org/600693002/diff/1/content/common/gpu/client/gp...
File content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc (right):

https://codereview.chromium.org/600693002/diff/1/content/common/gpu/client/gp...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:23:
callback_.Run();
On 2014/09/24 03:19:10, piman (Very slow to review) wrote:
> I think it would be nice if the callback_ was managed by the base
> GpuMemoryBufferImpl.
> 
> The way I would do it is by replacing the
> GpuMemoryBufferImpl*::InitializeFromHandle pattern by a static
> GpuMemoryBufferImpl*::CreateForHandle, that would
validate/map/lookup/anything,
> fail if needed, and if successful, pass the looked up data and the callback to
> the constructor (that would then delegate to the base GpuMemoryBufferImpl
> constructor).
> 
> That way we know that callback_ is called consistently, we avoid doing
> allocations in case of failure, and we don't have to worry about invalid
states
> in the GpuMemoryBufferImpl* implementation.
> 
> WDYT?

Yes, that's much better. Thanks for the suggestion!

https://codereview.chromium.org/600693002/diff/1/content/renderer/render_thre...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/600693002/diff/1/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:1313:
make_scoped_refptr(thread_safe_sender()),
On 2014/09/24 03:19:10, piman (Very slow to review) wrote:
> nit: you don't need make_scoped_refptr, Bind figures out whether to refcount
or
> not based on the presence of AddRef/Release, not on the fact that it's a
refptr.
> Similarly, you don't actually need a scoped_refptr in the callback.

hm, I think we do, unless I'm doing something completely wrong. Bind is using
the NeedsScopedRefptrButGetsRawPtr trait from
raw_scoped_refptr_mismatch_checker.h and the comment at the top of that file
seem to indicate that scoped_refptr needs to be used explicitly.

https://codereview.chromium.org/600693002/diff/1/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:1315:
handle.global_id)).PassAs<gfx::GpuMemoryBuffer>();
On 2014/09/24 03:19:10, piman (Very slow to review) wrote:
> Should DeletedGpuMemoryBuffer be called if CreateFromHandle fails?

Good catch. Done.

[piman, 2014-09-24 06:40:28]

lgtm

https://codereview.chromium.org/600693002/diff/1/content/renderer/render_thre...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/600693002/diff/1/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:1313:
make_scoped_refptr(thread_safe_sender()),
On 2014/09/24 06:03:38, reveman wrote:
> On 2014/09/24 03:19:10, piman (Very slow to review) wrote:
> > nit: you don't need make_scoped_refptr, Bind figures out whether to refcount
> or
> > not based on the presence of AddRef/Release, not on the fact that it's a
> refptr.
> > Similarly, you don't actually need a scoped_refptr in the callback.
> 
> hm, I think we do, unless I'm doing something completely wrong. Bind is using
> the NeedsScopedRefptrButGetsRawPtr trait from
> raw_scoped_refptr_mismatch_checker.h and the comment at the top of that file
> seem to indicate that scoped_refptr needs to be used explicitly.

Ah, sorry you're right. What I said is only true for the |this| parameter of a
method.

(you still don't need it on the DeletedGpuMemoryBuffer function, but either
way).

https://codereview.chromium.org/600693002/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc (left):

https://codereview.chromium.org/600693002/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:62: VLOG(1) <<
"IOSurface lookup failed";
nit: do you want to keep this?

[reveman, 2014-09-24 07:10:54]

https://codereview.chromium.org/600693002/diff/1/content/renderer/render_thre...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/600693002/diff/1/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:1313:
make_scoped_refptr(thread_safe_sender()),
On 2014/09/24 06:40:28, piman (Very slow to review) wrote:
> On 2014/09/24 06:03:38, reveman wrote:
> > On 2014/09/24 03:19:10, piman (Very slow to review) wrote:
> > > nit: you don't need make_scoped_refptr, Bind figures out whether to
refcount
> > or
> > > not based on the presence of AddRef/Release, not on the fact that it's a
> > refptr.
> > > Similarly, you don't actually need a scoped_refptr in the callback.
> > 
> > hm, I think we do, unless I'm doing something completely wrong. Bind is
using
> > the NeedsScopedRefptrButGetsRawPtr trait from
> > raw_scoped_refptr_mismatch_checker.h and the comment at the top of that file
> > seem to indicate that scoped_refptr needs to be used explicitly.
> 
> Ah, sorry you're right. What I said is only true for the |this| parameter of a
> method.
> 
> (you still don't need it on the DeletedGpuMemoryBuffer function, but either
> way).

Are you saying I could safely use base::Unretained with the ThreadSafeSender
instance or I don't need it at all?

https://codereview.chromium.org/600693002/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc (left):

https://codereview.chromium.org/600693002/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:62: VLOG(1) <<
"IOSurface lookup failed";
On 2014/09/24 06:40:28, piman (Very slow to review) wrote:
> nit: do you want to keep this?

I don't think this will ever fail unless there's something wrong with our code.
I'd keep it if it could fail because of some platform specific reasons that
we're not in control over and don't already have logging output for but that
doesn't seem to be the case.

[reveman, 2014-09-24 07:11:25]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-09-24 07:12:40]

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

[commit-bot: I haz the power, 2014-09-24 07:27:13]

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

[commit-bot: I haz the power, 2014-09-24 07:27:15]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[piman, 2014-09-24 07:58:32]

On Wed, Sep 24, 2014 at 12:10 AM, <reveman@chromium.org> wrote:

>
> https://codereview.chromium.org/600693002/diff/1/content/
> renderer/render_thread_impl.cc
> File content/renderer/render_thread_impl.cc (right):
>
> https://codereview.chromium.org/600693002/diff/1/content/
> renderer/render_thread_impl.cc#newcode1313
> content/renderer/render_thread_impl.cc:1313:
> make_scoped_refptr(thread_safe_sender()),
> On 2014/09/24 06:40:28, piman (Very slow to review) wrote:
>
>> On 2014/09/24 06:03:38, reveman wrote:
>> > On 2014/09/24 03:19:10, piman (Very slow to review) wrote:
>> > > nit: you don't need make_scoped_refptr, Bind figures out whether
>>
> to refcount
>
>> > or
>> > > not based on the presence of AddRef/Release, not on the fact that
>>
> it's a
>
>> > refptr.
>> > > Similarly, you don't actually need a scoped_refptr in the
>>
> callback.
>
>> >
>> > hm, I think we do, unless I'm doing something completely wrong. Bind
>>
> is using
>
>> > the NeedsScopedRefptrButGetsRawPtr trait from
>> > raw_scoped_refptr_mismatch_checker.h and the comment at the top of
>>
> that file
>
>> > seem to indicate that scoped_refptr needs to be used explicitly.
>>
>
>  Ah, sorry you're right. What I said is only true for the |this|
>>
> parameter of a
>
>> method.
>>
>
>  (you still don't need it on the DeletedGpuMemoryBuffer function, but
>>
> either
>
>> way).
>>
>
> Are you saying I could safely use base::Unretained with the
> ThreadSafeSender instance or I don't need it at all?


I'm saying the first parameter of DeletedGpuMemoryBuffer can be a
ThreadSafeSender* instead of a scoped_refptr<ThreadSafeSender>.

Antoine


>
> https://codereview.chromium.org/600693002/diff/20001/
> content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc
> File content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc
> (left):
>
> https://codereview.chromium.org/600693002/diff/20001/
> content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc#oldcode62
> content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:62:
> VLOG(1) << "IOSurface lookup failed";
> On 2014/09/24 06:40:28, piman (Very slow to review) wrote:
>
>> nit: do you want to keep this?
>>
>
> I don't think this will ever fail unless there's something wrong with
> our code. I'd keep it if it could fail because of some platform specific
> reasons that we're not in control over and don't already have logging
> output for but that doesn't seem to be the case.
>
> https://codereview.chromium.org/600693002/
>

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

[reveman, 2014-09-24 13:47:45]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-09-24 13:48:11]

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

[reveman, 2014-09-24 13:48:27]

On 2014/09/24 07:58:32, piman (Very slow to review) wrote:
> On Wed, Sep 24, 2014 at 12:10 AM, <mailto:reveman@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/600693002/diff/1/content/
> > renderer/render_thread_impl.cc
> > File content/renderer/render_thread_impl.cc (right):
> >
> > https://codereview.chromium.org/600693002/diff/1/content/
> > renderer/render_thread_impl.cc#newcode1313
> > content/renderer/render_thread_impl.cc:1313:
> > make_scoped_refptr(thread_safe_sender()),
> > On 2014/09/24 06:40:28, piman (Very slow to review) wrote:
> >
> >> On 2014/09/24 06:03:38, reveman wrote:
> >> > On 2014/09/24 03:19:10, piman (Very slow to review) wrote:
> >> > > nit: you don't need make_scoped_refptr, Bind figures out whether
> >>
> > to refcount
> >
> >> > or
> >> > > not based on the presence of AddRef/Release, not on the fact that
> >>
> > it's a
> >
> >> > refptr.
> >> > > Similarly, you don't actually need a scoped_refptr in the
> >>
> > callback.
> >
> >> >
> >> > hm, I think we do, unless I'm doing something completely wrong. Bind
> >>
> > is using
> >
> >> > the NeedsScopedRefptrButGetsRawPtr trait from
> >> > raw_scoped_refptr_mismatch_checker.h and the comment at the top of
> >>
> > that file
> >
> >> > seem to indicate that scoped_refptr needs to be used explicitly.
> >>
> >
> >  Ah, sorry you're right. What I said is only true for the |this|
> >>
> > parameter of a
> >
> >> method.
> >>
> >
> >  (you still don't need it on the DeletedGpuMemoryBuffer function, but
> >>
> > either
> >
> >> way).
> >>
> >
> > Are you saying I could safely use base::Unretained with the
> > ThreadSafeSender instance or I don't need it at all?
> 
> 
> I'm saying the first parameter of DeletedGpuMemoryBuffer can be a
> ThreadSafeSender* instead of a scoped_refptr<ThreadSafeSender>.

Ah, OK. Done.

> 
> Antoine
> 
> 
> >
> > https://codereview.chromium.org/600693002/diff/20001/
> > content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc
> > File content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc
> > (left):
> >
> > https://codereview.chromium.org/600693002/diff/20001/
> > content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc#oldcode62
> > content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:62:
> > VLOG(1) << "IOSurface lookup failed";
> > On 2014/09/24 06:40:28, piman (Very slow to review) wrote:
> >
> >> nit: do you want to keep this?
> >>
> >
> > I don't think this will ever fail unless there's something wrong with
> > our code. I'd keep it if it could fail because of some platform specific
> > reasons that we're not in control over and don't already have logging
> > output for but that doesn't seem to be the case.
> >
> > https://codereview.chromium.org/600693002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-09-24 14:47:57]

Committed patchset #6 (id:100001) as 12332e114e2d42b3c6b1244f09b6738b04cf1691

[commit-bot: I haz the power, 2014-09-24 14:48:30]

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