Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1345473002:
Reset frame timestamp epoch for new capturers. (Closed)
|
Created:
5 years, 3 months ago by pbos-webrtc Modified:
5 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), Andrew MacDonald, tnakamura-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReset frame timestamp epoch for new capturers.
Incoming frames usually have an epoch of time since the capturer was
created or similar, not any fixed-time epoch. As such, setting a new
capturer resulted in delivering frames with older timestamps which
caused these frames to be dropped before encoding.
BUG=webrtc:4994
R=stefan@webrtc.org
TBR=pthatcher@webrtc.org
Committed: https://crrev.com/1cb121dea478a4bb4f88e76cf92719e2853543cf
Cr-Commit-Position: refs/heads/master@{#9934}
Patch Set 1 #
Total comments: 2
Patch Set 2 : test fix #Patch Set 3 : uint32 cast #
Total comments: 2
Messages
Total messages: 25 (8 generated)
PTAL
The CQ bit was checked by pbos@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/1345473002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1345473002/1
LGTM with my nit fixed. https://codereview.webrtc.org/1345473002/diff/1/talk/media/webrtc/webrtcvideo... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1345473002/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2_unittest.cc:528: frame.time_stamp = kInitialTimestamp; Make this smaller than kInitialTimestamp to clarify that it avoids producing super old frames.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/9322)
test fix
uint32 cast
pbos@webrtc.org changed reviewers: + pthatcher@webrtc.org
TBR=pthatcher@ for talk/media/base
The CQ bit was checked by pbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1345473002/#ps40001 (title: "uint32 cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1345473002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1345473002/40001
https://codereview.webrtc.org/1345473002/diff/1/talk/media/webrtc/webrtcvideo... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1345473002/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2_unittest.cc:528: frame.time_stamp = kInitialTimestamp; On 2015/09/14 15:21:49, stefan-webrtc (holmer) wrote: > Make this smaller than kInitialTimestamp to clarify that it avoids producing > super old frames. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by tnakamura@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1345473002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1345473002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1cb121dea478a4bb4f88e76cf92719e2853543cf Cr-Commit-Position: refs/heads/master@{#9934}
Message was sent while issue was closed.
mflodman@webrtc.org changed reviewers: + mflodman@webrtc.org
Message was sent while issue was closed.
This still assumed certain behavior of the incoming frame timestamp. Do we know if that holds? I'd prefer to extend checks here, let's discuss.
Message was sent while issue was closed.
https://codereview.webrtc.org/1345473002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1345473002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:519: rtc::Thread::Current()->SleepMs(1000); If you have to do a real sleep, can you at least make it shorter?
Message was sent while issue was closed.
https://codereview.webrtc.org/1345473002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1345473002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:519: rtc::Thread::Current()->SleepMs(1000); On 2015/09/17 05:00:31, pthatcher1 wrote: > If you have to do a real sleep, can you at least make it shorter? It's a bit longer due to incoming frames being stamped with a real clock which can't be injected and I didn't want this test to flakily pass on slower bots if there is a bug because threads are scheduled to run later.
Message was sent while issue was closed.
On 2015/09/17 06:36:20, pbos-webrtc wrote: > https://codereview.webrtc.org/1345473002/diff/40001/talk/media/webrtc/webrtcv... > File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): > > https://codereview.webrtc.org/1345473002/diff/40001/talk/media/webrtc/webrtcv... > talk/media/webrtc/webrtcvideoengine2_unittest.cc:519: > rtc::Thread::Current()->SleepMs(1000); > On 2015/09/17 05:00:31, pthatcher1 wrote: > > If you have to do a real sleep, can you at least make it shorter? > > It's a bit longer due to incoming frames being stamped with a real clock which > can't be injected and I didn't want this test to flakily pass on slower bots if > there is a bug because threads are scheduled to run later. LGTM for webrtcvideoengine2.cc, but please finish the discussion with Peter re the test. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
