Bug Fix: Logitech C930 On Linux Results In Freezing Video

Bug Fix: Logitech C930 On Linux Results In Freezing Video

We implemented using camera timestamp in https://codereview.chromium.org/1983193002

But in testing, we found a problem on Logitech C930, which
gives us non-monotonic timestamps, which will result in
vido freezing.

BUG=660883
Committed: https://crrev.com/6b8877a5ac02394679fdf37944af414d7534edba
Cr-Commit-Position: refs/heads/master@{#429620}

[qiangchen, 2016-10-31 16:52:50]

Description was changed from

==========
Bug Fix: Logitech C930 On Linux Results In Freezing Video

BUG=
==========

to

==========
Bug Fix: Logitech C930 On Linux Results In Freezing Video

We implemented using camera timestamp in
https://codereview.chromium.org/1983193002

But in testing, we found a problem on Logitech C930, which 
gives us non-monotonic timestamps, which will result in 
vido freezing.

BUG=660883
==========

[qiangchen, 2016-10-31 16:53:08]

qiangchen@chromium.org changed reviewers:
+ miu@chromium.org

[qiangchen, 2016-10-31 16:55:08]

Can you take a look?

As we found the camera timestamp for C930 is weird on Linux, this is a quick
fix.

[miu, 2016-10-31 21:02:04]

On 2016/10/31 16:55:08, qiangchenC wrote:
> Can you take a look?
> 
> As we found the camera timestamp for C930 is weird on Linux, this is a quick
> fix.

This feels wrong to me: You're removing the timestamp for all cameras that would
run on Linux because there is one specific vendor+model that is broken? Is there
a "smaller stick" approach here?

OTOH suggestion: How about dropping video frames that do not have
monotonically-increasing timestamps?

[qiangchen, 2016-10-31 21:19:54]

On 2016/10/31 21:02:04, miu wrote:
> On 2016/10/31 16:55:08, qiangchenC wrote:
> > Can you take a look?
> > 
> > As we found the camera timestamp for C930 is weird on Linux, this is a quick
> > fix.
> 
> This feels wrong to me: You're removing the timestamp for all cameras that
would
> run on Linux because there is one specific vendor+model that is broken? Is
there
> a "smaller stick" approach here?
> 
> OTOH suggestion: How about dropping video frames that do not have
> monotonically-increasing timestamps?

Dropping frame does not work. The timestamp could be very bad, not just a few
outliers.
Actually it is the webrtc's dropping frame mechanism which drops the frame due
to non-monotonicity of timestamps, so we see freezing.

Do you know the way to detect the camera model?

[Pawel Osciak, 2016-11-01 08:25:39]

On 2016/10/31 21:19:54, qiangchenC wrote:
> On 2016/10/31 21:02:04, miu wrote:
> > On 2016/10/31 16:55:08, qiangchenC wrote:
> > > Can you take a look?
> > > 
> > > As we found the camera timestamp for C930 is weird on Linux, this is a
quick
> > > fix.
> > 
> > This feels wrong to me: You're removing the timestamp for all cameras that
> would
> > run on Linux because there is one specific vendor+model that is broken? Is
> there
> > a "smaller stick" approach here?
> > 
> > OTOH suggestion: How about dropping video frames that do not have
> > monotonically-increasing timestamps?
> 
> Dropping frame does not work. The timestamp could be very bad, not just a few
> outliers.
> Actually it is the webrtc's dropping frame mechanism which drops the frame due
> to non-monotonicity of timestamps, so we see freezing.
> 
> Do you know the way to detect the camera model?

Perhaps we could instead keep track of the previous timestamp, and keep checking
whether the current timestamp was not monotonically increasing, in which case we
could only then pass an empty one?

[qiangchen, 2016-11-01 16:49:21]

On 2016/11/01 08:25:39, Pawel Osciak wrote:
> On 2016/10/31 21:19:54, qiangchenC wrote:
> > On 2016/10/31 21:02:04, miu wrote:
> > > On 2016/10/31 16:55:08, qiangchenC wrote:
> > > > Can you take a look?
> > > > 
> > > > As we found the camera timestamp for C930 is weird on Linux, this is a
> quick
> > > > fix.
> > > 
> > > This feels wrong to me: You're removing the timestamp for all cameras that
> > would
> > > run on Linux because there is one specific vendor+model that is broken? Is
> > there
> > > a "smaller stick" approach here?
> > > 
> > > OTOH suggestion: How about dropping video frames that do not have
> > > monotonically-increasing timestamps?
> > 
> > Dropping frame does not work. The timestamp could be very bad, not just a
few
> > outliers.
> > Actually it is the webrtc's dropping frame mechanism which drops the frame
due
> > to non-monotonicity of timestamps, so we see freezing.
> > 
> > Do you know the way to detect the camera model?
> 
> Perhaps we could instead keep track of the previous timestamp, and keep
checking
> whether the current timestamp was not monotonically increasing, in which case
we
> could only then pass an empty one?

Such simple solution won't work. 
For example: timestamps are 0, 33, 66, 1000000, 1, 34, 67
             ref times are  0, 33, 66, 100,   133, 166, 200
At frame with timestamp 1, we can detect the non-monotonicity, but at this
point, even if we switch back to reference time, 133 < 1000000, and thus
freezing will be observed.

WebRTC now has a timestamp aligner, which aligns the timestamp with reference
time, and automatically detects "bad" timestamp and switches using reference
time instead. So by testing we found it can solve the problem perfectly. 
But we need to merge the fix to M55.

[Niklas Enbom, 2016-11-02 16:13:09]

miu@, I see a couple of reasons to disable this completely on Linux:

1. There's no real gain in using it, as opposed to Windows and OSX we have ms
accuracy on the system clock
2. This was turned off in the Linux kernel tree since it wasn't working
correctly:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5d...
True, the referenced camera was also Logitech but a different model.

In WebRTC we already drop frames that aren't increase monotonically, and that's
what's giving the video freeze in this case. For example we receive timestamp N
(sent), then N+10000000 (sent), then N+100 (dropped), N+200 (dropped) etc

On 2016/11/01 16:49:21, qiangchenC wrote:
> On 2016/11/01 08:25:39, Pawel Osciak wrote:
> > On 2016/10/31 21:19:54, qiangchenC wrote:
> > > On 2016/10/31 21:02:04, miu wrote:
> > > > On 2016/10/31 16:55:08, qiangchenC wrote:
> > > > > Can you take a look?
> > > > > 
> > > > > As we found the camera timestamp for C930 is weird on Linux, this is a
> > quick
> > > > > fix.
> > > > 
> > > > This feels wrong to me: You're removing the timestamp for all cameras
that
> > > would
> > > > run on Linux because there is one specific vendor+model that is broken?
Is
> > > there
> > > > a "smaller stick" approach here?
> > > > 
> > > > OTOH suggestion: How about dropping video frames that do not have
> > > > monotonically-increasing timestamps?
> > > 
> > > Dropping frame does not work. The timestamp could be very bad, not just a
> few
> > > outliers.
> > > Actually it is the webrtc's dropping frame mechanism which drops the frame
> due
> > > to non-monotonicity of timestamps, so we see freezing.
> > > 
> > > Do you know the way to detect the camera model?
> > 
> > Perhaps we could instead keep track of the previous timestamp, and keep
> checking
> > whether the current timestamp was not monotonically increasing, in which
case
> we
> > could only then pass an empty one?
> 
> Such simple solution won't work. 
> For example: timestamps are 0, 33, 66, 1000000, 1, 34, 67
>              ref times are  0, 33, 66, 100,   133, 166, 200
> At frame with timestamp 1, we can detect the non-monotonicity, but at this
> point, even if we switch back to reference time, 133 < 1000000, and thus
> freezing will be observed.
> 
> WebRTC now has a timestamp aligner, which aligns the timestamp with reference
> time, and automatically detects "bad" timestamp and switches using reference
> time instead. So by testing we found it can solve the problem perfectly. 
> But we need to merge the fix to M55.

[miu, 2016-11-02 19:34:56]

https://codereview.chromium.org/2463853002/diff/1/media/capture/video/linux/v...
File media/capture/video/linux/v4l2_capture_delegate.cc (right):

https://codereview.chromium.org/2463853002/diff/1/media/capture/video/linux/v...
media/capture/video/linux/v4l2_capture_delegate.cc:568: capture_format_,
rotation_, base::TimeTicks::Now(), base::TimeDelta());
Given niklase's recent comments, it now makes sense to not use the timing data
from the v4l2_buffer. Let's do two things here, though: 1) Document w/ code
comments what's going on; 2) Use surrogate timestamps instead of "blanking-out"
the media timestamp field (because WebRTC is not the only consumer of webcam
video, and we should never blanketly violate API contracts anyway).

