Post GpuMemoryBufferVideoFramePool deletion on media thread

Post GpuMemoryBufferVideoFramePool deletion on media thread

This fixes possible crash that might happen when
GpuMemoryBufferVideoFramePool is deleted before the task is posted.

BUG=585593
Committed: https://crrev.com/06bc927facb58cd87d4241d821d4b414bc239084
Cr-Commit-Position: refs/heads/master@{#374867}

[Daniele Castagna, 2016-02-10 21:20:57]

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

[Daniele Castagna, 2016-02-10 21:20:57]

https://codereview.chromium.org/1686123004/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1686123004/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_video_renderer_sink.cc:120:
base::Unretained(gpu_memory_buffer_pool_.get()), frame,
Can you add a comment here on why it's fine to use base::Unretained?

[emircan, 2016-02-10 21:21:09]

Description was changed from

==========
new stuff


new stuff

BUG=
==========

to

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

BUG=
==========

[emircan, 2016-02-10 21:27:03]

PTAL

[emircan, 2016-02-10 21:27:15]

Patchset #1 (id:1) has been deleted

[Daniele Castagna, 2016-02-10 21:33:47]

LGTM.

Can you add crbug.com/585901.

An brief explanation of why we want this in the CL description would be nice.

https://codereview.chromium.org/1686123004/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1686123004/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink.cc:116: //
|gpu_memory_buffer_pool_| would be deleted on |media_task_runner_| in dtor,
Maybe something like
|gpu_memory_buffer_pool_| deletion is going to be posted to |media_task_runner_|
...
base::Unretained() usage is fine since |gpu_memory_buffer_pool_| outlives the
task.

[emircan, 2016-02-10 23:04:55]

https://codereview.chromium.org/1686123004/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1686123004/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink.cc:116: //
|gpu_memory_buffer_pool_| would be deleted on |media_task_runner_| in dtor,
On 2016/02/10 21:33:46, Daniele Castagna wrote:
> Maybe something like
> |gpu_memory_buffer_pool_| deletion is going to be posted to
|media_task_runner_|
> ...
> base::Unretained() usage is fine since |gpu_memory_buffer_pool_| outlives the
> task.

Done.

[emircan, 2016-02-10 23:05:17]

emircan@chromium.org changed reviewers:
+ mcasas@chromium.org

[emircan, 2016-02-10 23:05:17]

mcasas@ for owners review pls.

[emircan, 2016-02-10 23:16:10]

Description was changed from

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

BUG=
==========

to

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

BUG=585593
==========

[mcasas, 2016-02-10 23:24:02]

https://codereview.chromium.org/1686123004/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1686123004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink.cc:117: // 
|media_task_runner_|. base::Unretained() usage is fine since
Unretained? I see |gpu_memory_buffer_pool_| is only used
in |media_task_runner_|, so why not adding to the pool a 
WeakPtrFactory, and in l.111, instead of testing for 
|!gpu_memory_buffer_pool_|, add a 

bool ShouldUseGpuMemoryBuffers() {
  return gpu_factories &&
gpu_factories->ShouldUseGpuMemoryBuffersForVideoFrames();
}

and use it in l.35, 43 and 111 ?

[emircan, 2016-02-11 00:13:19]

https://codereview.chromium.org/1686123004/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1686123004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink.cc:117: // 
|media_task_runner_|. base::Unretained() usage is fine since
On 2016/02/10 23:24:02, mcasas wrote:
> Unretained? I see |gpu_memory_buffer_pool_| is only used
> in |media_task_runner_|, so why not adding to the pool a 
> WeakPtrFactory, and in l.111, instead of testing for 
> |!gpu_memory_buffer_pool_|, add a 
> 
> bool ShouldUseGpuMemoryBuffers() {
>   return gpu_factories &&
> gpu_factories->ShouldUseGpuMemoryBuffersForVideoFrames();
> }
> 
> and use it in l.35, 43 and 111 ?

I added WeakPtrFactory and GetWeakPtr() calls in PS#3

However, it looks like you and dcastagna@ have very strong opinions about
Unretained vs. WeakPtr usage. I will leave this discussion up to you, and will
move on with the one you agree on.

Personally, I think Unretained and WeakPtr both provide the same functionality
in this case, as dtor deletes the scoped_ptr() and there wouldn't be any more
tasks. Also, I understand that WeakPtr&DeleteSoon has been used in Chrome as a
pattern. I think WeakPtr would have the edge in this discussion, just in case
GpuMemoryBufferVideoFramePool calls invalidatePtrs() in case of a loss.

[Daniele Castagna, 2016-02-11 00:45:39]

On 2016/02/11 at 00:13:19, emircan wrote:
>
https://codereview.chromium.org/1686123004/diff/40001/content/renderer/media/...
> File content/renderer/media/media_stream_video_renderer_sink.cc (right):
> 
>
https://codereview.chromium.org/1686123004/diff/40001/content/renderer/media/...
> content/renderer/media/media_stream_video_renderer_sink.cc:117: // 
|media_task_runner_|. base::Unretained() usage is fine since
> On 2016/02/10 23:24:02, mcasas wrote:
> > Unretained? I see |gpu_memory_buffer_pool_| is only used
> > in |media_task_runner_|, so why not adding to the pool a 
> > WeakPtrFactory, and in l.111, instead of testing for 
> > |!gpu_memory_buffer_pool_|, add a 
> > 
> > bool ShouldUseGpuMemoryBuffers() {
> >   return gpu_factories &&
> > gpu_factories->ShouldUseGpuMemoryBuffersForVideoFrames();
> > }
> > 
> > and use it in l.35, 43 and 111 ?
> 
> I added WeakPtrFactory and GetWeakPtr() calls in PS#3
> 
> However, it looks like you and dcastagna@ have very strong opinions about
Unretained vs. WeakPtr usage. I will leave this discussion up to you, and will
move on with the one you agree on.
> 

Ahah, I don't have a strong opinion, I just noticed that weakptrs are not needed
here. We're posting the deletesoon after all the tasks have been posted, so any
task referencing to weakptrs will be always referencing to valid weakptrs.
Weakptrs where is_valid is always true are not really useful, and are not
referencing something in any weak way.
Is that not the case?


> Personally, I think Unretained and WeakPtr both provide the same functionality
in this case, as dtor deletes the scoped_ptr() and there wouldn't be any more
tasks. Also, I understand that WeakPtr&DeleteSoon has been used in Chrome as a
pattern. I think WeakPtr would have the edge in this discussion, just in case
GpuMemoryBufferVideoFramePool calls invalidatePtrs() in case of a loss.

Yeah, they both provide the same functionality in the sense that adding logic
that doesn't change the behavior in any way provide the same functionality.
Where did you see the pattern? Are you sure that in those cases weakptrs are
always valid too?

[Daniele Castagna, 2016-02-11 00:49:32]

Btw, this patch LGTM. I think it's better to land it ASAP, with or without
weakptrs. :)

