Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2105683002:
RtcVideoEncoder: use the timestamp from encoder. (Closed)
|
Created:
4 years, 5 months ago by wuchengli Modified:
4 years, 5 months ago CC:
chromium-reviews, posciak+watch_chromium.org, mlamouri+watch-content_chromium.org, feature-media-reviews_chromium.org, jam, darin-cc_chromium.org, pbos, shenghao Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRtcVideoEncoder: use the timestamp from encoder.
Fallback to the current time if the timestamp from the encoder
callback is 0.
BUG=chromium:620565
TEST=Run apprtc loopback on oak.
Committed: https://crrev.com/a31c66943657af646c8baaaab4ce42bb0f650931
Cr-Commit-Position: refs/heads/master@{#403423}
Patch Set 1 #Patch Set 2 : use the timestamp from native handle #
Total comments: 2
Patch Set 3 : address Shenghao's comment and run git cl format #
Total comments: 6
Patch Set 4 : rebase #Patch Set 5 : address mcasas' comments #Messages
Total messages: 29 (13 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== RtcVideoEncoder: use the timestamp from encoder. Fallback to the current time if the timestamp from the encoder callback is 0. BUG=chromium: TEST=Run apprtc loopback on oak and cyan. ========== to ========== RtcVideoEncoder: use the timestamp from encoder. Fallback to the current time if the timestamp from the encoder callback is 0. BUG=chromium: TEST=Run apprtc loopback on oak. ==========
wuchengli@chromium.org changed reviewers: + mcasas@chromium.org, posciak@chromium.org
pbos@chromium.org changed reviewers: + pbos@chromium.org
lgtm, provided that implementations provide 0ms as default. Also put a BUG= in the description (it said chromium:)
I found we need to use the timestamp from native handle if we are using native handle. I'll upload a new patchset
Description was changed from ========== RtcVideoEncoder: use the timestamp from encoder. Fallback to the current time if the timestamp from the encoder callback is 0. BUG=chromium: TEST=Run apprtc loopback on oak. ========== to ========== RtcVideoEncoder: use the timestamp from encoder. Fallback to the current time if the timestamp from the encoder callback is 0. BUG=chromium:620565 TEST=Run apprtc loopback on oak. ==========
I won't be able to test the new patchset from home. I'll upload the new patchset tomorrow.
PTAL. I also added BUG= in the change description.
wuchengli@chromium.org changed reviewers: + shenghao@chromium.org
shenghao@google.com changed reviewers: + shenghao@google.com
https://codereview.chromium.org/2105683002/diff/40001/content/renderer/media/... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/2105683002/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:574: base::TimeDelta timestamp; Consider to change to: base::TimeDelta timestamp = frame ? frame->timestamp() : base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms());
https://codereview.chromium.org/2105683002/diff/40001/content/renderer/media/... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/2105683002/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:574: base::TimeDelta timestamp; On 2016/06/29 08:39:17, shenghao1 wrote: > Consider to change to: > base::TimeDelta timestamp = frame ? frame->timestamp() : > base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms()); Done. I also ran git cl format.
lgtm
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
lgtm
Miguel. Owner review. Thanks.
RS LGTM with a few ideas. https://codereview.chromium.org/2105683002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/2105683002/diff/60001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:482: rtp_timestamp = static_cast<uint32_t>(timestamp.InMilliseconds()) * 90; nit: s/timestamp.InMilliseconds()/capture_time_ms/ to avoid redundant function call and calculation. https://codereview.chromium.org/2105683002/diff/60001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:487: rtp_timestamp = static_cast<uint32_t>( nit: We'd probably want |rtp_timestamp| to wrap around if |capture_time_us| is too large, right? (here and in l.482) If yes: add a comment for that ? If no: consider using base::saturatedcast(): https://cs.chromium.org/chromium/src/base/numerics/safe_conversions.h?dr=C&q=... https://codereview.chromium.org/2105683002/diff/60001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:573: base::TimeDelta timestamp = nit: const
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2105683002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/2105683002/diff/60001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:482: rtp_timestamp = static_cast<uint32_t>(timestamp.InMilliseconds()) * 90; On 2016/06/29 14:55:50, mcasas wrote: > nit: s/timestamp.InMilliseconds()/capture_time_ms/ > to avoid redundant function call and calculation. Now I used the same calculation for both cases because I found we may lose precision if we use milliseconds * 90. microseconds * 90 / 1000 is better. https://codereview.chromium.org/2105683002/diff/60001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:487: rtp_timestamp = static_cast<uint32_t>( On 2016/06/29 14:55:50, mcasas wrote: > nit: We'd probably want |rtp_timestamp| to wrap > around if |capture_time_us| is too large, right? > (here and in l.482) If yes: add a comment for > that ? If no: consider using base::saturatedcast(): > > https://cs.chromium.org/chromium/src/base/numerics/safe_conversions.h?dr=C&q=... Yes. We want RTC timestamp to wrap around. I added the comment. From book "RTP: Audio and Video for the Internet": Timestamp wrap-around is a normal part of RTP operation and should be handled by all applications. https://codereview.chromium.org/2105683002/diff/60001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:573: base::TimeDelta timestamp = On 2016/06/29 14:55:50, mcasas wrote: > nit: const Done.
On 2016/07/01 06:19:09, wuchengli wrote: > https://codereview.chromium.org/2105683002/diff/60001/content/renderer/media/... > File content/renderer/media/rtc_video_encoder.cc (right): > > https://codereview.chromium.org/2105683002/diff/60001/content/renderer/media/... > content/renderer/media/rtc_video_encoder.cc:482: rtp_timestamp = > static_cast<uint32_t>(timestamp.InMilliseconds()) * 90; > On 2016/06/29 14:55:50, mcasas wrote: > > nit: s/timestamp.InMilliseconds()/capture_time_ms/ > > to avoid redundant function call and calculation. > Now I used the same calculation for both cases because I found we may lose > precision if we use milliseconds * 90. microseconds * 90 / 1000 is better. > > https://codereview.chromium.org/2105683002/diff/60001/content/renderer/media/... > content/renderer/media/rtc_video_encoder.cc:487: rtp_timestamp = > static_cast<uint32_t>( > On 2016/06/29 14:55:50, mcasas wrote: > > nit: We'd probably want |rtp_timestamp| to wrap > > around if |capture_time_us| is too large, right? > > (here and in l.482) If yes: add a comment for > > that ? If no: consider using base::saturatedcast(): > > > > > https://cs.chromium.org/chromium/src/base/numerics/safe_conversions.h?dr=C&q=... > Yes. We want RTC timestamp to wrap around. I added the comment. > > From book "RTP: Audio and Video for the Internet": > Timestamp wrap-around is a normal part of RTP operation and should be handled by > all applications. > > https://codereview.chromium.org/2105683002/diff/60001/content/renderer/media/... > content/renderer/media/rtc_video_encoder.cc:573: base::TimeDelta timestamp = > On 2016/06/29 14:55:50, mcasas wrote: > > nit: const > > Done. lgtm
The CQ bit was checked by wuchengli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@chromium.org, mcasas@chromium.org, pbos@webrtc.org Link to the patchset: https://codereview.chromium.org/2105683002/#ps120001 (title: "address mcasas' comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== RtcVideoEncoder: use the timestamp from encoder. Fallback to the current time if the timestamp from the encoder callback is 0. BUG=chromium:620565 TEST=Run apprtc loopback on oak. ========== to ========== RtcVideoEncoder: use the timestamp from encoder. Fallback to the current time if the timestamp from the encoder callback is 0. BUG=chromium:620565 TEST=Run apprtc loopback on oak. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== RtcVideoEncoder: use the timestamp from encoder. Fallback to the current time if the timestamp from the encoder callback is 0. BUG=chromium:620565 TEST=Run apprtc loopback on oak. ========== to ========== RtcVideoEncoder: use the timestamp from encoder. Fallback to the current time if the timestamp from the encoder callback is 0. BUG=chromium:620565 TEST=Run apprtc loopback on oak. Committed: https://crrev.com/a31c66943657af646c8baaaab4ce42bb0f650931 Cr-Commit-Position: refs/heads/master@{#403423} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a31c66943657af646c8baaaab4ce42bb0f650931 Cr-Commit-Position: refs/heads/master@{#403423} |
