Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1225153002:
Let WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame carry the input frame's timestamp to out… (Closed)
|
Created:
5 years, 5 months ago by qiangchen Modified:
5 years, 5 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionLet WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame carry the input frame's timestamp to output frame.
Essentially we are carrying over the capture timestamp to the encoded frame sent out, so the frame lengths will contain no noise.
Committed: https://crrev.com/c27d89fdc6b33846ff06e8ca8bd03119d05c6530
Cr-Commit-Position: refs/heads/master@{#9597}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use millisecond unit #Patch Set 3 : Add Unittest #Patch Set 4 : Style Fix #
Total comments: 5
Patch Set 5 : Style Fix #Patch Set 6 : Black frame timestamp #
Total comments: 2
Patch Set 7 : Style Fix #Patch Set 8 : Add a comment #Patch Set 9 : Remove SendStream after unit test #
Total comments: 6
Patch Set 10 : Black frame timestamp modification #
Total comments: 6
Patch Set 11 : Style Fix and Comment Fix #
Messages
Total messages: 45 (15 generated)
qiangchen@chromium.org changed reviewers: + pbos@chromium.org
Welcome to add anyone familiar with this part of code to reviewer.
https://codereview.chromium.org/1225153002/diff/1/talk/media/webrtc/webrtcvid... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.chromium.org/1225153002/diff/1/talk/media/webrtc/webrtcvid... talk/media/webrtc/webrtcvideoengine2.cc:1732: // frame->GetTimeStamp is essentially a delta, align to system time GetTimeStamp(), align to webrtc time. https://codereview.chromium.org/1225153002/diff/1/talk/media/webrtc/webrtcvid... talk/media/webrtc/webrtcvideoengine2.cc:1735: webrtc::TickTime::MicrosecondTimestamp() * 1000 - frame->GetTimeStamp(); Pref using MillisecondTimestamp(), even though they should be based on the same time. That's what we correct with inside IncomingCapturedFrame.
Fixed https://codereview.chromium.org/1225153002/diff/1/talk/media/webrtc/webrtcvid... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.chromium.org/1225153002/diff/1/talk/media/webrtc/webrtcvid... talk/media/webrtc/webrtcvideoengine2.cc:1732: // frame->GetTimeStamp is essentially a delta, align to system time On 2015/07/09 15:13:07, pbos wrote: > GetTimeStamp(), align to webrtc time. Done. https://codereview.chromium.org/1225153002/diff/1/talk/media/webrtc/webrtcvid... talk/media/webrtc/webrtcvideoengine2.cc:1735: webrtc::TickTime::MicrosecondTimestamp() * 1000 - frame->GetTimeStamp(); On 2015/07/09 15:13:07, pbos wrote: > Pref using MillisecondTimestamp(), even though they should be based on the same > time. That's what we correct with inside IncomingCapturedFrame. Done.
pbos@webrtc.org changed reviewers: + pbos@webrtc.org - pbos@chromium.org
Any chance you could get an unittest in there? webrtcvideoengine2_unittest.cc I think we save the last input frame, but I can't be sure, so if you input 2 frames with a render timestamp that's x ms apart, your render ms should also be input with 2ms apart (and you could make sure that the NTP timestamp is unset). Sounds good?
On 2015/07/09 23:13:33, pbos-webrtc wrote: > Any chance you could get an unittest in there? webrtcvideoengine2_unittest.cc > > I think we save the last input frame, but I can't be sure, so if you input 2 > frames with a render timestamp that's x ms apart, your render ms should also be > input with 2ms apart (and you could make sure that the NTP timestamp is unset). > Sounds good? Sorry, 2x frames into WebRtcVideoEngine2, x ms apart. There should be 2 frames input into FakeCall (in webrtcvideoengine2_unittest.cc) that are also exactly x ms apart.
On 2015/07/09 23:13:33, pbos-webrtc wrote: > Any chance you could get an unittest in there? webrtcvideoengine2_unittest.cc > > I think we save the last input frame, but I can't be sure, so if you input 2 > frames with a render timestamp that's x ms apart, your render ms should also be > input with 2ms apart (and you could make sure that the NTP timestamp is unset). > Sounds good? NTP timestamp is set in VideoCaptureInput::IncomingCapturedFrame as long as we put a correct render_time_ms. webrtc::TickTime::MillisecondTimestamp() is not UnixTimestamp, but MONOTONIC_CLOCK time, which is reset to zero each time one reboots the computer. So rtc::UnixNanoTimestampToNTPMilliseconds does not work. The only correct way is to really measure a delta between NTP time and webrtc time, as is done in VideoCaptureInput::IncomingCapturedFrame. And I've tested, that VideoCaptureInput::IncomingCapturedFrame's conversion to NTP time is correct. So I think there is no need to redo another NTP time alignment in webrtcvideoengine2. I'll update the unittest.
On 2015/07/09 23:37:13, qiangchenC wrote: > On 2015/07/09 23:13:33, pbos-webrtc wrote: > > Any chance you could get an unittest in there? webrtcvideoengine2_unittest.cc > > > > I think we save the last input frame, but I can't be sure, so if you input 2 > > frames with a render timestamp that's x ms apart, your render ms should also > be > > input with 2ms apart (and you could make sure that the NTP timestamp is > unset). > > Sounds good? > > NTP timestamp is set in VideoCaptureInput::IncomingCapturedFrame as long as we > put > a correct render_time_ms. > > webrtc::TickTime::MillisecondTimestamp() is not UnixTimestamp, > but MONOTONIC_CLOCK time, which is reset to zero each time one reboots the > computer. > So rtc::UnixNanoTimestampToNTPMilliseconds does not work. The only correct way > is > to really measure a delta between NTP time and webrtc time, as is done in > VideoCaptureInput::IncomingCapturedFrame. And I've tested, that > VideoCaptureInput::IncomingCapturedFrame's conversion to NTP time is correct. So > I > think there is no need to redo another NTP time alignment in webrtcvideoengine2. > > I'll update the unittest. Sorry, I meant the render_time_ms, not the NTP timestamp. Make sure that ntp_time_ms doesn't get set though, otherwise the capturer input won't convert it properly but rather use the NTP timestamp (I think?).
On 2015/07/09 23:14:27, pbos-webrtc wrote: > On 2015/07/09 23:13:33, pbos-webrtc wrote: > > Any chance you could get an unittest in there? webrtcvideoengine2_unittest.cc > > > > I think we save the last input frame, but I can't be sure, so if you input 2 > > frames with a render timestamp that's x ms apart, your render ms should also > be > > input with 2ms apart (and you could make sure that the NTP timestamp is > unset). > > Sounds good? > > Sorry, 2x frames into WebRtcVideoEngine2, x ms apart. There should be 2 frames > input into FakeCall (in webrtcvideoengine2_unittest.cc) that are also exactly x > ms apart. Another question on unittest, can you tell me which build file should I look into? My problem is that I can't find which executable webrtcvideoengine2_unittest is under.
On 2015/07/09 23:50:13, qiangchenC wrote: > On 2015/07/09 23:14:27, pbos-webrtc wrote: > > On 2015/07/09 23:13:33, pbos-webrtc wrote: > > > Any chance you could get an unittest in there? > webrtcvideoengine2_unittest.cc > > > > > > I think we save the last input frame, but I can't be sure, so if you input 2 > > > frames with a render timestamp that's x ms apart, your render ms should also > > be > > > input with 2ms apart (and you could make sure that the NTP timestamp is > > unset). > > > Sounds good? > > > > Sorry, 2x frames into WebRtcVideoEngine2, x ms apart. There should be 2 frames > > input into FakeCall (in webrtcvideoengine2_unittest.cc) that are also exactly > x > > ms apart. > > Another question on unittest, can you tell me which build file should I look > into? > My problem is that I can't find which executable webrtcvideoengine2_unittest is > under. Are you building under chromium? We do webrtc development in the webrtc repository: http://www.webrtc.org/native-code/development#TOC-Getting-the-code talk/libjingle_tests.gyp builds libjingle_media_unittest which is the binary you want.
Did the unittest.
https://codereview.webrtc.org/1225153002/diff/60001/talk/media/webrtc/fakeweb... File talk/media/webrtc/fakewebrtccall.cc (right): https://codereview.webrtc.org/1225153002/diff/60001/talk/media/webrtc/fakeweb... talk/media/webrtc/fakewebrtccall.cc:116: return last_frame_.ntp_time_ms(); So long as we should never set NTP time, just DCHECK that ntp_time_ms() is zero. Do that on IncomingCapturedFrame. https://codereview.webrtc.org/1225153002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1225153002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:459: TEST_F(WebRtcVideoEngine2Test, SendStreamTimestampCarryOver) { PropagatesInputFrameTimestamp https://codereview.webrtc.org/1225153002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:496: EXPECT_LE(cricket::VideoFormat::FpsToInterval(60) / 1000000, interval); Is EXPECT_NEAR applicable here?
Fixed https://codereview.chromium.org/1225153002/diff/60001/talk/media/webrtc/webrt... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.chromium.org/1225153002/diff/60001/talk/media/webrtc/webrt... talk/media/webrtc/webrtcvideoengine2_unittest.cc:459: TEST_F(WebRtcVideoEngine2Test, SendStreamTimestampCarryOver) { On 2015/07/12 15:30:22, pbos-webrtc wrote: > PropagatesInputFrameTimestamp Done. https://codereview.chromium.org/1225153002/diff/60001/talk/media/webrtc/webrt... talk/media/webrtc/webrtcvideoengine2_unittest.cc:496: EXPECT_LE(cricket::VideoFormat::FpsToInterval(60) / 1000000, interval); On 2015/07/12 15:30:22, pbos-webrtc wrote: > Is EXPECT_NEAR applicable here? Done.
lgtm, but please land this after M45 cuts (I think). I'd like this to have some baking in Canary/Dev before going to Beta.
The CQ bit was checked by qiangchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225153002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/156)
One issue found during unit test. In VideoCaptureInput::IncomingCapturedFrame, we did a timestamp check and drop the frame whose timestamp is less than or equal to the last frame's timestamp. Here comes the problem for unittest WebRtcVideoChannel2BastTest::AddRemoveCapturer: When it generates the fake frames, it adds 33ms to the timestamp to each frame, but it does not really wait for 33ms. When SetCapturer to NULL, we generate a black frame with no timestamp, which will be interpreted as NOW being the timestamp. Thus the black frame's timestamp will be before the faked frames, and so the black frame is droped, and then the unittest that expects black frame to be rendered will fail. My current code is simplest to bypass this issue. But I think it is better to move the timestamp reversing check into WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame, and thus we can force the black frame to be rendered when capturer is set to NULL. What do you think?
lgtm, run git cl format. I think this is an OK way to make sure the last black frame gets sent. We want to keep the underlying check in the capture input, it still translate to an RTP timestamp, which might get dropped elsewhere or get something to flip out if we don't have increasing timestamps. https://codereview.webrtc.org/1225153002/diff/100001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1225153002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvideoengine2.cc:1740: last_timestamp_ms_=base_timestamp_ms_ + frame->GetTimeStamp() / 1000000; style (= -> " = "), run git cl format. https://codereview.webrtc.org/1225153002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvideoengine2.cc:1771: black_frame.set_render_time_ms(last_timestamp_ms_ + 1); Add a comment here, I think.
The CQ bit was checked by qiangchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.chromium.org/1225153002/#ps140001 (title: "Add a comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225153002/140001
pbos@chromium.org changed reviewers: + pbos@chromium.org
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/169)
pbos@webrtc.org changed reviewers: + pthatcher@webrtc.org - pbos@chromium.org
+R pthatcher@, can you stamp talk/media/base?
The CQ bit was checked by qiangchen@chromium.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/1225153002/160001
The CQ bit was unchecked by qiangchen@chromium.org
Patchset #9 (id:160001) has been deleted
https://codereview.webrtc.org/1225153002/diff/180001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1225153002/diff/180001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvideoengine2.cc:1741: video_frame.set_render_time_ms(last_timestamp_ms_); I think this would be more clear if you put "frame->GetTimeStamp() / 1000000" on its own line and used webrtc/base/timeutils.h instead of webrtc/system_wrappers/interface/tick_util.h: // frame->GetTimeStamp() is essentially a delta in nanoseconds uint64 frame_delta_ms_ = frame->GetTimeStamp() / rtc:: kNumNanosecsPerMillisec; if (base_timestamp_ms_ == 0) { base_timestamp_ms_ = rtc::Time() - frame_delta_ms_; } last_timestamp_ms_ = base_timestamp_ms_ + frame_delta_ms_; video_frame.set_render_time_ms(last_timestamp_ms_); https://codereview.webrtc.org/1225153002/diff/180001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvideoengine2.cc:1772: // check. Can you explain a little more how adding "+ 1" accomplishes what you want here? https://codereview.webrtc.org/1225153002/diff/180001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvideoengine2.h (right): https://codereview.webrtc.org/1225153002/diff/180001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvideoengine2.h:397: int64_t last_timestamp_ms_ GUARDED_BY(lock_); Will you please add comments explaining what these values are?
Patchset #10 (id:200001) has been deleted
Fixed. And it would be better to add an expected frame length to black frame's timestamp. https://codereview.chromium.org/1225153002/diff/180001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.chromium.org/1225153002/diff/180001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvideoengine2.cc:1741: video_frame.set_render_time_ms(last_timestamp_ms_); On 2015/07/14 17:57:28, pthatcher1 wrote: > I think this would be more clear if you put "frame->GetTimeStamp() / 1000000" on > its own line and used webrtc/base/timeutils.h instead of > webrtc/system_wrappers/interface/tick_util.h: > > // frame->GetTimeStamp() is essentially a delta in nanoseconds > uint64 frame_delta_ms_ = frame->GetTimeStamp() / rtc:: kNumNanosecsPerMillisec; > if (base_timestamp_ms_ == 0) { > base_timestamp_ms_ = rtc::Time() - frame_delta_ms_; > } > > last_timestamp_ms_ = base_timestamp_ms_ + frame_delta_ms_; > video_frame.set_render_time_ms(last_timestamp_ms_); Done. https://codereview.chromium.org/1225153002/diff/180001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvideoengine2.cc:1772: // check. On 2015/07/14 17:57:28, pthatcher1 wrote: > Can you explain a little more how adding "+ 1" accomplishes what you want here? Done. Add 1 could bypass the timestamp check in IncomingCapturedFrame, however, on the rendering side the frame would still be dropped because it is so close to previous frame. Add a expected frame length. https://codereview.chromium.org/1225153002/diff/180001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvideoengine2.h (right): https://codereview.chromium.org/1225153002/diff/180001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvideoengine2.h:397: int64_t last_timestamp_ms_ GUARDED_BY(lock_); On 2015/07/14 17:57:28, pthatcher1 wrote: > Will you please add comments explaining what these values are? Done.
https://codereview.chromium.org/1225153002/diff/220001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.chromium.org/1225153002/diff/220001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvideoengine2.cc:1737: base_timestamp_ms_ = rtc::Time() - frame_delta_ms; This makes it more clear that this value should probably be named first_frame_timestamp_ms_ https://codereview.chromium.org/1225153002/diff/220001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvideoengine2.cc:1740: last_timestamp_ms_ = base_timestamp_ms_ + frame_delta_ms; And this should be last_frame_timestamp_ms_. https://codereview.chromium.org/1225153002/diff/220001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvideoengine2.h (right): https://codereview.chromium.org/1225153002/diff/220001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvideoengine2.h:397: // To align the delta timestamp with rtc time I like the comment on the other value, and think this should have a similar one: // The timestamp of the first frame received // Used to generate the timestamps of subsequent frames
Fixed https://codereview.chromium.org/1225153002/diff/220001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.chromium.org/1225153002/diff/220001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvideoengine2.cc:1737: base_timestamp_ms_ = rtc::Time() - frame_delta_ms; On 2015/07/15 17:53:07, pthatcher1 wrote: > This makes it more clear that this value should probably be named > first_frame_timestamp_ms_ Done. https://codereview.chromium.org/1225153002/diff/220001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvideoengine2.cc:1740: last_timestamp_ms_ = base_timestamp_ms_ + frame_delta_ms; On 2015/07/15 17:53:07, pthatcher1 wrote: > And this should be last_frame_timestamp_ms_. Done. https://codereview.chromium.org/1225153002/diff/220001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvideoengine2.h (right): https://codereview.chromium.org/1225153002/diff/220001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvideoengine2.h:397: // To align the delta timestamp with rtc time On 2015/07/15 17:53:07, pthatcher1 wrote: > I like the comment on the other value, and think this should have a similar one: > > // The timestamp of the first frame received > // Used to generate the timestamps of subsequent frames Done.
lgtm
The CQ bit was checked by qiangchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, pbos@chromium.org Link to the patchset: https://codereview.chromium.org/1225153002/#ps240001 (title: "Style Fix and Comment Fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225153002/240001
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/c27d89fdc6b33846ff06e8ca8bd03119d05c6530 Cr-Commit-Position: refs/heads/master@{#9597} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
