Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1875713002:
New peerconnection test case, for video stream forwarding. (Closed)
|
Created:
4 years, 8 months ago by nisse-webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNew peerconnection test case, for video stream forwarding.
Split WebRtcVideoSendStream::OnFrame, to invoke encoder on the worker thread.
BUG=webrtc:5546
Committed: https://crrev.com/d98cf1ff1ee971dd74da3b5abac40ae6902cb984
Cr-Commit-Position: refs/heads/master@{#12471}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rebase, and drop the no-longer needed workerthread hack. #
Total comments: 6
Patch Set 3 : Address nits. #Messages
Total messages: 28 (10 generated)
Description was changed from ========== New peerconnection test case, for video stream forwarding. Split WebRtcVideoSendStream::OnFrame, to invoke encoder on the worker thread. BUG=webrtc::5546 ========== to ========== New peerconnection test case, for video stream forwarding. Split WebRtcVideoSendStream::OnFrame, to invoke encoder on the worker thread. BUG=webrtc:5546 ==========
nisse@webrtc.org changed reviewers: + pbos@webrtc.org, perkj@webrtc.org
This cl adds a test case for forwarding of remote video tracks. To make it actually work, had to add a thread jump in WebRtcVideoSendStream::OnFrame. I'm not entirely sure how these things really work, but the intention is that it should be a non-blocking post (since OnFrame is specified as fire-and-forget, that's perfect). And it has to be done after the conversion to webrtc::VideoFrame, since rtc::Bind isn't willing to handle the abstract class cricket::VideoFrame. I think the intention is that IncomingCapturedFrame should accept frames on any thread (bug number to link to?). But without the thread-jump part of this cl, the immediate crash was *not* in IncomingCapturedStream, but in RegisterSendCodec, called via stream_->ReconfigureVideoEncoder when the first frame arrives.
https://codereview.webrtc.org/1875713002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1875713002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1565: invoker_.AsyncInvoke<void>( I really don't want a thread hop here, I've CLs that should get rid of the requirement to run this on the worker thread that need to be relanded (hopefully early next week). Can we wait until that? I'm also a bit afraid that this invoke call could outlive this object (frame -> destroy stream -> invoke happens).
https://codereview.webrtc.org/1875713002/diff/1/webrtc/api/peerconnection_uni... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1875713002/diff/1/webrtc/api/peerconnection_uni... webrtc/api/peerconnection_unittest.cc:1997: LOG(INFO) << "XXXXX Echoing stream XXXXX"; remove this log https://codereview.webrtc.org/1875713002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1875713002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1565: invoker_.AsyncInvoke<void>( This can wait until then.... https://codereview.webrtc.org/1875713002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/1875713002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.h:359: void OnFrame_w(webrtc::VideoFrame video_frame, int64_t frame_delta_ms); no need to add a new method.
https://codereview.webrtc.org/1875713002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1875713002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1565: invoker_.AsyncInvoke<void>( On 2016/04/08 14:34:07, perkj_webrtc wrote: > This can wait until then.... Agreed. https://codereview.webrtc.org/1875713002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/1875713002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.h:359: void OnFrame_w(webrtc::VideoFrame video_frame, int64_t frame_delta_ms); On 2016/04/08 14:34:07, perkj_webrtc wrote: > no need to add a new method. I can't call rtc::Bind with a cricket::VideoFrame. Hence a new method with a different non-abstract argument type.
https://codereview.webrtc.org/1875713002/diff/1/webrtc/api/peerconnection_uni... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1875713002/diff/1/webrtc/api/peerconnection_uni... webrtc/api/peerconnection_unittest.cc:1997: LOG(INFO) << "XXXXX Echoing stream XXXXX"; On 2016/04/08 14:34:07, perkj_webrtc wrote: > remove this log Deleted locally, should be gone in next upload. https://codereview.webrtc.org/1875713002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1875713002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1565: invoker_.AsyncInvoke<void>( On 2016/04/08 14:08:56, pbos-webrtc wrote: > I really don't want a thread hop here, I've CLs that should get rid of the > requirement to run this on the worker thread that need to be relanded (hopefully > early next week). Can we wait until that? Can you cc me on that cl?
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1875713002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1875713002/20001
Now passes local testing, without the worker-thread hack in the video engine.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with the below at least considered. https://codereview.webrtc.org/1875713002/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1875713002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:1984: TEST_F(P2PTestConductor, StreamForwardVideoOnly) { Name ForwardVideoOnlyStream? https://codereview.webrtc.org/1875713002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2006: EXPECT_TRUE(receiving_client()->remote_streams()->count() == 1); nit: ASSERT_TRUE == 1 only.
rs-lgtm
On 2016/04/21 13:02:54, pbos-webrtc wrote: > rs-lgtm (Per knows this code better.)
https://codereview.webrtc.org/1875713002/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1875713002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:1995: ASSERT_TRUE (initializing_client()->can_receive_video()); git cl format please
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/1875713002/#ps40001 (title: "Address nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1875713002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1875713002/40001
https://codereview.webrtc.org/1875713002/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1875713002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:1984: TEST_F(P2PTestConductor, StreamForwardVideoOnly) { On 2016/04/21 08:18:28, perkj_webrtc wrote: > Name ForwardVideoOnlyStream? Done. https://codereview.webrtc.org/1875713002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:1995: ASSERT_TRUE (initializing_client()->can_receive_video()); On 2016/04/21 13:03:23, pbos-webrtc wrote: > git cl format please Done. https://codereview.webrtc.org/1875713002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2006: EXPECT_TRUE(receiving_client()->remote_streams()->count() == 1); On 2016/04/21 08:18:28, perkj_webrtc wrote: > nit: ASSERT_TRUE == 1 only. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1875713002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1875713002/40001
Message was sent while issue was closed.
Description was changed from ========== New peerconnection test case, for video stream forwarding. Split WebRtcVideoSendStream::OnFrame, to invoke encoder on the worker thread. BUG=webrtc:5546 ========== to ========== New peerconnection test case, for video stream forwarding. Split WebRtcVideoSendStream::OnFrame, to invoke encoder on the worker thread. BUG=webrtc:5546 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== New peerconnection test case, for video stream forwarding. Split WebRtcVideoSendStream::OnFrame, to invoke encoder on the worker thread. BUG=webrtc:5546 ========== to ========== New peerconnection test case, for video stream forwarding. Split WebRtcVideoSendStream::OnFrame, to invoke encoder on the worker thread. BUG=webrtc:5546 Committed: https://crrev.com/d98cf1ff1ee971dd74da3b5abac40ae6902cb984 Cr-Commit-Position: refs/heads/master@{#12471} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d98cf1ff1ee971dd74da3b5abac40ae6902cb984 Cr-Commit-Position: refs/heads/master@{#12471} |