Putting that all together, I propose:

  // There's a wide-spread issue where the kernel does not report
  // accurate, monotonically-increasing timestamps in the
  // v4l2_buffer::timestamp field (<LINK TO git.kernel.org COMMIT
  // HERE>). Until this issue is fixed, just use the reference
  // clock as a source of media timestamps.
  const base::TimeTicks now = base::TimeTicks::Now():
  const base::TimeDelta surrogate_timestamp = now - base::TimeTicks();
  client->OnIncomingCapturedData(..., now, surrogate_timestamp);

WDYT?

Alternatively, video capture on Android includes code that simply generates
perfect frame timestamps when using a fixed frame rate. So, you may want to look
at how this problem is solved there and consider whether that solution is
better:
https://cs.chromium.org/chromium/src/media/capture/video/android/video_captur...

[qiangchen, 2016-11-02 23:07:52]

fixed. Thanks.

https://codereview.chromium.org/2463853002/diff/1/media/capture/video/linux/v...
File media/capture/video/linux/v4l2_capture_delegate.cc (right):

https://codereview.chromium.org/2463853002/diff/1/media/capture/video/linux/v...
media/capture/video/linux/v4l2_capture_delegate.cc:568: capture_format_,
rotation_, base::TimeTicks::Now(), base::TimeDelta());
On 2016/11/02 19:34:56, miu wrote:
> Given niklase's recent comments, it now makes sense to not use the timing data
> from the v4l2_buffer. Let's do two things here, though: 1) Document w/ code
> comments what's going on; 2) Use surrogate timestamps instead of
"blanking-out"
> the media timestamp field (because WebRTC is not the only consumer of webcam
> video, and we should never blanketly violate API contracts anyway).
> 
> Putting that all together, I propose:
> 
>   // There's a wide-spread issue where the kernel does not report
>   // accurate, monotonically-increasing timestamps in the
>   // v4l2_buffer::timestamp field (<LINK TO http://git.kernel.org COMMIT
>   // HERE>). Until this issue is fixed, just use the reference
>   // clock as a source of media timestamps.
>   const base::TimeTicks now = base::TimeTicks::Now():
>   const base::TimeDelta surrogate_timestamp = now - base::TimeTicks();
>   client->OnIncomingCapturedData(..., now, surrogate_timestamp);
> 
> WDYT?
> 
> Alternatively, video capture on Android includes code that simply generates
> perfect frame timestamps when using a fixed frame rate. So, you may want to
look
> at how this problem is solved there and consider whether that solution is
> better:
>
https://cs.chromium.org/chromium/src/media/capture/video/android/video_captur...
> 

