RTCVideoEncoder not usable from non-construction thread |
|||||||||
Issue descriptionInitEncode/Release/etc. calls were moved off the libjingle worker thread (to the encoder thread, where it makes sense for them to be) to reduce contention and induced network jitter + permit setRemoteDescription to finish faster. This caused DCHECKs to fail inside RTCVideoEncoder. Woo. I'm not sure how to address these, because it doesn't look like I can just remove the DCHECKs. (And it looks like there is some posting back to the libjingle worker thread? The VideoEncoder instance is locked from the outside, and although this only happens from the encoder thread, this VideoEncoder instance can be reused between encoder threads (though not at the same time). posciak@/emircan@: Can you assist/suggest how this should be fixed in RTCVideoEncoder? It needs to be addressed fairly quickly or we have to revert a quite big performance improvement. Offending CL that moved all VideoEncoder usage onto the encoder thread: https://codereview.webrtc.org/1757313002
,
Mar 17 2016
,
Mar 19 2016
The new encoder thread is an rtc::PlatformThread. AFAIU there is no way to post tasks back to it [0] since it doesn't provide any task runner or message loop. Therefore, there isn't any WebRTC/Jingle thread to hand things over to. Is there a way you can use rtc::ProcessThread instead for Encoder thread, or something else that allows posting back? [0] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc/video/video_send_stream.h&rcl=1458329676&l=106
,
Mar 19 2016
You shouldn't need to post back, the callback should be invokable from the VDA thread (just make sure it's properly synchronized with the setter).
,
Mar 22 2016
pbos@ and emircan@ Will one of you have time to help fix this? We are busy with something else and don't have time to fix it now.
,
Mar 22 2016
Peter, Pawel and I discussed and here is the conclusion. - The only thing RtcVideoEncoder should rely on is it being synchronized from the outside (never called concurrently). It cannot assume the calling thread is postable. - RtcVideoEncoder needs to synchronize RegisterEncodeCompleteCallback and encode complete callback. - The discussed solution is (1) call encode complete callback from media thread. (2) Release/RegisterEncodeCompleteCallback post a task to media thread and wait until it finishes. - We can refer to Android implementation. https://chromium.googlesource.com/external/webrtc/+/master/webrtc/api/java/jni/androidmediaencoder_jni.cc I'll try fixing this next week. Currently the threading is as follows. This is just a note for myself for debugging. RtcVideoEncoder cannot assume the threading. - libjingle worker: constructor/destructor - videosendstream encoderthread (can be reused across multiple videosendstreams): the rest
,
Mar 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/external/webrtc.git/+/81cbd924447d507559dbd6e6d1f9fe439fcf2716 commit 81cbd924447d507559dbd6e6d1f9fe439fcf2716 Author: Peter Boström <pbos@webrtc.org> Date: Tue Mar 22 11:19:07 2016 Revert of Initialize/configure video encoders asychronously. (patchset #4 id:60001 of https://codereview.webrtc.org/1757313002/ ) Reason for revert: Breaks RTCVideoEncoder which has incorrect assumptions on where InitEncode etc. is called from. Temporarily reverting until RTCVideoEncoder has been updated. Original issue's description: > Initialize/configure video encoders asychronously. > > Greatly speeds up setRemoteDescription() by moving encoder initialization > off the main worker thread, which is free to move onto gathering ICE > candidates and other tasks while InitEncode() is performed. It also > un-blocks PeerConnection GetStats() which is no longer blocked on > encoder initialization. > > BUG= webrtc:5410 > R=stefan@webrtc.org > > Committed: https://chromium.googlesource.com/external/webrtc/+/fb647a67be94bb3c940d8b5fba01972f7ce91a29 R=stefan@webrtc.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=chromium:595274, chromium:595308 , webrtc:5410 Review URL: https://codereview.webrtc.org/1821983002 . Cr-Commit-Position: refs/heads/master@{#12086} [modify] https://crrev.com/81cbd924447d507559dbd6e6d1f9fe439fcf2716/webrtc/media/engine/fakewebrtcvideoengine.h [modify] https://crrev.com/81cbd924447d507559dbd6e6d1f9fe439fcf2716/webrtc/media/engine/webrtcvideoengine2_unittest.cc [modify] https://crrev.com/81cbd924447d507559dbd6e6d1f9fe439fcf2716/webrtc/video/video_quality_test.cc [modify] https://crrev.com/81cbd924447d507559dbd6e6d1f9fe439fcf2716/webrtc/video/video_send_stream.cc [modify] https://crrev.com/81cbd924447d507559dbd6e6d1f9fe439fcf2716/webrtc/video/video_send_stream.h [modify] https://crrev.com/81cbd924447d507559dbd6e6d1f9fe439fcf2716/webrtc/video/video_send_stream_tests.cc
,
Mar 22 2016
Adding M51 as target, we'd like to reland asynchronous encoder configuration by then (and then next week is also fine).
,
Mar 22 2016
,
Mar 31 2016
Note for myself: webrtc's VideoSender protects its webrtc::VideoEncoder with a critical section: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc/modules/video_coding/video_sender.cc Draft CL: https://codereview.chromium.org/1845563006
,
Apr 4 2016
,
Apr 11 2016
This won't make the M51 cut.
,
Apr 12 2016
status: https://codereview.chromium.org/1845563006 is waiting for owner's review.
,
Apr 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7aa249c90c143f50b554905c5dd515a8b25ef0ce commit 7aa249c90c143f50b554905c5dd515a8b25ef0ce Author: wuchengli <wuchengli@chromium.org> Date: Wed Apr 20 10:41:02 2016 Make RTCVideoEncoder usable from non-postable thread. - The only thing RtcVideoEncoder should rely on is it being synchronized from the outside (never called concurrently). It cannot assume the calling thread is postable. - RtcVideoEncoder needs to synchronize RegisterEncodeCompleteCallback and encode complete callback. BUG= chromium:595308 TEST=Revert https://codereview.webrtc.org/1821983002. Then try apprtc loopback and Hangout on oak. Review URL: https://codereview.chromium.org/1845563006 Cr-Commit-Position: refs/heads/master@{#388463} [modify] https://crrev.com/7aa249c90c143f50b554905c5dd515a8b25ef0ce/content/renderer/media/rtc_video_encoder.cc [modify] https://crrev.com/7aa249c90c143f50b554905c5dd515a8b25ef0ce/content/renderer/media/rtc_video_encoder.h
,
Apr 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/external/webrtc.git/+/14fe708f3d6357a3e3f9b62d8a6a9e310af86fcd commit 14fe708f3d6357a3e3f9b62d8a6a9e310af86fcd Author: pbos <pbos@webrtc.org> Date: Wed Apr 20 13:35:56 2016 Reland of Initialize/configure video encoders asychronously. (patchset #1 id:1 of https://codereview.webrtc.org/1821983002/ ) Reason for revert: RTCVideoEncoder has been updated to not make assumptions on calling threads/post back to a worker thread. This should now be landable again. Original issue's description: > Revert of Initialize/configure video encoders asychronously. (patchset #4 id:60001 of https://codereview.webrtc.org/1757313002/ ) > > Reason for revert: > Breaks RTCVideoEncoder which has incorrect assumptions on where InitEncode etc. is called from. Temporarily reverting until RTCVideoEncoder has been updated. > > Original issue's description: > > Initialize/configure video encoders asychronously. > > > > Greatly speeds up setRemoteDescription() by moving encoder initialization > > off the main worker thread, which is free to move onto gathering ICE > > candidates and other tasks while InitEncode() is performed. It also > > un-blocks PeerConnection GetStats() which is no longer blocked on > > encoder initialization. > > > > BUG= webrtc:5410 > > R=stefan@webrtc.org > > > > Committed: https://chromium.googlesource.com/external/webrtc/+/fb647a67be94bb3c940d8b5fba01972f7ce91a29 > > R=stefan@webrtc.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=chromium:595274, chromium:595308 , webrtc:5410 > > Committed: https://crrev.com/81cbd924447d507559dbd6e6d1f9fe439fcf2716 > Cr-Commit-Position: refs/heads/master@{#12086} TBR=stefan@webrtc.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=chromium:595274, chromium:595308 , webrtc:5410 Review URL: https://codereview.webrtc.org/1896413002 Cr-Commit-Position: refs/heads/master@{#12446} [modify] https://crrev.com/14fe708f3d6357a3e3f9b62d8a6a9e310af86fcd/webrtc/media/engine/fakewebrtcvideoengine.h [modify] https://crrev.com/14fe708f3d6357a3e3f9b62d8a6a9e310af86fcd/webrtc/media/engine/webrtcvideoengine2_unittest.cc [modify] https://crrev.com/14fe708f3d6357a3e3f9b62d8a6a9e310af86fcd/webrtc/video/video_quality_test.cc [modify] https://crrev.com/14fe708f3d6357a3e3f9b62d8a6a9e310af86fcd/webrtc/video/video_send_stream.cc [modify] https://crrev.com/14fe708f3d6357a3e3f9b62d8a6a9e310af86fcd/webrtc/video/video_send_stream.h [modify] https://crrev.com/14fe708f3d6357a3e3f9b62d8a6a9e310af86fcd/webrtc/video/video_send_stream_tests.cc
,
Apr 20 2016
,
Apr 28 2016
I got the following call stack on Mac. It suggests that we are not destroying RTCVideoEncoder::Impl cleanly, as earlier session's BitstreamBuffers are used. It results in a PlatformError and fallsback. I will work on a fix now. 33841:34307:0427/182322:VERBOSE2:gpu_video_encode_accelerator_host.cc(273)] OnRequireBitstreamBuffers(): input_count=3, input_coded_size=1280x720, output_buffer_size=921600 [33841:34307:0427/182322:VERBOSE3:rtc_video_encoder.cc(406)] Impl::RequireBitstreamBuffers(): input_count=3, input_coded_size=1280x720, output_buffer_size=921600 [33841:34307:0427/182322:VERBOSE3:rtc_video_encoder.cc(445)] RequireBitstreamBuffers25344 [33841:34307:0427/182322:VERBOSE3:rtc_video_encoder.cc(445)] RequireBitstreamBuffers25344 [33841:34307:0427/182322:VERBOSE3:rtc_video_encoder.cc(445)] RequireBitstreamBuffers25344 [33841:34307:0427/182322:VERBOSE3:rtc_video_encoder.cc(445)] RequireBitstreamBuffers921600 [33841:34307:0427/182322:VERBOSE3:rtc_video_encoder.cc(445)] RequireBitstreamBuffers921600 [33841:34307:0427/182322:VERBOSE3:rtc_video_encoder.cc(445)] RequireBitstreamBuffers921600 [33841:39175:0427/182322:VERBOSE3:rtc_video_encoder.cc(788)] RegisterEncodeCompleteCallback() [33826:1295:0427/182322:VERBOSE3:gpu_video_encode_accelerator.cc(343)] GpuVideoEncodeAccelerator::OnUseOutputBitstreamBuffer(): buffer_id=0, buffer_size=25344 [33826:1295:0427/182322:ERROR:gpu_video_encode_accelerator.cc(355)] GpuVideoEncodeAccelerator::OnUseOutputBitstreamBuffer(): 25344,921600buffer too small for buffer_id=0 [33826:1295:0427/182322:VERBOSE3:gpu_video_encode_accelerator.cc(163)] NotifyError [33826:1295:0427/182322:VERBOSE3:gpu_video_encode_accelerator.cc(343)] GpuVideoEncodeAccelerator::OnUseOutputBitstreamBuffer(): buffer_id=1, buffer_size=25344 [33826:1295:0427/182322:ERROR:gpu_video_encode_accelerator.cc(355)] GpuVideoEncodeAccelerator::OnUseOutputBitstreamBuffer(): 25344,921600buffer too small for buffer_id=1 [33826:1295:0427/182322:VERBOSE3:gpu_video_encode_accelerator.cc(163)] NotifyError [33826:1295:0427/182322:VERBOSE3:gpu_video_encode_accelerator.cc(343)] GpuVideoEncodeAccelerator::OnUseOutputBitstreamBuffer(): buffer_id=2, buffer_size=25344 [33826:1295:0427/182322:ERROR:gpu_video_encode_accelerator.cc(355)] GpuVideoEncodeAccelerator::OnUseOutputBitstreamBuffer(): 25344,921600buffer too small for buffer_id=2 [33826:1295:0427/182322:VERBOSE3:gpu_video_encode_accelerator.cc(163)] NotifyError [33826:1295:0427/182322:VERBOSE3:gpu_video_encode_accelerator.cc(343)] GpuVideoEncodeAccelerator::OnUseOutputBitstreamBuffer(): buffer_id=3, buffer_size=921600 [33826:1295:0427/182322:VERBOSE3:vt_video_encode_accelerator_mac.cc(191)] UseOutputBitstreamBuffer: buffer size=921600 [33826:1295:0427/182322:VERBOSE3:gpu_video_encode_accelerator.cc(343)] GpuVideoEncodeAccelerator::OnUseOutputBitstreamBuffer(): buffer_id=4, buffer_size=921600 [33826:1295:0427/182322:VERBOSE3:vt_video_encode_accelerator_mac.cc(191)] UseOutputBitstreamBuffer: buffer size=921600 [33826:1295:0427/182322:VERBOSE3:gpu_video_encode_accelerator.cc(343)] GpuVideoEncodeAccelerator::OnUseOutputBitstreamBuffer(): buffer_id=5, buffer_size=921600 [33826:1295:0427/182322:VERBOSE3:vt_video_encode_accelerator_mac.cc(191)] UseOutputBitstreamBuffer: buffer size=921600 [33841:29711:0427/182322:WARNING:stunport.cc(393)] Jingle:Port[0x7fe10b0bde00:audio:1:0::Net[en0:2620:0:1000:1610::/64:Wifi]]: StunPort: stun host lookup received error 0 [33841:34307:0427/182322:VERBOSE3:rtc_video_encoder.cc(654)] RegisterEncodeCompleteCallback() [33841:39175:0427/182322:VERBOSE3:rtc_video_encoder.cc(788)] RegisterEncodeCompleteCallback() [33841:34307:0427/182322:VERBOSE2:gpu_video_encode_accelerator_host.cc(318)] OnNotifyError(): error=2
,
Apr 28 2016
Thanks, please do. Can you open a new bug and cc me for it? If you've any questions about how RTCVideoEncoder is called from webrtc/ let me know.
,
Apr 28 2016
I already put one up for review here: https://codereview.chromium.org/1928873002/
,
May 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/50f037b918776a78e40dad72a5f0e777179a5424 commit 50f037b918776a78e40dad72a5f0e777179a5424 Author: emircan <emircan@chromium.org> Date: Thu May 05 17:56:16 2016 Revert to recreating RTCVideoEncoder::Impl This CL revert to recreating RTCVideoEncoder::Impl each time RTCVideoEncoder::Impl::Destroy is called to avoid issues like https://bugs.chromium.org/p/chromium/issues/detail?id=595308#c17. BUG= 595308 Review-Url: https://codereview.chromium.org/1928873002 Cr-Commit-Position: refs/heads/master@{#391840} [modify] https://crrev.com/50f037b918776a78e40dad72a5f0e777179a5424/content/renderer/media/rtc_video_encoder.cc [modify] https://crrev.com/50f037b918776a78e40dad72a5f0e777179a5424/content/renderer/media/rtc_video_encoder.h |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by pbos@chromium.org
, Mar 16 2016