Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2272873002:
Android video capture: only use reference time for outbound timestamp. (Closed)
|
Created:
4 years, 3 months ago by braveyao Modified:
4 years, 3 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid video capture: only use reference time for outbound timestamp.
on Android, video capture may be stopped/started when Chrome switches between
background and foreground. Each time when video capture is started, a new
instance of VideoCaptureDeviceAndroid will be created, which will change the
base value of "first_ref_time_". So the timestamp calculated by "current_time -
first_ref_time_" won't be monotonically increasing to the same video stream.
The consequence is frames will be dropped until the timestamp catches up to the
old one cached in video_capture_input.cc finally.
So on Android we should set the timestamp with 0 and use the reference time only.
BUG=640387
Committed: https://crrev.com/4a93aa65fbe67592f324b1ea45dcdc0342801f0d
Cr-Commit-Position: refs/heads/master@{#416814}
Patch Set 1 #Patch Set 2 : use raw timestamp from camera capture #
Total comments: 15
Patch Set 3 : rebase to get this branch compiled #Patch Set 4 : address comments #
Total comments: 2
Patch Set 5 : estimate a timestamp in Camera1 callback #
Messages
Total messages: 45 (30 generated)
The CQ bit was checked by braveyao@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...
braveyao@chromium.org changed reviewers: + qiangchen@chromium.org
Hi qiangchen@, please take a look.
The current code relies on the [1] to calculate timestamp from reference time. Ask miu@ for more opinions, as miu@ ever thought let all the devices to provide both timestamp and reference time, and thus we can actually remove [1]. [1] https://cs.chromium.org/chromium/src/content/renderer/media/video_capture_imp...
lgtm
braveyao@chromium.org changed reviewers: + miu@chromium.org
+ miu@
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by braveyao@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...
Hi qiangchen@ and miu@, The first patchset is to fallback to the previous processing. Here is another patchset to use the raw timestamp from Android camera capture, which is also monotonically increasing and can fix this problem. I tested a whole morning and didn't see any obvious AV sync problem. Please help to make a decision which patchset will be adopted.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Hi miu@, please have a look when you get chance.
On 2016/08/30 22:17:55, braveyao wrote: > Hi miu@, please have a look when you get chance. Taking a look now...
https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:125: public native void nativeOnFrameAvailable( Please plumb in timestamp here too. Let's be consistent for both code paths. https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:236: base::TimeDelta()); Please plumb-in timestamp here too. https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:259: const uint64_t absolute_micro = nit: Should be int64_t since both operands are signed integers. https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:261: const base::TimeDelta start_time = naming nit: How about |capture_time|? According to the Android API docs, the timestamps are all relative and monotonically increasing, and determined by the source capturer. So, basically they are exactly the media timestmaps OnIncomingCapturedData() expects. https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:300: if (expected_next_frame_time_ <= current_time) { What happened to the code from the "part I" patch that used the VideoCaptureOracle to do proper throttling? https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.h (right): https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/video_capture_device_android.h:133: base::TimeTicks expected_next_frame_time_; Seems like we should just get rid of |expected_next_frame_time_| and |frame_interval_|. There were, at best, estimates. Now that we're plumbing in the actual timestamps, let's get rid of these and the code doing any "guesstimating."
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by braveyao@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...
Hi miu, Thanks a lot! But I can't get all your concerns. Please check my replies. PS: patchset2 is a rebase to build my branch. The mits are fixed in patchset3. https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:125: public native void nativeOnFrameAvailable( On 2016/08/31 21:09:16, miu wrote: > Please plumb in timestamp here too. Let's be consistent for both code paths. This method is for the deprecated android.hardware.Camera API. The captured image is not read from the imageReader, so no such timestamp info. I suppose reading system time here does no good. So maybe keep the previous behavior? https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:236: base::TimeDelta()); On 2016/08/31 21:09:16, miu wrote: > Please plumb-in timestamp here too. No capture timestamp available here. Maybe better to keep the previous way. https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:259: const uint64_t absolute_micro = On 2016/08/31 21:09:16, miu wrote: > nit: Should be int64_t since both operands are signed integers. Done. https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:261: const base::TimeDelta start_time = On 2016/08/31 21:09:16, miu wrote: > naming nit: How about |capture_time|? According to the Android API docs, the > timestamps are all relative and monotonically increasing, and determined by the > source capturer. So, basically they are exactly the media timestmaps > OnIncomingCapturedData() expects. Done. https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:300: if (expected_next_frame_time_ <= current_time) { On 2016/08/31 21:09:16, miu wrote: > What happened to the code from the "part I" patch that used the > VideoCaptureOracle to do proper throttling? Can't get you. My 'part I' patch is for content capture. It didn't touch this camera capture path. Also there is no such a CaptureOracle to do throttling yet. https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.h (right): https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/video_capture_device_android.h:133: base::TimeTicks expected_next_frame_time_; On 2016/08/31 21:09:16, miu wrote: > Seems like we should just get rid of |expected_next_frame_time_| and > |frame_interval_|. There were, at best, estimates. Now that we're plumbing in > the actual timestamps, let's get rid of these and the code doing any > "guesstimating." These two variables are for satisfying framerate constraints on Android. Before we have another way to do it, they should be kept. If there is better way to do this, we can do it in anther cl.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:125: public native void nativeOnFrameAvailable( On 2016/08/31 23:01:28, braveyao wrote: > On 2016/08/31 21:09:16, miu wrote: > > Please plumb in timestamp here too. Let's be consistent for both code paths. > > This method is for the deprecated android.hardware.Camera API. The captured > image is not read from the imageReader, so no such timestamp info. > I suppose reading system time here does no good. So maybe keep the previous > behavior? Oh, I mistook all this for "Android screen capture." I see now that this is all about camera capture. So, sgtm. https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:300: if (expected_next_frame_time_ <= current_time) { On 2016/08/31 23:01:28, braveyao wrote: > On 2016/08/31 21:09:16, miu wrote: > > What happened to the code from the "part I" patch that used the > > VideoCaptureOracle to do proper throttling? > > Can't get you. > My 'part I' patch is for content capture. It didn't touch this camera capture > path. Also there is no such a CaptureOracle to do throttling yet. Acknowledged. My misunderstanding. ;-) https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.h (right): https://codereview.chromium.org/2272873002/diff/20001/media/capture/video/and... media/capture/video/android/video_capture_device_android.h:133: base::TimeTicks expected_next_frame_time_; On 2016/08/31 23:01:28, braveyao wrote: > On 2016/08/31 21:09:16, miu wrote: > > Seems like we should just get rid of |expected_next_frame_time_| and > > |frame_interval_|. There were, at best, estimates. Now that we're plumbing in > > the actual timestamps, let's get rid of these and the code doing any > > "guesstimating." > > These two variables are for satisfying framerate constraints on Android. Before > we have another way to do it, they should be kept. > If there is better way to do this, we can do it in anther cl. Acknowledged. https://codereview.chromium.org/2272873002/diff/80001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2272873002/diff/80001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:274: base::TimeDelta()); I'm pretty sure this could throw off consumers of the video frames downstream. You should try your best to provide reasonable media timestamp values here. Perhaps revert this to use "current_time - first_ref_time_" like it was before?
The CQ bit was checked by braveyao@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...
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by braveyao@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 checked by braveyao@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...
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by braveyao@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...
Thanks so much for the discussion. PTAL! https://codereview.chromium.org/2272873002/diff/80001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2272873002/diff/80001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:274: base::TimeDelta()); On 2016/09/02 21:49:15, miu wrote: > I'm pretty sure this could throw off consumers of the video frames downstream. > You should try your best to provide reasonable media timestamp values here. > > Perhaps revert this to use "current_time - first_ref_time_" like it was before? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qiangchen@chromium.org Link to the patchset: https://codereview.chromium.org/2272873002/#ps140001 (title: "estimate a timestamp in Camera1 callback")
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.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Android video capture: only use reference time for outbound timestamp. on Android, video capture may be stopped/started when Chrome switches between background and foreground. Each time when video capture is started, a new instance of VideoCaptureDeviceAndroid will be created, which will change the base value of "first_ref_time_". So the timestamp calculated by "current_time - first_ref_time_" won't be monotonically increasing to the same video stream. The consequence is frames will be dropped until the timestamp catches up to the old one cached in video_capture_input.cc finally. So on Android we should set the timestamp with 0 and use the reference time only. BUG=640387 ========== to ========== Android video capture: only use reference time for outbound timestamp. on Android, video capture may be stopped/started when Chrome switches between background and foreground. Each time when video capture is started, a new instance of VideoCaptureDeviceAndroid will be created, which will change the base value of "first_ref_time_". So the timestamp calculated by "current_time - first_ref_time_" won't be monotonically increasing to the same video stream. The consequence is frames will be dropped until the timestamp catches up to the old one cached in video_capture_input.cc finally. So on Android we should set the timestamp with 0 and use the reference time only. BUG=640387 Committed: https://crrev.com/4a93aa65fbe67592f324b1ea45dcdc0342801f0d Cr-Commit-Position: refs/heads/master@{#416814} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4a93aa65fbe67592f324b1ea45dcdc0342801f0d Cr-Commit-Position: refs/heads/master@{#416814} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
