Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2472273002:
Move passing of WebRTC rendering frames from main thread to compositor thread (Closed)
|
Created:
4 years, 1 month ago by emircan Modified:
4 years, 1 month ago Reviewers:
qiangchen, Avi (use Gerrit), tommi (sloooow) - chröme, Wez, Lei Zhang, DaleCurtis, perkj_chrome CC:
avayvod+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, isheriff+watch_chromium.org, jam, jasonroberts+watch_google.com, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove passing of WebRTC rendering frames from main thread to compositor thread
When main render thread is busy with JS functions, WebRTC video playback stalls
and starts accumulating frames resulting in increased memory. It sometimes
recovers by playing the accumulated frames quickly, and sometimes causes out
of memory error.
This CL modifies the passing to video frames in rendering path such that main
thread is not used. We jump from IO thread directly to compositor thread and
avoid trampolining in between. Since lifetime and methods of these classes, such
as WebMediaPlayerMS, is still tied to main thread, I created inner classes which
are pinned to specific threads and carry forwarding tasks:
- FrameReceiver receives frames on IO and forwards to compositor.
- FrameDeliverer forwards frames on compositor.
BUG=652923
TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC
loopback.
I am still confused to how/where to add a proper browsertest. Running alert() on
a page requires manual user interaction.
Committed: https://crrev.com/99eb6bc119173a68cc695f5179c72cadf0b515da
Cr-Commit-Position: refs/heads/master@{#433356}
Patch Set 1 : #Patch Set 2 : Make |render_frame_suspended_| android specific. #
Total comments: 21
Patch Set 3 : perkj@ and qiangchen@ comments. #
Total comments: 18
Patch Set 4 : perkj@ comments. #
Total comments: 42
Patch Set 5 : dalecurtis@ comments. #Patch Set 6 : wez@ and dalecurtis@ comments. #
Total comments: 20
Patch Set 7 : wez@ comments. #
Total comments: 4
Patch Set 8 : dalecurtis@ comments. #
Total comments: 12
Patch Set 9 : perkj@ comments. #Patch Set 10 : wez@ nits. #
Total comments: 2
Messages
Total messages: 115 (83 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Moving off main thread ========== to ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining this. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiverOnIO receives frames on IO and forwards to compositor. - FrameDelivererOnCompositor forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running Js alert() in console during an AppRTC loopback. I am still confused to how/where to add a content browsertest. ==========
emircan@chromium.org changed reviewers: + mcasas@chromium.org, qiangchen@chromium.org, tommi@chromium.org
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining this. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiverOnIO receives frames on IO and forwards to compositor. - FrameDelivererOnCompositor forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running Js alert() in console during an AppRTC loopback. I am still confused to how/where to add a content browsertest. ========== to ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining this. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiverOnIO receives frames on IO and forwards to compositor. - FrameDelivererOnCompositor forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. ==========
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
emircan@chromium.org changed reviewers: + dalecurtis@chromium.org
mcasas@: PTAL at content/renderer/media/* changes. qiangchenC@: PTAL at content/renderer/media/webmediaplayer_ms.* changes. dalecurtis:PTAL at content/renderer/media/webmediaplayer_ms.* changes with focus on Android specific code. tommi@: PTAL at changes and add other reviewers as necessary.
Description was changed from ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining this. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiverOnIO receives frames on IO and forwards to compositor. - FrameDelivererOnCompositor forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. ========== to ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining in between. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiverOnIO receives frames on IO and forwards to compositor. - FrameDelivererOnCompositor forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. ==========
Description was changed from ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining in between. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiverOnIO receives frames on IO and forwards to compositor. - FrameDelivererOnCompositor forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. ========== to ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory error. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining in between. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiverOnIO receives frames on IO and forwards to compositor. - FrameDelivererOnCompositor forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. ==========
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
perkj@chromium.org changed reviewers: + perkj@chromium.org
Took a first look. There is much here that I dont know though. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:161: // Used for DCHECKs to ensure method calls executed in the correct thread. s/ calls are executed ... on the correct https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:293: void MediaStreamVideoRendererSink::OnVideoFrameForTesting( please remove. Is it possible to create a fake video track instead where you inject frames. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:131: // Used for DCHECKs to ensure method calls executed in the correct thread, /s on the https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms_compositor.h (right): https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_compositor.h:91: base::WeakPtr<WebMediaPlayerMSCompositor> GetWeakPtr(); So these weakptr's may only be dereferences on the compositor? That is not obvious from just looking at the header file. Does it have to be public?
Not finished yet. Just post my findings so far. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:91: main_task_runner_->PostTask( I am worrying that this might be an issue. Theoretically, it is possible that other compositor thread tasks run before OnFirstFrameReceived got called. Could there be an issue? Can you double checkthat? https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:133: base::ThreadChecker thread_checker_; nit: Can you rename it to compositor_thread_checker_? As it would make the code readability better.
some more comments. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:174: class MediaStreamVideoRendererSink::FrameReceiverOnIO { Is it possible to remove this class? I mean just implement OnVideoFrame in MediaStreamVideoRendererSink. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:597: video_rotation_ = video_rotation; Is it possible to eliminate video_rotation_? I found it is just used once below within this function.
Patchset #3 (id:140001) has been deleted
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:161: // Used for DCHECKs to ensure method calls executed in the correct thread. On 2016/11/09 15:27:13, perkj_chrome wrote: > s/ calls are executed ... on the correct Done. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:174: class MediaStreamVideoRendererSink::FrameReceiverOnIO { On 2016/11/09 23:23:48, qiangchen wrote: > Is it possible to remove this class? I mean just implement OnVideoFrame in > MediaStreamVideoRendererSink. I think that would make the ownership and threading less clear. We would provide a callback to MediaStreamVideoRendererSink::OnVideoFrame when calling MediaStreamVideoSink::ConnectToTrack then. MediaStreamVideoRendererSink would receive frames on IO thread. If we were to provide MediaStreamVideoRendererSink as a weak_ptr, we would have to make sure that the weak pointers are invalidated on IO thread before dtor is run. Since, dtor is run on main thread, that would require synchronization. Pinning MediaStreamVideoRendererSink to only one thread makes it much easier to maintain. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:293: void MediaStreamVideoRendererSink::OnVideoFrameForTesting( On 2016/11/09 15:27:13, perkj_chrome wrote: > please remove. Is it possible to create a fake video track instead where you > inject frames. This is how test have been injecting the frames before my CL as well, see MediaStreamVideoRendererSinkTest::OnVideoFrame. But I agree with you that this is not ideal. I am removing this and injecting frames via MockMediaStreamVideoSource. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:91: main_task_runner_->PostTask( On 2016/11/09 19:44:39, qiangchen wrote: > I am worrying that this might be an issue. Theoretically, it is possible that > other compositor thread tasks run before OnFirstFrameReceived got called. Could > there be an issue? Can you double checkthat? Yes, compositor tasks may run on before OnFirstFrameReceived() is completed. Unfortunately, we do not know rotation/opacity information until first frame is received, so we do not have enough info for initialization. I can add an AsyncWaiter to make sure OnFirstFrameReceived() completes before moving on, but again we would keep waiting when main thread is busy. WebMediaPlayerImpl does this based on metadata[0][1] but again posts a task on main. I am not sure about the synchronization there. dalecurtis@ might know more about it. [0] https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=14788666... [1] https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=1... https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:131: // Used for DCHECKs to ensure method calls executed in the correct thread, On 2016/11/09 15:27:13, perkj_chrome wrote: > /s on the Done. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:133: base::ThreadChecker thread_checker_; On 2016/11/09 19:44:38, qiangchen wrote: > nit: Can you rename it to compositor_thread_checker_? As it would make the code > readability better. Done. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:597: video_rotation_ = video_rotation; On 2016/11/09 23:23:48, qiangchen wrote: > Is it possible to eliminate video_rotation_? I found it is just used once below > within this function. It is also used in WebMediaPlayerMS::naturalSize() and WebMediaPlayerMS::paint(). https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms_compositor.h (right): https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_compositor.h:91: base::WeakPtr<WebMediaPlayerMSCompositor> GetWeakPtr(); On 2016/11/09 15:27:13, perkj_chrome wrote: > So these weakptr's may only be dereferences on the compositor? That is not > obvious from just looking at the header file. Does it have to be public? Yes, they would be used to post tasks on compositor thread and invalidated on compositor thread via ~WebMediaPlayerMSCompositor. It needs to be public because WebMediaPlayerMS needs to access it.
Mostly nits. But I wondering about all the thread hops. Can you remove one thread hop to cc thread when GPU buffers need to be fetched and written? https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:293: void MediaStreamVideoRendererSink::OnVideoFrameForTesting( On 2016/11/11 22:27:19, emircan wrote: > On 2016/11/09 15:27:13, perkj_chrome wrote: > > please remove. Is it possible to create a fake video track instead where you > > inject frames. > > This is how test have been injecting the frames before my CL as well, see > MediaStreamVideoRendererSinkTest::OnVideoFrame. > But I agree with you that this is not ideal. I am removing this and injecting > frames via MockMediaStreamVideoSource. thanks https://codereview.chromium.org/2472273002/diff/160001/content/public/rendere... File content/public/renderer/media_stream_renderer_factory.h (right): https://codereview.chromium.org/2472273002/diff/160001/content/public/rendere... content/public/renderer/media_stream_renderer_factory.h:39: virtual scoped_refptr<MediaStreamVideoRenderer> GetVideoRenderer( Can you add documentation on what all these task runners are for? media_task_runner seems to be for handling GPU buffers? https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:87: DCHECK(frame); should you check the state here too? This frame can now be rendered after Stop() since OnVideoFrame is async.- To answer my own question-No, it looks like RenderSingnalingFrame renderers a frame in state == Stopped. Maybe add a comment about that. https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:102: // It also ensure that the renderer don't hold a reference to a real video /s doesn't https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:162: // Used for DCHECKs to ensure method calls executed on the correct thread. /s calls are https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:171: // compositor thread by posting a task on |deliverer_|'s OnFrame() method. /s to the compositor thread https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:174: // be destructed on IO thread. /s the IO https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:190: compositor_task_runner_->PostTask( What is the most common case? with or without GPU buffers?, ie- can we avoid the extra thread hop from IO->compositor->media_task_runner->compositor here? and first jump to the media_task_runner? It looks like media::GpuMemoryBufferVideoFramePool::MaybeCreateHardwareFrame do yet another thread hop to worker_task_runner. That is a lot of thread hops.... https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:110: base::AutoLock auto_lock(render_frame_suspended_lock_); can't this be posted to the compositor thread instead of locking? https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.h (right): https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:50: class RenderFrameObserver; This class does not seem to be used in this header file..
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2472273002/diff/160001/content/public/rendere... File content/public/renderer/media_stream_renderer_factory.h (right): https://codereview.chromium.org/2472273002/diff/160001/content/public/rendere... content/public/renderer/media_stream_renderer_factory.h:39: virtual scoped_refptr<MediaStreamVideoRenderer> GetVideoRenderer( On 2016/11/14 15:19:55, perkj_chrome wrote: > Can you add documentation on what all these task runners are for? > media_task_runner seems to be for handling GPU buffers? Done. https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:87: DCHECK(frame); On 2016/11/14 15:19:55, perkj_chrome wrote: > should you check the state here too? This frame can now be rendered after Stop() > since OnVideoFrame is async.- To answer my own question-No, it looks like > RenderSingnalingFrame renderers a frame in state == Stopped. Maybe add a comment > about that. Actually, we should check it. Thanks for catching this. It looks like I modified this behavior in the earlier CL below. I refactored it so it works like before. https://codereview.chromium.org/1358883003/diff/330001/content/renderer/media... https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:102: // It also ensure that the renderer don't hold a reference to a real video On 2016/11/14 15:19:55, perkj_chrome wrote: > /s doesn't Done. https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:162: // Used for DCHECKs to ensure method calls executed on the correct thread. On 2016/11/14 15:19:55, perkj_chrome wrote: > /s calls are Done. https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:171: // compositor thread by posting a task on |deliverer_|'s OnFrame() method. On 2016/11/14 15:19:55, perkj_chrome wrote: > /s to the compositor thread Done. https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:174: // be destructed on IO thread. On 2016/11/14 15:19:55, perkj_chrome wrote: > /s the IO Done. https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:190: compositor_task_runner_->PostTask( On 2016/11/14 15:19:55, perkj_chrome wrote: > What is the most common case? with or without GPU buffers?, ie- can we avoid the > extra thread hop from IO->compositor->media_task_runner->compositor here? and > first jump to the media_task_runner? > > It looks like media::GpuMemoryBufferVideoFramePool::MaybeCreateHardwareFrame do > yet another thread hop to worker_task_runner. That is a lot of thread hops.... Right now GpuMemoryBufferVideoFramePool is disabled by default, see crbug.com/652068. But AFAIK future plans are that it would be the only way to render, such that VideoResourceUpdater would expect only GMB backed frames. Here, we still should check the state before posting the task to GMBVFP so we don't have an unnecessary texture upload when paused. That would mean we would move state interactions on IO thread, but I think we should avoid using IO thread so that it is more responsive. I thought about this trade-off earlier and decided to do the thread hop instead. WDYT? https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:110: base::AutoLock auto_lock(render_frame_suspended_lock_); On 2016/11/14 15:19:55, perkj_chrome wrote: > can't this be posted to the compositor thread instead of locking? It can and I switched to posting task. Initially, I thought |render_frame_suspended_| needs to be very responsive, but in second look it doesn't provide much benefit. I am still confused about why that bool is used instead of Pause()/Resume(). https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.h (right): https://codereview.chromium.org/2472273002/diff/160001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:50: class RenderFrameObserver; On 2016/11/14 15:19:55, perkj_chrome wrote: > This class does not seem to be used in this header file.. Leftover from rebase. Removed.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:174: class MediaStreamVideoRendererSink::FrameReceiverOnIO { On 2016/11/11 22:27:19, emircan wrote: > On 2016/11/09 23:23:48, qiangchen wrote: > > Is it possible to remove this class? I mean just implement OnVideoFrame in > > MediaStreamVideoRendererSink. > > I think that would make the ownership and threading less clear. > We would provide a callback to MediaStreamVideoRendererSink::OnVideoFrame when > calling MediaStreamVideoSink::ConnectToTrack then. MediaStreamVideoRendererSink > would receive frames on IO thread. If we were to provide > MediaStreamVideoRendererSink as a weak_ptr, we would have to make sure that the > weak pointers are invalidated on IO thread before dtor is run. Since, dtor is > run on main thread, that would require synchronization. Pinning > MediaStreamVideoRendererSink to only one thread makes it much easier to > maintain. Acknowledged. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:91: main_task_runner_->PostTask( On 2016/11/11 22:27:19, emircan wrote: > On 2016/11/09 19:44:39, qiangchen wrote: > > I am worrying that this might be an issue. Theoretically, it is possible that > > other compositor thread tasks run before OnFirstFrameReceived got called. > Could > > there be an issue? Can you double checkthat? > > Yes, compositor tasks may run on before OnFirstFrameReceived() is completed. > Unfortunately, we do not know rotation/opacity information until first frame is > received, so we do not have enough info for initialization. > > I can add an AsyncWaiter to make sure OnFirstFrameReceived() completes before > moving on, but again we would keep waiting when main thread is busy. > > WebMediaPlayerImpl does this based on metadata[0][1] but again posts a task on > main. I am not sure about the synchronization there. dalecurtis@ might know more > about it. > > [0] > https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=14788666... > [1] > https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=1... Never mind. I did some research, it is the "setWebLayer" call in the |OnFirstFrameReceived| that starts calls to WebMediaPlayerMSCompositor on compositor thread. So I think it is fine here. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:133: base::ThreadChecker thread_checker_; On 2016/11/11 22:27:19, emircan wrote: > On 2016/11/09 19:44:38, qiangchen wrote: > > nit: Can you rename it to compositor_thread_checker_? As it would make the > code > > readability better. > > Done. Acknowledged. https://codereview.chromium.org/2472273002/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:597: video_rotation_ = video_rotation; On 2016/11/11 22:27:19, emircan wrote: > On 2016/11/09 23:23:48, qiangchen wrote: > > Is it possible to eliminate video_rotation_? I found it is just used once > below > > within this function. > > It is also used in WebMediaPlayerMS::naturalSize() and > WebMediaPlayerMS::paint(). Acknowledged.
Description was changed from ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory error. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining in between. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiverOnIO receives frames on IO and forwards to compositor. - FrameDelivererOnCompositor forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. ========== to ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory error. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining in between. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiverOnIO receives frames on IO and forwards to compositor. - FrameDelivererOnCompositor forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. Running alert() on a page requires manual user interaction. ==========
The explosion of WeakPtr usage is very worrying and currently incorrect. These classes are well overdue for some cleanup with regards to lifetime. As is, it's fairly impossible to tell what should be alive at what point. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:31: class MediaStreamVideoRendererSink::FrameDelivererOnCompositor { Generally we describe where a class runs by it's variable name, not it's class name (if it all). I think this naming is more confusing than anything, but up to you. I'd just call this a FrameDeliverer or FrameSender if you want to keep it inline with the receiver terminology on its pair-wise class. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:34: const RepaintCB repaint_cb, const& https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:35: const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, New classes should pass scoped_refptr by value. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:42: weak_factory_for_compositor_(this) { There's only one, so not really any reason to call this "for_compositor" https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:52: ~FrameDelivererOnCompositor() { Add blank line. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:73: frame_size_ = frame->natural_size(); Isn't this going to get out of sync with what's actually rendering if you update it here instead of after the copy completes? https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:92: void FrameReady(const scoped_refptr<media::VideoFrame>& frame) { Sequeneces of Start/Stop/Start are going to result in this method calling repaint_cb_ with something from a previous run. Is that okay? https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:106: void RenderSignalingFrame() { RenderEndOfStream? https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:115: (state_ == STOPPED) ? gfx::Size(kMinFrameSize, kMinFrameSize) No unnecessary parens. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:132: DCHECK(state_ == STARTED || state_ == PAUSED); for multiple DCHECKs() like this add a <<state_ to save yourself some debugging later. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:138: if (state_ == PAUSED) Instead of conditionals can these be DCHECKs() ? https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:144: if (state_ == STARTED) Ditto? https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:148: base::WeakPtr<FrameDelivererOnCompositor> GetWeakPtr() { This is a bit confusingly named since we also have a SupportsWeakPtr class you can inherit for this function. Instead of keeping a factory and having the method just extend SupportsWeakPtr if you really need to vend these externally (typically this is frowned upon). https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:177: // FrameReceiverOnIO is responsible for trampolining frames from IO thread to Ditto on naming. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:189: weak_factory_for_io_(this) { Ditto on SupportsWeakPtr. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:246: frame_deliverer_->GetWeakPtr())); You can't vend WeakPtr's for the compositor from the main thread. WeakPtr's must be created and dereferenced on the same thread. Once created they can be passed anywhere though. This and every other instance of GetWeakPtr() from the main thread is incorrect. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.h (right): https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.h:72: // Inner class used for trampolining frames from IO thread to compositor No reason to define these here, just put them with the unique_ptrs. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:48: typedef base::Callback<void(const scoped_refptr<media::VideoFrame>&)> New usage should be by value. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:112: base::WeakPtr<FrameDelivererOnCompositor> GetWeakPtr() { Ditto on WeakPtr.
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 emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:31: class MediaStreamVideoRendererSink::FrameDelivererOnCompositor { On 2016/11/15 00:30:16, DaleCurtis wrote: > Generally we describe where a class runs by it's variable name, not it's class > name (if it all). I think this naming is more confusing than anything, but up to > you. I'd just call this a FrameDeliverer or FrameSender if you want to keep it > inline with the receiver terminology on its pair-wise class. Done. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:34: const RepaintCB repaint_cb, On 2016/11/15 00:30:15, DaleCurtis wrote: > const& Done. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:35: const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, On 2016/11/15 00:30:16, DaleCurtis wrote: > New classes should pass scoped_refptr by value. Done. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:42: weak_factory_for_compositor_(this) { On 2016/11/15 00:30:16, DaleCurtis wrote: > There's only one, so not really any reason to call this "for_compositor" Done. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:52: ~FrameDelivererOnCompositor() { On 2016/11/15 00:30:16, DaleCurtis wrote: > Add blank line. Done. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:73: frame_size_ = frame->natural_size(); On 2016/11/15 00:30:16, DaleCurtis wrote: > Isn't this going to get out of sync with what's actually rendering if you update > it here instead of after the copy completes? Moving it to after copy. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:92: void FrameReady(const scoped_refptr<media::VideoFrame>& frame) { On 2016/11/15 00:30:16, DaleCurtis wrote: > Sequeneces of Start/Stop/Start are going to result in this method calling > repaint_cb_ with something from a previous run. Is that okay? I would say that is OK and consistent, considering that the frame would be shown if |gpu_memory_buffer_pool_| isn't supported. Time spent in GMBVFP would otherwise be spent during the copies in rendering SW backed frame. Considering this, the second |state_| check looks inconsistent now and I am removing. WDYT? https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:106: void RenderSignalingFrame() { On 2016/11/15 00:30:16, DaleCurtis wrote: > RenderEndOfStream? Done. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:115: (state_ == STOPPED) ? gfx::Size(kMinFrameSize, kMinFrameSize) On 2016/11/15 00:30:16, DaleCurtis wrote: > No unnecessary parens. Done. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:132: DCHECK(state_ == STARTED || state_ == PAUSED); On 2016/11/15 00:30:16, DaleCurtis wrote: > for multiple DCHECKs() like this add a <<state_ to save yourself some debugging > later. Done. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:138: if (state_ == PAUSED) On 2016/11/15 00:30:15, DaleCurtis wrote: > Instead of conditionals can these be DCHECKs() ? I am not sure. Running "play(); play();" on JS would result in double Resume() calls. I guess we wouldn't want to crash on second one, but just ignore. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:144: if (state_ == STARTED) On 2016/11/15 00:30:16, DaleCurtis wrote: > Ditto? Same as above, "pause(); pause();" would crash if we add. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:148: base::WeakPtr<FrameDelivererOnCompositor> GetWeakPtr() { On 2016/11/15 00:30:15, DaleCurtis wrote: > This is a bit confusingly named since we also have a SupportsWeakPtr class you > can inherit for this function. Instead of keeping a factory and having the > method just extend SupportsWeakPtr if you really need to vend these externally > (typically this is frowned upon). I switched to AsWeakPtr(). https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:177: // FrameReceiverOnIO is responsible for trampolining frames from IO thread to On 2016/11/15 00:30:16, DaleCurtis wrote: > Ditto on naming. Done. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:189: weak_factory_for_io_(this) { On 2016/11/15 00:30:16, DaleCurtis wrote: > Ditto on SupportsWeakPtr. Done. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:246: frame_deliverer_->GetWeakPtr())); On 2016/11/15 00:30:16, DaleCurtis wrote: > You can't vend WeakPtr's for the compositor from the main thread. WeakPtr's must > be created and dereferenced on the same thread. Once created they can be passed > anywhere though. This and every other instance of GetWeakPtr() from the main > thread is incorrect. I am confused by what you said. WeakPtrs must be dereferenced and invalidated on the same thread[0]. In this case, both happens on compositor thread. PostTask dereferences them via WeakPtr::get() and pins them to |compositor_task_runner_|. ~FrameDelivererOnCompositor invalidates them on |compositor_task_runner_|. There are DCHECKs ensuring that and it passes those DCHECKs currently. AFAIK, there isn't any restriction on where WeakPtr instances are created and WeakPtr instances can be handed off to other threads. Let me know if I am missing something. Alternative would be making these classes scoped_refptr's but that makes ownership and lifetime less clear. I tried hard to avoid using scoped_refptr here. Currently, these classes are unique_ptr's belonging to MediaStreamVideoRendererSink. WeakPtr's are used to pass references between threads. [0] https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?rcl=0&l=52 https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.h (right): https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.h:72: // Inner class used for trampolining frames from IO thread to compositor On 2016/11/15 00:30:16, DaleCurtis wrote: > No reason to define these here, just put them with the unique_ptrs. Done. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:48: typedef base::Callback<void(const scoped_refptr<media::VideoFrame>&)> On 2016/11/15 00:30:16, DaleCurtis wrote: > New usage should be by value. Done. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:112: base::WeakPtr<FrameDelivererOnCompositor> GetWeakPtr() { On 2016/11/15 00:30:16, DaleCurtis wrote: > Ditto on WeakPtr. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
dalecurtis@chromium.org changed reviewers: + wez@chromium.org
+wez for WeakPtr semantics in case I've recalled something incorrectly. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:92: void FrameReady(const scoped_refptr<media::VideoFrame>& frame) { On 2016/11/15 at 19:54:01, emircan wrote: > On 2016/11/15 00:30:16, DaleCurtis wrote: > > Sequeneces of Start/Stop/Start are going to result in this method calling > > repaint_cb_ with something from a previous run. Is that okay? > > I would say that is OK and consistent, considering that the frame would be shown if |gpu_memory_buffer_pool_| isn't supported. Time spent in GMBVFP would otherwise be spent during the copies in rendering SW backed frame. Considering this, the second |state_| check looks inconsistent now and I am removing. WDYT? Seems fine, but defer to qiangchen@. https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:138: if (state_ == PAUSED) On 2016/11/15 at 19:54:01, emircan wrote: > On 2016/11/15 00:30:15, DaleCurtis wrote: > > Instead of conditionals can these be DCHECKs() ? > > I am not sure. Running "play(); play();" on JS would result in double Resume() calls. I guess we wouldn't want to crash on second one, but just ignore. Does calling play() play() not already have an existing state check? https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:246: frame_deliverer_->GetWeakPtr())); On 2016/11/15 at 19:54:01, emircan wrote: > On 2016/11/15 00:30:16, DaleCurtis wrote: > > You can't vend WeakPtr's for the compositor from the main thread. WeakPtr's must > > be created and dereferenced on the same thread. Once created they can be passed > > anywhere though. This and every other instance of GetWeakPtr() from the main > > thread is incorrect. > > I am confused by what you said. WeakPtrs must be dereferenced and invalidated on the same thread[0]. In this case, both happens on compositor thread. PostTask dereferences them via WeakPtr::get() and pins them to |compositor_task_runner_|. ~FrameDelivererOnCompositor invalidates them on |compositor_task_runner_|. There are DCHECKs ensuring that and it passes those DCHECKs currently. AFAIK, there isn't any restriction on where WeakPtr instances are created and WeakPtr instances can be handed off to other threads. > > Let me know if I am missing something. Alternative would be making these classes scoped_refptr's but that makes ownership and lifetime less clear. I tried hard to avoid using scoped_refptr here. Currently, these classes are unique_ptr's belonging to MediaStreamVideoRendererSink. WeakPtr's are used to pass references between threads. > > [0] https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?rcl=0&l=52 If you create the weak ptr on a different thread than it's used you have a race in WeakReferenceOwner::GetRef() (see weak_ptr.cc) - HasRefs() and GetRefs() may race. I could have sworn the WeakPtr documentation used to have a note about not creating them on threads other than their used on. +wez in case the times have changed and I've missed something.
On 2016/11/15 20:29:03, DaleCurtis wrote: > +wez for WeakPtr semantics in case I've recalled something incorrectly. > > https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... > File content/renderer/media/media_stream_video_renderer_sink.cc (right): > > https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... > content/renderer/media/media_stream_video_renderer_sink.cc:92: void > FrameReady(const scoped_refptr<media::VideoFrame>& frame) { > On 2016/11/15 at 19:54:01, emircan wrote: > > On 2016/11/15 00:30:16, DaleCurtis wrote: > > > Sequeneces of Start/Stop/Start are going to result in this method calling > > > repaint_cb_ with something from a previous run. Is that okay? > > > > I would say that is OK and consistent, considering that the frame would be > shown if |gpu_memory_buffer_pool_| isn't supported. Time spent in GMBVFP would > otherwise be spent during the copies in rendering SW backed frame. Considering > this, the second |state_| check looks inconsistent now and I am removing. WDYT? > > Seems fine, but defer to qiangchen@. > > https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... > content/renderer/media/media_stream_video_renderer_sink.cc:138: if (state_ == > PAUSED) > On 2016/11/15 at 19:54:01, emircan wrote: > > On 2016/11/15 00:30:15, DaleCurtis wrote: > > > Instead of conditionals can these be DCHECKs() ? > > > > I am not sure. Running "play(); play();" on JS would result in double Resume() > calls. I guess we wouldn't want to crash on second one, but just ignore. > > Does calling play() play() not already have an existing state check? > > https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... > content/renderer/media/media_stream_video_renderer_sink.cc:246: > frame_deliverer_->GetWeakPtr())); > On 2016/11/15 at 19:54:01, emircan wrote: > > On 2016/11/15 00:30:16, DaleCurtis wrote: > > > You can't vend WeakPtr's for the compositor from the main thread. WeakPtr's > must > > > be created and dereferenced on the same thread. Once created they can be > passed > > > anywhere though. This and every other instance of GetWeakPtr() from the main > > > thread is incorrect. > > > > I am confused by what you said. WeakPtrs must be dereferenced and invalidated > on the same thread[0]. In this case, both happens on compositor thread. PostTask > dereferences them via WeakPtr::get() and pins them to |compositor_task_runner_|. > ~FrameDelivererOnCompositor invalidates them on |compositor_task_runner_|. There > are DCHECKs ensuring that and it passes those DCHECKs currently. AFAIK, there > isn't any restriction on where WeakPtr instances are created and WeakPtr > instances can be handed off to other threads. > > > > Let me know if I am missing something. Alternative would be making these > classes scoped_refptr's but that makes ownership and lifetime less clear. I > tried hard to avoid using scoped_refptr here. Currently, these classes are > unique_ptr's belonging to MediaStreamVideoRendererSink. WeakPtr's are used to > pass references between threads. > > > > [0] https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?rcl=0&l=52 > > If you create the weak ptr on a different thread than it's used you have a race > in WeakReferenceOwner::GetRef() (see weak_ptr.cc) - HasRefs() and GetRefs() may > race. I could have sworn the WeakPtr documentation used to have a note about not > creating them on threads other than their used on. +wez in case the times have > changed and I've missed something. WeakPtrs must be dereferenced and invalidated on the same thread. That thread may differ from the one on which GetWeakPtr() is called, i.e. you can call GetWeakPtr() on thread A, but then use and invalidate on thread B. There are two reasons not to use GetWeakPtr() from thread A once you've started using WeakPtrs from that factory on thread B: 1. As Dale notes, the implementation is racey, though in this specific case the race will fail-safe, IIUC. 2. In order to call GetWeakPtr() from the main thread, you must *know* that the WeakPtrFactory (and the object it vends WeakPtrs to) is still valid. If you *know* that it is valid at that point then its lifetime is almost certainly[1] controlled by the main thread. If that is the case then you almost certainly[1] don't need to use WeakPtrs. Looking at your use-case, you should be using base::Unretained(), not WeakPtr - you are deleting the FrameDelivererOnCompositor instance from your destructor, on the compositor thread, so when you PostTask(), you are guaranteed that the task will be processed before the FDOC is deleted. [1] There are complex exceptions to these. :P
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
> > If you create the weak ptr on a different thread than it's used you have a > race > > in WeakReferenceOwner::GetRef() (see weak_ptr.cc) - HasRefs() and GetRefs() > may > > race. I could have sworn the WeakPtr documentation used to have a note about > not > > creating them on threads other than their used on. +wez in case the times have > > changed and I've missed something. > > WeakPtrs must be dereferenced and invalidated on the same thread. > > That thread may differ from the one on which GetWeakPtr() is called, i.e. you > can call GetWeakPtr() on thread A, but then use and invalidate on thread B. > > There are two reasons not to use GetWeakPtr() from thread A once you've started > using WeakPtrs from that factory on thread B: > 1. As Dale notes, the implementation is racey, though in this specific case the > race will fail-safe, IIUC. > 2. In order to call GetWeakPtr() from the main thread, you must *know* that the > WeakPtrFactory (and the object it vends WeakPtrs to) is still valid. If you > *know* that it is valid at that point then its lifetime is almost certainly[1] > controlled by the main thread. If that is the case then you almost certainly[1] > don't need to use WeakPtrs. > > Looking at your use-case, you should be using base::Unretained(), not WeakPtr - > you are deleting the FrameDelivererOnCompositor instance from your destructor, > on the compositor thread, so when you PostTask(), you are guaranteed that the > task will be processed before the FDOC is deleted. > > [1] There are complex exceptions to these. :P Thanks for the clarification. I was avoiding base::Unretained() usage as it is simply dangerous, but in this case it clarifies the ownership. I added base::Unretained() for all methods running on main thread. However, I still need a WeakPtr<FrameDeliverer> instance to pass to FrameReceiver, because FrameReceiver is destroyed on IO thread whereas FrameDeliverer is on compositor. Is the current use safe? Also, can we work-around this race issue by just calling GetWeakPtr()/AsWeakPtr() once and storing the WeakPtr instance? https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/180001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:138: if (state_ == PAUSED) On 2016/11/15 20:29:03, DaleCurtis wrote: > On 2016/11/15 at 19:54:01, emircan wrote: > > On 2016/11/15 00:30:15, DaleCurtis wrote: > > > Instead of conditionals can these be DCHECKs() ? > > > > I am not sure. Running "play(); play();" on JS would result in double Resume() > calls. I guess we wouldn't want to crash on second one, but just ignore. > > Does calling play() play() not already have an existing state check? It actually does, "load(); play();" causes the problem.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Yes, the preferred model is to call GetWeakPtr() once during setup of your class, and to store the pointer for use in Bind(). https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:31: class MediaStreamVideoRendererSink::FrameDeliverer This naming is rather confusing; MediaStreamVideoSink already has a FrameDeliverer sub-class. https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:32: : public base::SupportsWeakPtr<FrameDeliverer> { Please use WeakPtrFactory rather than SupportsWeakPtr. Thanks! https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:42: compositor_thread_checker_.DetachFromThread(); Is the compositor thread one of the ones passed in as a task-runner, by any chance? https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:123: DCHECK_EQ(state_, STOPPED) << state_; Do you need << state_ here? I thought DCHECK_EQ() logged both values on failure? https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:233: compositor_task_runner_, frame_deliverer_->AsWeakPtr())); Bear in mind that FrameReceiver's scope is Start->Stop, whereas the FrameDeliverer's scope is the lifetime of the Sink - you may have an OnVideoFrame task in-flight to the FD when you call Stop(), and that will still be processed and result in a call to |repaint_cb_|. If you have a rapid Start()->Stop()->Start() cycle then that means you may get a |repaint_cb_| after the second Start(), but with frame content from between the first Start()->Stop(). Is that a problem? https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:236: base::Bind(&FrameReceiver::OnVideoFrame, frame_receiver_->AsWeakPtr()), nit: It is not obvious from the context that ConnectToTrack actually causes the supplied callback to be called on the IO thread; may be worth mentioning here with a comment, for clarity. Also, since MediaStreamVideoSink will invoke the callback on the IO thread, and the FrameReceiver is DeleteSoon()d to the IO thread, could you use RemoveSink() in Stop(), and then you can use base::Unretained() for the receiver and avoid exposing WeakPtr? https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:254: ChildProcess::current()->io_task_runner()->DeleteSoon( Rather than getting at the IO task runner for this process this way, I'd suggest either (1) wrapping the logic in a private io_task_runner() getter on this class or (2) passing it in explicitly from the caller, to the constructor.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:31: class MediaStreamVideoRendererSink::FrameDeliverer On 2016/11/16 01:47:10, Wez wrote: > This naming is rather confusing; MediaStreamVideoSink already has a > FrameDeliverer sub-class. Are you referring to MediaStreamVideoTrack::FrameDeliverer?[0] It is used for a similar purpose, so I wanted to keep the naming similar for these inner classes. Do you have any suggestions? [0] https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide... https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:32: : public base::SupportsWeakPtr<FrameDeliverer> { On 2016/11/16 01:47:10, Wez wrote: > Please use WeakPtrFactory rather than SupportsWeakPtr. Thanks! I had WeakPtrFactory, but switched to SupportsWeakPtr after comments on #54. Reverting now. https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:42: compositor_thread_checker_.DetachFromThread(); On 2016/11/16 01:47:10, Wez wrote: > Is the compositor thread one of the ones passed in as a task-runner, by any > chance? No. However, I can pass it in ctor and change thread checkers into |compositor_task_runner_.BelongsToCurrentThread()| if that is what you are suggesting. https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:123: DCHECK_EQ(state_, STOPPED) << state_; On 2016/11/16 01:47:10, Wez wrote: > Do you need << state_ here? I thought DCHECK_EQ() logged both values on > failure? Done. https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:233: compositor_task_runner_, frame_deliverer_->AsWeakPtr())); On 2016/11/16 01:47:10, Wez wrote: > Bear in mind that FrameReceiver's scope is Start->Stop, whereas the > FrameDeliverer's scope is the lifetime of the Sink - you may have an > OnVideoFrame task in-flight to the FD when you call Stop(), and that will still > be processed and result in a call to |repaint_cb_|. > > If you have a rapid Start()->Stop()->Start() cycle then that means you may get a > |repaint_cb_| after the second Start(), but with frame content from between the > first Start()->Stop(). > > Is that a problem? Yes, it looks like a problem, i.e. OnVideoFrame() is scheduled after RenderEndOfStream(). Thanks for catching this. I couldn't think of another solution then calling InvalidateWeakPtrs in FrameDeliverer::Stop(). Then, we would need to make sure that FrameReceiver has a valid WeakPtr. To make things clear, I synched both classes' lifetimes. WDYT? https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:236: base::Bind(&FrameReceiver::OnVideoFrame, frame_receiver_->AsWeakPtr()), On 2016/11/16 01:47:10, Wez wrote: > nit: It is not obvious from the context that ConnectToTrack actually causes the > supplied callback to be called on the IO thread; may be worth mentioning here > with a comment, for clarity. > Added the comment. > Also, since MediaStreamVideoSink will invoke the callback on the IO thread, and > the FrameReceiver is DeleteSoon()d to the IO thread, could you use RemoveSink() > in Stop(), and then you can use base::Unretained() for the receiver and avoid > exposing WeakPtr? Thanks for pointing this out. It makes sense now that it will be synchronized on IO thread, i.e. DeliverFrameOnIO()->RemoveCallbackOnIO()->~FrameReceiver() sequence. Note that I similarly removed WebMediaPlayerMSCompositor::GetweakPtr(). https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:254: ChildProcess::current()->io_task_runner()->DeleteSoon( On 2016/11/16 01:47:10, Wez wrote: > Rather than getting at the IO task runner for this process this way, I'd suggest > either (1) wrapping the logic in a private io_task_runner() getter on this class > or (2) passing it in explicitly from the caller, to the constructor. I tried to do (1). Note that this is currently how IO thread is acquired in source. https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide...
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:32: : public base::SupportsWeakPtr<FrameDeliverer> { On 2016/11/17 at 01:23:25, emircan wrote: > On 2016/11/16 01:47:10, Wez wrote: > > Please use WeakPtrFactory rather than SupportsWeakPtr. Thanks! > > I had WeakPtrFactory, but switched to SupportsWeakPtr after comments on #54. Reverting now. Wez, I'm surprised by this advice. If you have a class with a public GetWeakPtr() method, why go through the fuss of a factory and getter when it's provided by SupportsWeakPtr?
https://codereview.chromium.org/2472273002/diff/240001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/240001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:238: compositor_task_runner_, frame_deliverer_->GetWeakPtr())); As mentioned on the other instance of this class, it looks like you could have the deliverer return a callback internally bound to a weakptr so you don't need to pass the weakptr around. https://codereview.chromium.org/2472273002/diff/240001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/240001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:233: base::Bind(&FrameDeliverer::OnVideoFrame, frame_deliverer_->AsWeakPtr()), Can you just have the frame deliverer return a callback to OnVideoFrame? that way you don't need a public WeakPtr objet.
https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:32: : public base::SupportsWeakPtr<FrameDeliverer> { On 2016/11/17 02:13:17, DaleCurtis wrote: > On 2016/11/17 at 01:23:25, emircan wrote: > > On 2016/11/16 01:47:10, Wez wrote: > > > Please use WeakPtrFactory rather than SupportsWeakPtr. Thanks! > > > > I had WeakPtrFactory, but switched to SupportsWeakPtr after comments on #54. > Reverting now. > > Wez, I'm surprised by this advice. If you have a class with a public > GetWeakPtr() method, why go through the fuss of a factory and getter when it's > provided by SupportsWeakPtr? Sorry, I meant to add the suggestion of instead having a getter on FrameDeliverer, that returns Bind(&FrameDeliverer::OnVideoFrame, weak_factory_.GetWeakPtr()) that the calling class would use to get a callback that it can pass to the FrameReceiver, so you can avoid AsWeakPtr() entirely. FWIW the two main arguments against SupportsWeakPtr are (1) it forces you to expose AsWeakPtr(), risking later changes using WeakPtrs in ways you didn't intend, and (2) it is a base-class, so doesn't invalidate WeakPtrs until *after* the sub-class members have torn-down (only a problem if callbacks might be invoked mid-destructor, though). In this case the Deliverer and Receiver are implementation details, so (1) is less of an issue.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:32: : public base::SupportsWeakPtr<FrameDeliverer> { On 2016/11/17 02:30:27, Wez wrote: > Sorry, I meant to add the suggestion of instead having a getter on > FrameDeliverer, that returns Bind(&FrameDeliverer::OnVideoFrame, > weak_factory_.GetWeakPtr()) that the calling class would use to get a callback > that it can pass to the FrameReceiver, so you can avoid AsWeakPtr() entirely. > Done. https://codereview.chromium.org/2472273002/diff/240001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/240001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:238: compositor_task_runner_, frame_deliverer_->GetWeakPtr())); On 2016/11/17 02:18:51, DaleCurtis wrote: > As mentioned on the other instance of this class, it looks like you could have > the deliverer return a callback internally bound to a weakptr so you don't need > to pass the weakptr around. Done. https://codereview.chromium.org/2472273002/diff/240001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/240001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:233: base::Bind(&FrameDeliverer::OnVideoFrame, frame_deliverer_->AsWeakPtr()), On 2016/11/17 02:18:51, DaleCurtis wrote: > Can you just have the frame deliverer return a callback to OnVideoFrame? that > way you don't need a public WeakPtr objet. Done. I removed base::SupportsWeakPtr<FrameDeliverer> as well so WeakPtr is private.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Patchset #8 (id:260001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:30: // be destructed on compositor thread. /s on the compositor thread https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:263: GetIOTaskRunner()->DeleteSoon(FROM_HERE, frame_receiver_.release()); Is Stop always called? Othertwise frame_receiver will be deleted on the wrong thread. https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:45: // be destructed on compositor thread. on the ..
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Thanks. https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:30: // be destructed on compositor thread. On 2016/11/17 21:54:01, perkj_chrome wrote: > /s on the compositor thread Done. https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:263: GetIOTaskRunner()->DeleteSoon(FROM_HERE, frame_receiver_.release()); On 2016/11/17 21:54:01, perkj_chrome wrote: > Is Stop always called? Othertwise frame_receiver will be deleted on the wrong > thread. Yes, see https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms.... https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:45: // be destructed on compositor thread. On 2016/11/17 21:54:01, perkj_chrome wrote: > on the .. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, description needs a bit of update though
Description was changed from ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory error. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining in between. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiverOnIO receives frames on IO and forwards to compositor. - FrameDelivererOnCompositor forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. Running alert() on a page requires manual user interaction. ========== to ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory error. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining in between. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiver receives frames on IO and forwards to compositor. - FrameDeliverer forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. Running alert() on a page requires manual user interaction. ==========
emircan@chromium.org changed reviewers: - mcasas@chromium.org
> lgtm, description needs a bit of update though Thanks. I just updated the class names.
emircan@chromium.org changed reviewers: + avi@chromium.org
avi@, can PTAL at content/public/renderer/media_stream_renderer_factory.* and content/shell/renderer/layout_test/test_media_stream_renderer_factory.*?
On 2016/11/18 00:33:30, emircan wrote: > avi@, can PTAL at content/public/renderer/media_stream_renderer_factory.* and > content/shell/renderer/layout_test/test_media_stream_renderer_factory.*? content/public and content/shell lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Some final suggestions but otherwise WeakPtr usage LGTM. https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:233: compositor_task_runner_, frame_deliverer_->AsWeakPtr())); On 2016/11/17 01:23:25, emircan wrote: > On 2016/11/16 01:47:10, Wez wrote: > > Bear in mind that FrameReceiver's scope is Start->Stop, whereas the > > FrameDeliverer's scope is the lifetime of the Sink - you may have an > > OnVideoFrame task in-flight to the FD when you call Stop(), and that will > still > > be processed and result in a call to |repaint_cb_|. > > > > If you have a rapid Start()->Stop()->Start() cycle then that means you may get > a > > |repaint_cb_| after the second Start(), but with frame content from between > the > > first Start()->Stop(). > > > > Is that a problem? > > Yes, it looks like a problem, i.e. OnVideoFrame() is scheduled after > RenderEndOfStream(). Thanks for catching this. > > I couldn't think of another solution then calling InvalidateWeakPtrs in > FrameDeliverer::Stop(). Then, we would need to make sure that FrameReceiver has > a valid WeakPtr. To make things clear, I synched both classes' lifetimes. WDYT? If having the two classes' lifetimes scoped to Start/Stop works for this impl then yes, that seems a good solution to the problem (plus as a minor bonus it avoids the FrameDeliverer sitting around idle for no good reason :) https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:254: ChildProcess::current()->io_task_runner()->DeleteSoon( On 2016/11/17 01:23:25, emircan wrote: > On 2016/11/16 01:47:10, Wez wrote: > > Rather than getting at the IO task runner for this process this way, I'd > suggest > > either (1) wrapping the logic in a private io_task_runner() getter on this > class > > or (2) passing it in explicitly from the caller, to the constructor. > > I tried to do (1). Note that this is currently how IO thread is acquired in > source. > https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide... OK; if there is a //src/media/ convention for that approach then feel free to follow it rather than defining a helper getter. :) https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:131: weak_factory_for_compositor_.InvalidateWeakPtrs(); Is there any point InvalidateWeakPtrs()ing here, since you always DeleteSoon() this object immediately after Stop()? In fact could you simply move this logic into the FrameDeliverer destructor and eliminate this Stop() method? https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:170: base::WeakPtr<FrameDeliverer> weak_this_; nit: FYI it is strictly safest to put |weak_ptr_| as the first member, so that, if something happens to call into |this| mid-destructor (e.g. via a callback) and touches |weak_ptr_|, it is still constructed, albeit null, at that point. Shouldn't be a problem for this class, though, just an FYI. https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:171: base::WeakPtrFactory<FrameDeliverer> weak_factory_for_compositor_; This can just be |weak_factory_|, since this class is used exclusively on the compositor thread - even if it were used on multiple threads, you couldn't have separate WeakPtrFactorys for each without causing yourself much horror. :P
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/220001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:254: ChildProcess::current()->io_task_runner()->DeleteSoon( On 2016/11/18 02:26:56, Wez wrote: > On 2016/11/17 01:23:25, emircan wrote: > > On 2016/11/16 01:47:10, Wez wrote: > > > Rather than getting at the IO task runner for this process this way, I'd > > suggest > > > either (1) wrapping the logic in a private io_task_runner() getter on this > > class > > > or (2) passing it in explicitly from the caller, to the constructor. > > > > I tried to do (1). Note that this is currently how IO thread is acquired in > > source. > > > https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide... > > OK; if there is a //src/media/ convention for that approach then feel free to > follow it rather than defining a helper getter. :) Done. https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... File content/renderer/media/media_stream_video_renderer_sink.cc (right): https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:131: weak_factory_for_compositor_.InvalidateWeakPtrs(); On 2016/11/18 02:26:56, Wez wrote: > Is there any point InvalidateWeakPtrs()ing here, since you always DeleteSoon() > this object immediately after Stop()? > > In fact could you simply move this logic into the FrameDeliverer destructor and > eliminate this Stop() method? Sure, I moved it into the dtor. https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:170: base::WeakPtr<FrameDeliverer> weak_this_; On 2016/11/18 02:26:56, Wez wrote: > nit: FYI it is strictly safest to put |weak_ptr_| as the first member, so that, > if something happens to call into |this| mid-destructor (e.g. via a callback) > and touches |weak_ptr_|, it is still constructed, albeit null, at that point. > Shouldn't be a problem for this class, though, just an FYI. Thanks, it is a very helpful tip. https://codereview.chromium.org/2472273002/diff/280001/content/renderer/media... content/renderer/media/media_stream_video_renderer_sink.cc:171: base::WeakPtrFactory<FrameDeliverer> weak_factory_for_compositor_; On 2016/11/18 02:26:56, Wez wrote: > This can just be |weak_factory_|, since this class is used exclusively on the > compositor thread - even if it were used on multiple threads, you couldn't have > separate WeakPtrFactorys for each without causing yourself much horror. :P Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by emircan@chromium.org
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qiangchen@chromium.org, perkj@chromium.org, avi@chromium.org, wez@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2472273002/#ps320001 (title: "wez@ nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory error. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining in between. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiver receives frames on IO and forwards to compositor. - FrameDeliverer forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. Running alert() on a page requires manual user interaction. ========== to ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory error. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining in between. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiver receives frames on IO and forwards to compositor. - FrameDeliverer forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. Running alert() on a page requires manual user interaction. ==========
Message was sent while issue was closed.
Committed patchset #10 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory error. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining in between. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiver receives frames on IO and forwards to compositor. - FrameDeliverer forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. Running alert() on a page requires manual user interaction. ========== to ========== Move passing of WebRTC rendering frames from main thread to compositor thread When main render thread is busy with JS functions, WebRTC video playback stalls and starts accumulating frames resulting in increased memory. It sometimes recovers by playing the accumulated frames quickly, and sometimes causes out of memory error. This CL modifies the passing to video frames in rendering path such that main thread is not used. We jump from IO thread directly to compositor thread and avoid trampolining in between. Since lifetime and methods of these classes, such as WebMediaPlayerMS, is still tied to main thread, I created inner classes which are pinned to specific threads and carry forwarding tasks: - FrameReceiver receives frames on IO and forwards to compositor. - FrameDeliverer forwards frames on compositor. BUG=652923 TEST=Modified unit tests. Tested running JS alert() in console during an AppRTC loopback. I am still confused to how/where to add a proper browsertest. Running alert() on a page requires manual user interaction. Committed: https://crrev.com/99eb6bc119173a68cc695f5179c72cadf0b515da Cr-Commit-Position: refs/heads/master@{#433356} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/99eb6bc119173a68cc695f5179c72cadf0b515da Cr-Commit-Position: refs/heads/master@{#433356}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
MSAN bots are red. Bug has been filed. Should be an easy fix. https://codereview.chromium.org/2472273002/diff/320001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (left): https://codereview.chromium.org/2472273002/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:510: ignore_result(frame->metadata()->GetRotation( The previous code was calling GetRotation() on |video_rotation_| and that was initialized in the ctor. https://codereview.chromium.org/2472273002/diff/320001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2472273002/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:87: media::VideoRotation video_rotation; This is not initialized and is triggering bug https://crbug.com/667026. |
