Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(14)

Issue 2692853009: Set capture_time_ms using rtc::TimeMicros() (Closed)

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.

Description

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/+/1dd4ccf354c3286647e35ed6e02c36de695901fc

Patch Set 1 #

Total comments: 4

Patch Set 2 : #

Patch Set 3 : #

Total comments: 6

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -15 lines) Patch
M content/renderer/media/gpu/rtc_video_encoder.cc View 1 2 3 3 chunks +21 lines, -15 lines 0 comments Download

Messages

Total messages: 46 (25 generated)
emircan
PTAL.
3 years, 10 months ago (2017-02-14 19:56:17 UTC) #3
emircan
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/source/rtp_sender_video.cc?rcl=149997cfe303770079d5b4887e937f80a2b5c46b&l=302 Would setting it ...
3 years, 10 months ago (2017-02-14 20:06:46 UTC) #4
nisse-chromium (ooo August 14)
On 2017/02/14 20:06:46, emircan wrote: > I just realized that the returned capture_time_ms is also ...
3 years, 10 months ago (2017-02-15 07:46:45 UTC) #5
nisse-chromium (ooo August 14)
+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/rtc_video_encoder.cc File ...
3 years, 10 months ago (2017-02-15 07:53:18 UTC) #7
Stefan
https://codereview.chromium.org/2692853009/diff/1/content/renderer/media/gpu/rtc_video_encoder.cc File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2692853009/diff/1/content/renderer/media/gpu/rtc_video_encoder.cc#newcode471 content/renderer/media/gpu/rtc_video_encoder.cc:471: On 2017/02/15 07:53:18, nisse-chromium wrote: > I'd suggest reading ...
3 years, 10 months ago (2017-02-15 12:31:24 UTC) #9
emircan
I made the changes to use rtc::TimeMicros() and updated CL description. https://codereview.chromium.org/2692853009/diff/1/content/renderer/media/gpu/rtc_video_encoder.cc File content/renderer/media/gpu/rtc_video_encoder.cc (right): ...
3 years, 10 months ago (2017-02-15 19:35:34 UTC) #15
nisse-chromium (ooo August 14)
lgtm
3 years, 10 months ago (2017-02-16 07:43:50 UTC) #21
Stefan
lgtm as this fixes the immediate problem, but: Have you compared to how we do ...
3 years, 10 months ago (2017-02-16 08:18:34 UTC) #22
emircan
On 2017/02/16 08:18:34, Stefan wrote: > lgtm as this fixes the immediate problem, but: > ...
3 years, 10 months ago (2017-02-16 17:45:49 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2692853009/60001
3 years, 10 months ago (2017-02-16 17:47:01 UTC) #25
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full ...
3 years, 10 months ago (2017-02-16 17:47:04 UTC) #27
emircan
mcasas@, can you RS review?
3 years, 10 months ago (2017-02-16 17:51:42 UTC) #29
mcasas
Some comments. Feeling a bit uneasy with all that truncating and mixing int64_t, uint64_t and ...
3 years, 10 months ago (2017-02-16 19:10:15 UTC) #30
emircan
https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/gpu/rtc_video_encoder.cc File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/gpu/rtc_video_encoder.cc#newcode237 content/renderer/media/gpu/rtc_video_encoder.cc:237: // capture_time_ms_ field of the last returned webrtc::EncodedImage from ...
3 years, 10 months ago (2017-02-16 19:41:55 UTC) #32
mcasas
On 2017/02/16 19:41:55, emircan wrote: > https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/gpu/rtc_video_encoder.cc > File content/renderer/media/gpu/rtc_video_encoder.cc (right): > > https://codereview.chromium.org/2692853009/diff/60001/content/renderer/media/gpu/rtc_video_encoder.cc#newcode237 > ...
3 years, 10 months ago (2017-02-16 19:51:43 UTC) #34
emircan
On 2017/02/16 19:51:43, mcasas wrote: > Since unittests are landed in https://crrev.com/2435693004, > I won't ...
3 years, 10 months ago (2017-02-16 19:55:00 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2692853009/80001
3 years, 10 months ago (2017-02-17 00:36:53 UTC) #40
commit-bot: I haz the power
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1dd4ccf354c3286647e35ed6e02c36de695901fc
3 years, 10 months ago (2017-02-17 01:58:38 UTC) #43
Stefan
On 2017/02/16 08:18:34, Stefan wrote: > lgtm as this fixes the immediate problem, but: > ...
3 years, 10 months ago (2017-02-20 14:57:23 UTC) #44
emircan
On 2017/02/20 14:57:23, Stefan wrote: > On 2017/02/16 08:18:34, Stefan wrote: > > lgtm as ...
3 years, 10 months ago (2017-02-20 19:51:40 UTC) #45
Stefan
3 years, 10 months ago (2017-02-21 15:34:00 UTC) #46
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. :)

Powered by Google App Engine
This is Rietveld 408576698