Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1888003002:
Drop dependency on webrtc's tick_utils.h. (Closed)
|
Created:
4 years, 8 months ago by nisse-chromium (ooo August 14) Modified:
4 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDrop dependency on webrtc's tick_utils.h.
Use webrtc's rtc::TimeMicros instead.
BUG=webrtc:5740
Committed: https://crrev.com/12f3628a481dbaa535437acc7d079a3f37bacf50
Cr-Commit-Position: refs/heads/master@{#388453}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 19 (9 generated)
nisse@chromium.org changed reviewers: + perkj@webrtc.org, qiangchen@chromium.org
I'm considering deleting tick_utils in webrtc, and I'd like to not have chrome depend on it. Not entirely sure rtc::TimeMicros is the right replacement, though. Should the code just use base::TimeTicks?
https://codereview.webrtc.org/1888003002/diff/1/content/renderer/media/rtc_vi... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.webrtc.org/1888003002/diff/1/content/renderer/media/rtc_vi... content/renderer/media/rtc_video_encoder.cc:425: const int64_t capture_time_us = rtc::TimeMicros(); IS the todo above easy to fix? This should come from the input frame and not be stamped here. This is a bug and leads to wrong timestamp at reconstruction of the frame on the receiving side. https://codereview.webrtc.org/1888003002/diff/1/content/renderer/media/webrtc... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.webrtc.org/1888003002/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc/media_stream_remote_video_source.cc:72: time_diff_(base::TimeTicks::Now() - base::TimeTicks() - Isn't all these methods from the same clock in the same base? ie- Isn't this always 0?
lgtm
nisse@webrtc.org changed reviewers: + nisse@webrtc.org
https://codereview.webrtc.org/1888003002/diff/1/content/renderer/media/rtc_vi... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.webrtc.org/1888003002/diff/1/content/renderer/media/rtc_vi... content/renderer/media/rtc_video_encoder.cc:425: const int64_t capture_time_us = rtc::TimeMicros(); On 2016/04/14 13:46:23, perkj_webrtc wrote: > IS the todo above easy to fix? This should come from the input frame and not be > stamped here. This is a bug and leads to wrong timestamp at reconstruction of > the frame on the receiving side. No idea really, but I don't think this cl should depend on that. pbos and jansson have commented on the bug recently. https://codereview.webrtc.org/1888003002/diff/1/content/renderer/media/webrtc... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.webrtc.org/1888003002/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc/media_stream_remote_video_source.cc:72: time_diff_(base::TimeTicks::Now() - base::TimeTicks() - On 2016/04/14 13:46:23, perkj_webrtc wrote: > Isn't all these methods from the same clock in the same base? ie- Isn't this > always 0? I haven't yet tried to understand time sources in chrome. Hopefully they're all clock_gettime(CLOCK_MONOTONIC), or some analogous clock on non-posix systems. time_diff_ was added in cl https://codereview.chromium.org/1320183003, it seems.
Description was changed from ========== Drop dependency on webrtc's tick_utils.h. Use webrtc's rtc::TimeMicros instead. BUG=webrtc:5740 ========== to ========== Drop dependency on webrtc's tick_utils.h. Use webrtc's rtc::TimeMicros instead. BUG=webrtc:5740 ==========
perkj@webrtc.org changed reviewers: + perkj@chromium.org - perkj@webrtc.org
lgtm
The CQ bit was checked by nisse@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1888003002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1888003002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by nisse@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1888003002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1888003002/1
Message was sent while issue was closed.
Description was changed from ========== Drop dependency on webrtc's tick_utils.h. Use webrtc's rtc::TimeMicros instead. BUG=webrtc:5740 ========== to ========== Drop dependency on webrtc's tick_utils.h. Use webrtc's rtc::TimeMicros instead. BUG=webrtc:5740 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Drop dependency on webrtc's tick_utils.h. Use webrtc's rtc::TimeMicros instead. BUG=webrtc:5740 ========== to ========== Drop dependency on webrtc's tick_utils.h. Use webrtc's rtc::TimeMicros instead. BUG=webrtc:5740 Committed: https://crrev.com/12f3628a481dbaa535437acc7d079a3f37bacf50 Cr-Commit-Position: refs/heads/master@{#388453} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/12f3628a481dbaa535437acc7d079a3f37bacf50 Cr-Commit-Position: refs/heads/master@{#388453} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
