|
|
Created:
3 years, 10 months ago by emircan Modified:
3 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet capture_time_ms using rtc::TimeMicros()
capture_time_ms should be based on system clock and setting it by using camera capture
timestamp may cause sync issue, i.e. the bug below. Set this field using rtc::TimeMicros().
BUG=webrtc:7045
Review-Url: https://codereview.chromium.org/2692853009
Cr-Commit-Position: refs/heads/master@{#451171}
Committed: https://chromium.googlesource.com/chromium/src/+/1dd4ccf354c3286647e35ed6e02c36de695901fc
Patch Set 1 #
Total comments: 4
Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #Messages
Total messages: 46 (25 generated)
Description was changed from ========== leave capture time to webrtc. BUG= ========== to ========== Leave setting capture_time_ms to rtp code capture_time_ms should be based on system clock and setting it by using camera capture timestamp may cause sync issue, i.e. the bug below. Set this field to -1 to leave it to rtp code in webrtc. BUG=webrtc:7045 ==========
emircan@chromium.org changed reviewers: + nisse@chromium.org
PTAL.
I just realized that the returned capture_time_ms is also used here: https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour... Would setting it to -1 cause an issue there? If yes, we can just set it to current system time.
On 2017/02/14 20:06:46, emircan wrote: > I just realized that the returned capture_time_ms is also used here: > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour... > > > Would setting it to -1 cause an issue there? If yes, we can just set it to > current system time. I honestly don't know how capture time is sent over the wire and what it's used for. I think audio sets it to -1, so probably not essential. But since the capture time is intended to be close to the time the frame was captured, it seems better to set it early.
nisse@chromium.org changed reviewers: + pbos@chromium.org
+pbos, do you know what capture_time_ms in the rtp header is used for? https://codereview.chromium.org/2692853009/diff/1/content/renderer/media/gpu/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2692853009/diff/1/content/renderer/media/gpu/... content/renderer/media/gpu/rtc_video_encoder.cc:471: I'd suggest reading system clock up front, int64_t current_time_us = rtc::TimeMicros(); and then use this value both for the fallback and for setting image.capture_time.
holmer@chromium.org changed reviewers: + holmer@chromium.org
https://codereview.chromium.org/2692853009/diff/1/content/renderer/media/gpu/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2692853009/diff/1/content/renderer/media/gpu/... content/renderer/media/gpu/rtc_video_encoder.cc:471: On 2017/02/15 07:53:18, nisse-chromium wrote: > I'd suggest reading system clock up front, > > int64_t current_time_us = rtc::TimeMicros(); > > and then use this value both for the fallback and for setting > image.capture_time. You should then also make sure that two frames don't get the same timestamp (make sure there's at least 1 ms difference or similar). Two frames with equal timestamps can confuse the RTP layers.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Leave setting capture_time_ms to rtp code capture_time_ms should be based on system clock and setting it by using camera capture timestamp may cause sync issue, i.e. the bug below. Set this field to -1 to leave it to rtp code in webrtc. BUG=webrtc:7045 ========== to ========== Set capture_time_ms using rtc::TimeMicros() capture_time_ms should be based on system clock and setting it by using camera capture timestamp may cause sync issue, i.e. the bug below. Set this field using rtc::TimeMicros(). BUG=webrtc:7045 ==========
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
I made the changes to use rtc::TimeMicros() and updated CL description. https://codereview.chromium.org/2692853009/diff/1/content/renderer/media/gpu/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2692853009/diff/1/content/renderer/media/gpu/... content/renderer/media/gpu/rtc_video_encoder.cc:471: On 2017/02/15 07:53:18, nisse-chromium wrote: > I'd suggest reading system clock up front, > > int64_t current_time_us = rtc::TimeMicros(); > > and then use this value both for the fallback and for setting > image.capture_time. Done. https://codereview.chromium.org/2692853009/diff/1/content/renderer/media/gpu/... content/renderer/media/gpu/rtc_video_encoder.cc:471: On 2017/02/15 12:31:24, Stefan wrote: > On 2017/02/15 07:53:18, nisse-chromium wrote: > > I'd suggest reading system clock up front, > > > > int64_t current_time_us = rtc::TimeMicros(); > > > > and then use this value both for the fallback and for setting > > image.capture_time. > > You should then also make sure that two frames don't get the same timestamp > (make sure there's at least 1 ms difference or similar). Two frames with equal > timestamps can confuse the RTP layers. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm as this fixes the immediate problem, but: Have you compared to how we do this on iOS? https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/objc/Framework/Cl... There we actually pass a FrameEncodeParams object to VTCompressionSessionEncodeFrame() and retrieve it in the VTCompressionOutputCallback(). That looks like a better solution to me. Is that not an option here? I can tell that this code is a bit more complicated, but do you think that makes sense as our long-term solution?
On 2017/02/16 08:18:34, Stefan wrote: > lgtm as this fixes the immediate problem, but: > > Have you compared to how we do this on iOS? > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/objc/Framework/Cl... > > There we actually pass a FrameEncodeParams object to > VTCompressionSessionEncodeFrame() and retrieve it in the > VTCompressionOutputCallback(). That looks like a better solution to me. Is that > not an option here? I can tell that this code is a bit more complicated, but do > you think that makes sense as our long-term solution? We do the exact same thing for Mac. However, some platforms, i.e. Android, don't preserve the timestamp. You can see the discussion here about carrying rtp_timestamp over, https://codereview.chromium.org/2435693004/#msg48. If that CL sticks, we can also try to carry render_time_ms(). Setting it to current time would fix the bug for now and be consistent all over, also let us merge back quickly. https://cs.chromium.org/chromium/src/media/gpu/vt_video_encode_accelerator_ma...
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
emircan@chromium.org changed reviewers: + mcasas@chromium.org
mcasas@, can you RS review?
Some comments. Feeling a bit uneasy with all that truncating and mixing int64_t, uint64_t and uint32_t. What about some testing...? https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:237: // capture_time_ms_ field of the last returned webrtc::EncodedImage from nit : |capture_time_ms_| https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:479: const int64_t capture_time_us = rtc::TimeMicros(); Could we do all these calcuations in ms and use rtc::TimeMillis() here instead? Or would that lose precious precision? https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:483: capture_time_ms = last_capture_time_ms_ + 1; Maybe capture_time_ms = std::min(capture_time_ms, last_capture_time_ms_ + 1); ? (Assuming it's OK to use the updated value of |capture_time_ms| in l.488).
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:237: // capture_time_ms_ field of the last returned webrtc::EncodedImage from On 2017/02/16 19:10:15, mcasas wrote: > nit : |capture_time_ms_| Done. https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:479: const int64_t capture_time_us = rtc::TimeMicros(); On 2017/02/16 19:10:15, mcasas wrote: > Could we do all these calcuations in ms and use > rtc::TimeMillis() here instead? Or would that lose > precious precision? It would lose the precision in |rtp_timestamp| calculations as we are converting to 90 kHz there. I don't want to change anything about |rtp_timestamp| in this CL. https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:483: capture_time_ms = last_capture_time_ms_ + 1; On 2017/02/16 19:10:15, mcasas wrote: > Maybe > capture_time_ms = std::min(capture_time_ms, last_capture_time_ms_ + 1); > ? > (Assuming it's OK to use the updated value of > |capture_time_ms| in l.488). That wouldn't work. Suppose there is a rapid succession of 3 calls within the same P ms. They would be P, P, P. We want them to be P, P+1,P+2. std::max() would work though. Note that we still would want to use |capture_time_us| below as I explained before.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/16 19:41:55, emircan wrote: > https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/... > File content/renderer/media/gpu/rtc_video_encoder.cc (right): > > https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/... > content/renderer/media/gpu/rtc_video_encoder.cc:237: // capture_time_ms_ field > of the last returned webrtc::EncodedImage from > On 2017/02/16 19:10:15, mcasas wrote: > > nit : |capture_time_ms_| > > Done. > > https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/... > content/renderer/media/gpu/rtc_video_encoder.cc:479: const int64_t > capture_time_us = rtc::TimeMicros(); > On 2017/02/16 19:10:15, mcasas wrote: > > Could we do all these calcuations in ms and use > > rtc::TimeMillis() here instead? Or would that lose > > precious precision? > > It would lose the precision in |rtp_timestamp| calculations as we are converting > to 90 kHz there. I don't want to change anything about |rtp_timestamp| in this > CL. > > https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/... > content/renderer/media/gpu/rtc_video_encoder.cc:483: capture_time_ms = > last_capture_time_ms_ + 1; > On 2017/02/16 19:10:15, mcasas wrote: > > Maybe > > capture_time_ms = std::min(capture_time_ms, last_capture_time_ms_ + 1); > > ? > > (Assuming it's OK to use the updated value of > > |capture_time_ms| in l.488). > > That wouldn't work. Suppose there is a rapid succession of 3 calls within the > same P ms. They would be P, P, P. We want them to be P, P+1,P+2. std::max() > would work though. > Note that we still would want to use |capture_time_us| below as I explained > before. Since unittests are landed in https://crrev.com/2435693004, I won't block this: RS lgtm But I find this code hard to parse... Maybe time for a refactoring? Surely the tests are comprehensive enough...
On 2017/02/16 19:51:43, mcasas wrote: > Since unittests are landed in https://crrev.com/2435693004, > I won't block this: RS lgtm > > But I find this code hard to parse... Maybe > time for a refactoring? Surely the tests are > comprehensive enough... Thanks. Feel free to drop comments on https://crrev.com/2435693004 about refactoring which does a lot more on this file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@chromium.org, holmer@chromium.org Link to the patchset: https://codereview.chromium.org/2692853009/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487291756985110, "parent_rev": "47ac662b73ab95e33844fe6e6a9ce06104708ebb", "commit_rev": "1dd4ccf354c3286647e35ed6e02c36de695901fc"}
Message was sent while issue was closed.
Description was changed from ========== Set capture_time_ms using rtc::TimeMicros() capture_time_ms should be based on system clock and setting it by using camera capture timestamp may cause sync issue, i.e. the bug below. Set this field using rtc::TimeMicros(). BUG=webrtc:7045 ========== to ========== Set capture_time_ms using rtc::TimeMicros() capture_time_ms should be based on system clock and setting it by using camera capture timestamp may cause sync issue, i.e. the bug below. Set this field using rtc::TimeMicros(). BUG=webrtc:7045 Review-Url: https://codereview.chromium.org/2692853009 Cr-Commit-Position: refs/heads/master@{#451171} Committed: https://chromium.googlesource.com/chromium/src/+/1dd4ccf354c3286647e35ed6e02c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1dd4ccf354c3286647e35ed6e02c...
Message was sent while issue was closed.
On 2017/02/16 08:18:34, Stefan wrote: > lgtm as this fixes the immediate problem, but: > > Have you compared to how we do this on iOS? > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/objc/Framework/Cl... > > There we actually pass a FrameEncodeParams object to > VTCompressionSessionEncodeFrame() and retrieve it in the > VTCompressionOutputCallback(). That looks like a better solution to me. Is that > not an option here? I can tell that this code is a bit more complicated, but do > you think that makes sense as our long-term solution? emircan, can you please comment on this?
Message was sent while issue was closed.
On 2017/02/20 14:57:23, Stefan wrote: > On 2017/02/16 08:18:34, Stefan wrote: > > lgtm as this fixes the immediate problem, but: > > > > Have you compared to how we do this on iOS? > > > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/objc/Framework/Cl... > > > > There we actually pass a FrameEncodeParams object to > > VTCompressionSessionEncodeFrame() and retrieve it in the > > VTCompressionOutputCallback(). That looks like a better solution to me. Is > that > > not an option here? I can tell that this code is a bit more complicated, but > do > > you think that makes sense as our long-term solution? > > emircan, can you please comment on this? See https://codereview.chromium.org/2692853009/#msg23.
Message was sent while issue was closed.
On 2017/02/20 19:51:40, emircan wrote: > On 2017/02/20 14:57:23, Stefan wrote: > > On 2017/02/16 08:18:34, Stefan wrote: > > > lgtm as this fixes the immediate problem, but: > > > > > > Have you compared to how we do this on iOS? > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/objc/Framework/Cl... > > > > > > There we actually pass a FrameEncodeParams object to > > > VTCompressionSessionEncodeFrame() and retrieve it in the > > > VTCompressionOutputCallback(). That looks like a better solution to me. Is > > that > > > not an option here? I can tell that this code is a bit more complicated, but > > do > > > you think that makes sense as our long-term solution? > > > > emircan, can you please comment on this? > > See https://codereview.chromium.org/2692853009/#msg23. Thank you! Sorry, I missed that comment. :) |