Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2071953003:
RtcVideoEncoder: use the current timestamp for encoded frames. (Closed)
|
Created:
4 years, 6 months ago by wuchengli Modified:
4 years, 6 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_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 current timestamp for encoded frames.
Nyan provides invalid timestamps in frame callback. Change back
to the current time before it is fixed.
BUG=chromium:620676
TEST=Run apprtc loopback and VEA test on nyan-big and oak.
Committed: https://crrev.com/95f4096226a6def5622a4d51634acb5e5b808451
Cr-Commit-Position: refs/heads/master@{#400415}
Patch Set 1 #Patch Set 2 : RtcVideoEncoder: use the current timestamp for encoded frames. #Patch Set 3 : update the comment #
Total comments: 6
Patch Set 4 : address mcasas' comments #Patch Set 5 : change to kMicrosecondsPerMillisecond #Messages
Total messages: 32 (18 generated)
wuchengli@chromium.org changed reviewers: + posciak@chromium.org
PTAL. FYI. The original Shenghao's patch is https://codereview.chromium.org/1996453003/diff/200001/content/renderer/media.... You can compare.
Description was changed from ========== RtcVideoEncoder: fallback if encoder does not provide a timestamp. Some encoders do not provide a timestamp in frame callback. Fallback to the current time if timestamp is equal or less than 0. BUG=chromium:620565 TEST=Run apprtc loopback on nyan-big and oak. ========== to ========== RtcVideoEncoder: use the current timestamp for encoded frames. Nyan provides invalid timestamps in frame callback. Change back to the current time before it is fixed. BUG=chromium:620565 TEST=Run apprtc loopback on nyan-big and oak. ==========
Description was changed from ========== RtcVideoEncoder: use the current timestamp for encoded frames. Nyan provides invalid timestamps in frame callback. Change back to the current time before it is fixed. BUG=chromium:620565 TEST=Run apprtc loopback on nyan-big and oak. ========== to ========== RtcVideoEncoder: use the current timestamp for encoded frames. Nyan provides invalid timestamps in frame callback. Change back to the current time before it is fixed. BUG=chromium:620565 TEST=Run apprtc loopback and VEA test on nyan-big and oak. ==========
lgtm
The CQ bit was checked by wuchengli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2071953003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by wuchengli@chromium.org
Description was changed from ========== RtcVideoEncoder: use the current timestamp for encoded frames. Nyan provides invalid timestamps in frame callback. Change back to the current time before it is fixed. BUG=chromium:620565 TEST=Run apprtc loopback and VEA test on nyan-big and oak. ========== to ========== RtcVideoEncoder: use the current timestamp for encoded frames. Nyan provides invalid timestamps in frame callback. Change back to the current time before it is fixed. BUG=chromium:620676 TEST=Run apprtc loopback and VEA test on nyan-big and oak. ==========
The CQ bit was checked by wuchengli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2071953003/40001
The CQ bit was unchecked by wuchengli@chromium.org
wuchengli@chromium.org changed reviewers: + mcasas@chromium.org
mcasas@ OWNER review. Thanks.
The CQ bit was checked by wuchengli@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/2071953003/40001
LGTM assuming rtc::TimeMicros() works as expected :) https://codereview.chromium.org/2071953003/diff/40001/content/renderer/media/... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/2071953003/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:479: // fixed. Can you add a https://crbug.com/ to this comment for tracking? https://codereview.chromium.org/2071953003/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:483: const int64_t capture_time_ms = capture_time_us / 1000; s/1000/base::Time::kNanosecondsPerMicrosecond/ from https://cs.chromium.org/chromium/src/base/time/time.h?q=kMilliseconds&sq=pack... https://codereview.chromium.org/2071953003/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:486: static_cast<uint32_t>(capture_time_us * 90 / 1000); Same here
Are you travelling? It's pretty late in MTV. :) Thanks for the quick review. https://codereview.chromium.org/2071953003/diff/40001/content/renderer/media/... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/2071953003/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:479: // fixed. On 2016/06/17 10:32:42, mcasas wrote: > Can you add a https://crbug.com/ to this comment for tracking? Done. PS1 has a http://crosbug.com/p/ bug. I just realized I can use 620565 to track this. https://codereview.chromium.org/2071953003/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:483: const int64_t capture_time_ms = capture_time_us / 1000; On 2016/06/17 10:32:42, mcasas wrote: > s/1000/base::Time::kNanosecondsPerMicrosecond/ from > > https://cs.chromium.org/chromium/src/base/time/time.h?q=kMilliseconds&sq=pack... Done. https://codereview.chromium.org/2071953003/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:486: static_cast<uint32_t>(capture_time_us * 90 / 1000); On 2016/06/17 10:32:42, mcasas wrote: > Same here Done.
rtc::TimeMicros() should work because that was the original code. I just reverted the change.
The CQ bit was checked by wuchengli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2071953003/#ps60001 (title: "address mcasas' comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2071953003/60001
The CQ bit was unchecked by wuchengli@chromium.org
The CQ bit was checked by wuchengli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2071953003/#ps80001 (title: "change to kMicrosecondsPerMillisecond")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2071953003/80001
Message was sent while issue was closed.
Description was changed from ========== RtcVideoEncoder: use the current timestamp for encoded frames. Nyan provides invalid timestamps in frame callback. Change back to the current time before it is fixed. BUG=chromium:620676 TEST=Run apprtc loopback and VEA test on nyan-big and oak. ========== to ========== RtcVideoEncoder: use the current timestamp for encoded frames. Nyan provides invalid timestamps in frame callback. Change back to the current time before it is fixed. BUG=chromium:620676 TEST=Run apprtc loopback and VEA test on nyan-big and oak. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== RtcVideoEncoder: use the current timestamp for encoded frames. Nyan provides invalid timestamps in frame callback. Change back to the current time before it is fixed. BUG=chromium:620676 TEST=Run apprtc loopback and VEA test on nyan-big and oak. ========== to ========== RtcVideoEncoder: use the current timestamp for encoded frames. Nyan provides invalid timestamps in frame callback. Change back to the current time before it is fixed. BUG=chromium:620676 TEST=Run apprtc loopback and VEA test on nyan-big and oak. Committed: https://crrev.com/95f4096226a6def5622a4d51634acb5e5b808451 Cr-Commit-Position: refs/heads/master@{#400415} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/95f4096226a6def5622a4d51634acb5e5b808451 Cr-Commit-Position: refs/heads/master@{#400415} |