Normalize the timestamp wrt the first reference time.

Did not use the android way here, as we are considering merging this fix, and
thus reverting back to the old way will be safer.

[miu, 2016-11-02 23:28:23]

The CQ bit was checked by miu@chromium.org

[miu, 2016-11-02 23:28:23]

lgtm

[commit-bot: I haz the power, 2016-11-02 23:28:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 23:33:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 23:33:11]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qiangchen, 2016-11-02 23:43:40]

The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 23:44:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 01:14:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 01:14:19]

Dry run: This issue passed the CQ dry run.

[qiangchen, 2016-11-03 16:46:00]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-11-03 16:46:01]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2463853002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-03 16:46:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 16:52:52]

Description was changed from

==========
Bug Fix: Logitech C930 On Linux Results In Freezing Video

We implemented using camera timestamp in
https://codereview.chromium.org/1983193002

But in testing, we found a problem on Logitech C930, which 
gives us non-monotonic timestamps, which will result in 
vido freezing.

BUG=660883
==========

to

==========
Bug Fix: Logitech C930 On Linux Results In Freezing Video

We implemented using camera timestamp in
https://codereview.chromium.org/1983193002

But in testing, we found a problem on Logitech C930, which 
gives us non-monotonic timestamps, which will result in 
vido freezing.

BUG=660883
==========

[commit-bot: I haz the power, 2016-11-03 16:52:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-03 17:13:10]

Description was changed from

==========
Bug Fix: Logitech C930 On Linux Results In Freezing Video

We implemented using camera timestamp in
https://codereview.chromium.org/1983193002

But in testing, we found a problem on Logitech C930, which 
gives us non-monotonic timestamps, which will result in 
vido freezing.

BUG=660883
==========

to

==========
Bug Fix: Logitech C930 On Linux Results In Freezing Video

We implemented using camera timestamp in
https://codereview.chromium.org/1983193002

But in testing, we found a problem on Logitech C930, which
gives us non-monotonic timestamps, which will result in
vido freezing.

BUG=660883

Committed: https://crrev.com/6b8877a5ac02394679fdf37944af414d7534edba
Cr-Commit-Position: refs/heads/master@{#429620}
==========

[commit-bot: I haz the power, 2016-11-03 17:13:11]

Patchset 3 (id:??) landed as
https://crrev.com/6b8877a5ac02394679fdf37944af414d7534edba
Cr-Commit-Position: refs/heads/master@{#429620}