Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1823683002:
media: GpuMemoryBufferVideoFramePool uses BindToCurrentLoop() (Closed)
Descriptionmedia: GpuMemoryBufferVideoFramePool uses BindToCurrentLoop()
BUG=356871
Committed: https://crrev.com/244491cd38a1a60fefbc472360e9958775398805
Cr-Commit-Position: refs/heads/master@{#382452}
Patch Set 1 #
Total comments: 5
Patch Set 2 : fix unittests #
Total comments: 10
Patch Set 3 : change CL's goal #
Total comments: 2
Patch Set 4 : rollback unrelated change #
Messages
Total messages: 30 (12 generated)
dongseong.hwang@intel.com changed reviewers: + dalecurtis@chromium.org, dcastagna@chromium.org
dalecurtis, could you review?
The CQ bit was checked by dongseong.hwang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1823683002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823683002/1
Defer to Daniele in this case.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/1823683002/diff/1/media/video/gpu_memory_buff... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1823683002/diff/1/media/video/gpu_memory_buff... media/video/gpu_memory_buffer_video_frame_pool.cc:736: DCHECK(media_task_runner_->BelongsToCurrentThread()); GpuMemoryBufferVideoFramePool::PoolImpl::MailboxHoldersReleased could be called by any thread when the VideoFrame goes out of scope. It is necessary to post the task to the media_task_runner_ since all the access to resources_pool_ is done on that thread. With this patch you'll have a race. https://codereview.chromium.org/1823683002/diff/1/media/video/gpu_memory_buff... media/video/gpu_memory_buffer_video_frame_pool.cc:746: gpu_factories_->WaitSyncToken(release_sync_token); This is not necessary. MailboxHoldersReleased gets called after the parent compositor returns the resource to the child compositor. For VideoFrames the resources get returned only after they have been completely consumed (ReadLockFences have passed.)
The CQ bit was checked by dongseong.hwang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1823683002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823683002/20001
Daniele, I answered your concern. could you review again? https://codereview.chromium.org/1823683002/diff/1/media/video/gpu_memory_buff... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1823683002/diff/1/media/video/gpu_memory_buff... media/video/gpu_memory_buffer_video_frame_pool.cc:746: gpu_factories_->WaitSyncToken(release_sync_token); On 2016/03/21 17:18:15, Daniele Castagna wrote: > This is not necessary. MailboxHoldersReleased gets called after the parent > compositor returns the resource to the child compositor. > For VideoFrames the resources get returned only after they have been completely > consumed (ReadLockFences have passed.) It's CC's implementation detail. I think it's not good to rely on other module's implementation detail. In addition, WebRTC uses this class and I'm not sure if it's true in WebRTC also. https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:590: base::Bind(&PoolImpl::MailboxHoldersReleased, this, frame_resources)); On 2016/03/21 17:18:15, Daniele Castagna wrote: > GpuMemoryBufferVideoFramePool::PoolImpl::MailboxHoldersReleased could be called > by any thread when the VideoFrame goes out of scope. It is necessary to post the > task to the media_task_runner_ since all the access to resources_pool_ is done > on that thread. > > With this patch you'll have a race. No, this patch doesn't have a race, because I use BindToCurrentLoop(). BindToCurrentLoop() is helper function to avoid two redundant callback. :) https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right): https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:77: new base::ThreadTaskRunnerHandle(media_task_runner_)); To use BindToCurrentLoop, unittest has to define ThreadTaskRunnerHandle::Get()
https://codereview.chromium.org/1823683002/diff/1/media/video/gpu_memory_buff... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1823683002/diff/1/media/video/gpu_memory_buff... media/video/gpu_memory_buffer_video_frame_pool.cc:746: gpu_factories_->WaitSyncToken(release_sync_token); On 2016/03/21 at 18:31:22, dshwang wrote: > On 2016/03/21 17:18:15, Daniele Castagna wrote: > > This is not necessary. MailboxHoldersReleased gets called after the parent > > compositor returns the resource to the child compositor. > > For VideoFrames the resources get returned only after they have been completely > > consumed (ReadLockFences have passed.) > > It's CC's implementation detail. I think it's not good to rely on other module's implementation detail. In addition, WebRTC uses this class and I'm not sure if it's true in WebRTC also. I wouldn't say so, this code explicitly sets frame->metadata()>SetBoolean(VideoFrameMetadata::READ_LOCK_FENCES_ENABLED,true); That means it knows pretty well that when a videoframe comes back we can reuse its buffers. https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:143: // This could be called on the thread where |media_task_runner_| is current. nit: s/could/must. https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:590: base::Bind(&PoolImpl::MailboxHoldersReleased, this, frame_resources)); On 2016/03/21 at 18:31:23, dshwang wrote: > On 2016/03/21 17:18:15, Daniele Castagna wrote: > > GpuMemoryBufferVideoFramePool::PoolImpl::MailboxHoldersReleased could be called > > by any thread when the VideoFrame goes out of scope. It is necessary to post the > > task to the media_task_runner_ since all the access to resources_pool_ is done > > on that thread. > > > > With this patch you'll have a race. > > No, this patch doesn't have a race, because I use BindToCurrentLoop(). > BindToCurrentLoop() is helper function to avoid two redundant callback. :) Ooh, sorry, I missed the BindToCurrentLoop. This is definitely better than the helper function. Do you mind mentioning it in the CL description? https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:731: // Called when a VideoFrame is no longer references. nit: s/references/referenced. https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right): https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:77: new base::ThreadTaskRunnerHandle(media_task_runner_)); On 2016/03/21 at 18:31:23, dshwang wrote: > To use BindToCurrentLoop, unittest has to define ThreadTaskRunnerHandle::Get() You might want to add this comment here.
Description was changed from ========== media: GpuMemoryBufferVideoFramePool waits for release_sync_token If reusing the GpuMemoryBuffer without wait, video decoder can updates the GpuMemoryBuffer before the compositor composites the previous video frame in theory. BUG=356871 ========== to ========== media: GpuMemoryBufferVideoFramePool uses BindToCurrentLoop() BUG=356871 ==========
thanks for quick review. could you review again? https://codereview.chromium.org/1823683002/diff/1/media/video/gpu_memory_buff... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1823683002/diff/1/media/video/gpu_memory_buff... media/video/gpu_memory_buffer_video_frame_pool.cc:746: gpu_factories_->WaitSyncToken(release_sync_token); On 2016/03/21 18:45:19, Daniele Castagna wrote: > I wouldn't say so, this code explicitly sets > frame->metadata()>SetBoolean(VideoFrameMetadata::READ_LOCK_FENCES_ENABLED,true); > > That means it knows pretty well that when a videoframe comes back we can reuse > its buffers. I see. Let me drop this change. Now CL is for just BindToCurrentLoop. BTW, READ_LOCK_FENCES_ENABLED and release sync point are sort of duplicated.. https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:143: // This could be called on the thread where |media_task_runner_| is current. On 2016/03/21 18:45:19, Daniele Castagna wrote: > nit: s/could/must. Done. https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:590: base::Bind(&PoolImpl::MailboxHoldersReleased, this, frame_resources)); On 2016/03/21 18:45:19, Daniele Castagna wrote: > On 2016/03/21 at 18:31:23, dshwang wrote: > > On 2016/03/21 17:18:15, Daniele Castagna wrote: > > > GpuMemoryBufferVideoFramePool::PoolImpl::MailboxHoldersReleased could be > called > > > by any thread when the VideoFrame goes out of scope. It is necessary to post > the > > > task to the media_task_runner_ since all the access to resources_pool_ is > done > > > on that thread. > > > > > > With this patch you'll have a race. > > > > No, this patch doesn't have a race, because I use BindToCurrentLoop(). > > BindToCurrentLoop() is helper function to avoid two redundant callback. :) > > Ooh, sorry, I missed the BindToCurrentLoop. This is definitely better than the > helper function. > Do you mind mentioning it in the CL description? Done. https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:731: // Called when a VideoFrame is no longer references. On 2016/03/21 18:45:19, Daniele Castagna wrote: > nit: s/references/referenced. Done. https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right): https://codereview.chromium.org/1823683002/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:77: new base::ThreadTaskRunnerHandle(media_task_runner_)); On 2016/03/21 18:45:19, Daniele Castagna wrote: > On 2016/03/21 at 18:31:23, dshwang wrote: > > To use BindToCurrentLoop, unittest has to define ThreadTaskRunnerHandle::Get() > > You might want to add this comment here. Done.
The CQ bit was checked by dongseong.hwang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1823683002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823683002/40001
https://codereview.chromium.org/1823683002/diff/40001/media/renderers/mock_gp... File media/renderers/mock_gpu_video_accelerator_factories.h (left): https://codereview.chromium.org/1823683002/diff/40001/media/renderers/mock_gp... media/renderers/mock_gpu_video_accelerator_factories.h:44: MOCK_METHOD1(WaitSyncToken, void(const gpu::SyncToken& sync_token)); You can drop the changes to this file otherwise LGTM.
dalecurtis, Daniele's review is completed. Could you review again? https://codereview.chromium.org/1823683002/diff/40001/media/renderers/mock_gp... File media/renderers/mock_gpu_video_accelerator_factories.h (left): https://codereview.chromium.org/1823683002/diff/40001/media/renderers/mock_gp... media/renderers/mock_gpu_video_accelerator_factories.h:44: MOCK_METHOD1(WaitSyncToken, void(const gpu::SyncToken& sync_token)); On 2016/03/21 19:16:08, Daniele Castagna wrote: > You can drop the changes to this file otherwise LGTM. Oops, I missed. Done.
The CQ bit was checked by dongseong.hwang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1823683002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823683002/60001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dcastagna@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/1823683002/#ps60001 (title: "rollback unrelated change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1823683002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823683002/60001
Message was sent while issue was closed.
Description was changed from ========== media: GpuMemoryBufferVideoFramePool uses BindToCurrentLoop() BUG=356871 ========== to ========== media: GpuMemoryBufferVideoFramePool uses BindToCurrentLoop() BUG=356871 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== media: GpuMemoryBufferVideoFramePool uses BindToCurrentLoop() BUG=356871 ========== to ========== media: GpuMemoryBufferVideoFramePool uses BindToCurrentLoop() BUG=356871 Committed: https://crrev.com/244491cd38a1a60fefbc472360e9958775398805 Cr-Commit-Position: refs/heads/master@{#382452} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/244491cd38a1a60fefbc472360e9958775398805 Cr-Commit-Position: refs/heads/master@{#382452} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