Thank you again for fixing this!

[Daniele Castagna, 2016-02-11 00:50:34]

On 2016/02/11 at 00:49:32, Daniele Castagna wrote:
> Btw, this patch LGTM. I think it's better to land it ASAP, with or without
weakptrs. :)
> 
> Thank you again for fixing this!

Actually, last thing, do you mind mentioning this fixes a crash in the CL
description?

[emircan, 2016-02-11 00:52:11]

Patchset #3 (id:60001) has been deleted

[emircan, 2016-02-11 00:55:37]

Description was changed from

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

BUG=585593
==========

to

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

This fixes possible crash that might happen when GpuMemoryBufferVideoFramePool
is deleted before the task is posted.

BUG=585593
==========

[emircan, 2016-02-11 00:56:41]

emircan@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[emircan, 2016-02-11 00:56:42]

Thanks dcastagna@.

dalecurtis@chromium.org: Please OWNERS review changes in
media/video/gpu_memory_buffer_video_frame_pool.*

[DaleCurtis, 2016-02-11 01:28:02]

https://codereview.chromium.org/1686123004/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1686123004/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink.cc:44:
media_task_runner_->DeleteSoon(FROM_HERE,
You don't need the GetWeakPtr() method, just DeleteSoon() on media task runner
since if this class is destructed it can't post any new tasks and all
outstanding ones will have completed. You already invalidate your own internal
weakptr so no calls should come back.

[emircan, 2016-02-11 01:34:49]

https://codereview.chromium.org/1686123004/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1686123004/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink.cc:44:
media_task_runner_->DeleteSoon(FROM_HERE,
On 2016/02/11 01:28:02, DaleCurtis wrote:
> You don't need the GetWeakPtr() method, just DeleteSoon() on media task runner
> since if this class is destructed it can't post any new tasks and all
> outstanding ones will have completed. You already invalidate your own internal
> weakptr so no calls should come back.

I agree that this would work as well. I am reverting back to PS#2.

[emircan, 2016-02-11 01:36:38]

Description was changed from

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

This fixes possible crash that might happen when GpuMemoryBufferVideoFramePool
is deleted before the task is posted.

BUG=585593
==========

to

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

This fixes possible crash that might happen when GpuMemoryBufferVideoFramePool 
is deleted before the task is posted.

BUG=585593
==========

[emircan, 2016-02-11 01:36:55]

Description was changed from

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

This fixes possible crash that might happen when GpuMemoryBufferVideoFramePool 
is deleted before the task is posted.

BUG=585593
==========

to

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

This fixes possible crash that might happen when 
GpuMemoryBufferVideoFramePool is deleted before the task is posted.

BUG=585593
==========

[DaleCurtis, 2016-02-11 01:44:33]

lgtm, to cover previous comments, you should never be exposing a GetWeakPtr() on
a class .. might as well make it extend SupportsWeakPtr (see caveats in
weak_ptr.h), but for a class which doesn't use that weak ptr internally (and in
fact has a refcountedthreadsafe internal impl) it doesn't make sense.

[emircan, 2016-02-11 06:22:08]

The CQ bit was checked by emircan@chromium.org

[emircan, 2016-02-11 06:22:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org
Link to the patchset: https://codereview.chromium.org/1686123004/#ps100001
(title: "Not using WeakPtr")

[commit-bot: I haz the power, 2016-02-11 06:22:34]

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

[commit-bot: I haz the power, 2016-02-11 06:27:55]

Description was changed from

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

This fixes possible crash that might happen when 
GpuMemoryBufferVideoFramePool is deleted before the task is posted.

BUG=585593
==========

to

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

This fixes possible crash that might happen when 
GpuMemoryBufferVideoFramePool is deleted before the task is posted.

BUG=585593
==========

[commit-bot: I haz the power, 2016-02-11 06:27:56]

Committed patchset #4 (id:100001)

[mcasas, 2016-02-11 15:40:41]

got that old nit.

https://codereview.chromium.org/1686123004/diff/80001/media/video/gpu_memory_...
File media/video/gpu_memory_buffer_video_frame_pool.h (right):

https://codereview.chromium.org/1686123004/diff/80001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.h:52:
base::WeakPtr<GpuMemoryBufferVideoFramePool> GetWeakPtr();
nit: for cases like this you can also make the class a
base::SupportsWeakPtr [1], although in this case it
might interact in funny ways with |pool_impl_| 
destruction.


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

[commit-bot: I haz the power, 2016-02-16 22:34:50]

Description was changed from

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

This fixes possible crash that might happen when 
GpuMemoryBufferVideoFramePool is deleted before the task is posted.

BUG=585593
==========

to

==========
Post GpuMemoryBufferVideoFramePool deletion on media thread

This fixes possible crash that might happen when
GpuMemoryBufferVideoFramePool is deleted before the task is posted.

BUG=585593

Committed: https://crrev.com/06bc927facb58cd87d4241d821d4b414bc239084
Cr-Commit-Position: refs/heads/master@{#374867}
==========

[commit-bot: I haz the power, 2016-02-16 22:34:51]

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