|
|
Created:
4 years, 10 months ago by emircan Modified:
4 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPost 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}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Using WeakPtr #
Total comments: 3
Patch Set 4 : Not using WeakPtr #
Messages
Total messages: 32 (14 generated)
dcastagna@chromium.org changed reviewers: + dcastagna@chromium.org
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?
Description was changed from ========== new stuff new stuff BUG= ========== to ========== Post GpuMemoryBufferVideoFramePool deletion on media thread BUG= ==========
PTAL
Patchset #1 (id:1) has been deleted
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.
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@chromium.org changed reviewers: + mcasas@chromium.org
mcasas@ for owners review pls.
Description was changed from ========== Post GpuMemoryBufferVideoFramePool deletion on media thread BUG= ========== to ========== Post GpuMemoryBufferVideoFramePool deletion on media thread BUG=585593 ==========
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 ?
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.
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?
Btw, this patch LGTM. I think it's better to land it ASAP, with or without weakptrs. :) Thank you again for fixing this!
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?
Patchset #3 (id:60001) has been deleted
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@chromium.org changed reviewers: + dalecurtis@chromium.org
Thanks dcastagna@. dalecurtis@chromium.org: Please OWNERS review changes in media/video/gpu_memory_buffer_video_frame_pool.*
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.
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.
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 ==========
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 ==========
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.
The CQ bit was checked by emircan@chromium.org
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")
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
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
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...
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/06bc927facb58cd87d4241d821d4b414bc239084 Cr-Commit-Position: refs/heads/master@{#374867} |