Bug Fix: Remote Video Hang For Apprtc Loopback

Bug Fix: Remote Video Hang For Apprtc Loopback

On ChromeOS (Pit), we observed remote video hang when running
apprtc loopback. The cause is that when the tab is inactive,
renderer does not pull frames from WebMediaPlayerMS, and it
relies on WebMediaPlayerMS to digest old frames.

In case of hardware decoding, we have only 4 decode buffers.
If we use up all 4 of them, no more decoding can happen, and
the video will hang. The old algorithm digesting old frames is
conservative, and then there is a chance that we use up all 4
buffers.

In this CL, we switch to an aggressive way. It is safe, as when
the tab is inactive, no one cares about what happens behind the
scenes.

BUG=588813
Committed: https://crrev.com/aa4011dd349cf4a3f5da97c8a20d55cedbf24ffc
Cr-Commit-Position: refs/heads/master@{#380461}

[qiangchen, 2016-03-09 00:25:48]

Description was changed from

==========
Bug Fix: Remote Video Hang For Apprtc Loopback

BUG=588813
==========

to

==========
Bug Fix: Remote Video Hang For Apprtc Loopback

On ChromeOS (Pit), we observed remote video hang when running 
apprtc loopback. The cause is that when the tab is inactive,
renderer does not pull frames from WebMediaPlayerMS, and it 
relies on WebMediaPlayerMS to digest old frames. 

In case of hardware decoding, we have only 4 decode buffers.
If we use up all 4 of them, no more decoding can happen, and
the video will hang. The old algorithm digesting old frames is
conservative, and then there is a chance that we use up all 4
buffers. 

In this CL, we switch to an aggressive way. It is safe, as when
the tab is inactive, no one cares about what happens behind the 
scenes.

BUG=588813
==========

[qiangchen, 2016-03-09 00:27:38]

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

[qiangchen, 2016-03-09 00:29:42]

Hi, Dale:

    This is another fix CL for some boundary case of my rendering smoothness
project. 
    Sorry, my original developement is so bugful.

Thanks.

[DaleCurtis, 2016-03-09 00:42:23]

https://codereview.chromium.org/1772353003/diff/1/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_ms_compositor.cc (right):

https://codereview.chromium.org/1772353003/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms_compositor.cc:224:
rendering_frame_buffer_->Reset();
Hmm, is this really what you want? This will drop the frame that was just
enqueued. Do you instead want to use RemoveExpiredFrames(now) ?

[qiangchen, 2016-03-09 19:55:02]

Changed to preserve the most recent frame during inactive tab.

And consider stats update now.

https://codereview.chromium.org/1772353003/diff/1/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_ms_compositor.cc (right):

https://codereview.chromium.org/1772353003/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms_compositor.cc:224:
rendering_frame_buffer_->Reset();
On 2016/03/09 00:42:23, DaleCurtis wrote:
> Hmm, is this really what you want? This will drop the frame that was just
> enqueued. Do you instead want to use RemoveExpiredFrames(now) ?

Probably RemoveExpiredFrames(now) will not work. As it sets the deadline as now
- frame_length - acceptable_drift, which is almost now - 1.5*frame_length. But
most queued frames have render_time in the future. 

I changed the code to preserve exactly one frame when tab is inactive.
Theoretically this would affect smoothness for the first several frames when the
tab is activated later. But in reality considering the start up over head of
renderer, the effect is unnoticable.

[DaleCurtis, 2016-03-09 21:54:01]

I forget all the details here, so you might want to have someone from RTC check
this over too, but it lgtm now.

https://codereview.chromium.org/1772353003/diff/20001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms_compositor.cc (right):

https://codereview.chromium.org/1772353003/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_compositor.cc:218: // This block
considers the case that vsyncs stops rendering frames.
"The code below handles the case where UpdateCurrentFrame() callbacks stop.
These callbacks can stop when the tab is hidden or the page area containing the
video frame is scrolled out of view. Since some hardware decoders only have a
limited number of output frames, we must aggressively release frames in this
case."

https://codereview.chromium.org/1772353003/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_compositor.cc:223: dropped_frame_count_
+= rendering_frame_buffer_->frames_queued() - 1;
Add a comment explaining why the -1.

[qiangchen, 2016-03-09 22:46:40]

https://codereview.chromium.org/1772353003/diff/20001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms_compositor.cc (right):

https://codereview.chromium.org/1772353003/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_compositor.cc:218: // This block
considers the case that vsyncs stops rendering frames.
On 2016/03/09 21:54:01, DaleCurtis wrote:
> "The code below handles the case where UpdateCurrentFrame() callbacks stop.
> These callbacks can stop when the tab is hidden or the page area containing
the
> video frame is scrolled out of view. Since some hardware decoders only have a
> limited number of output frames, we must aggressively release frames in this
> case."

Done.

https://codereview.chromium.org/1772353003/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_compositor.cc:223: dropped_frame_count_
+= rendering_frame_buffer_->frames_queued() - 1;
On 2016/03/09 21:54:01, DaleCurtis wrote:
> Add a comment explaining why the -1.

Done.

[qiangchen, 2016-03-09 22:46:45]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-03-09 22:46:45]

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

[commit-bot: I haz the power, 2016-03-09 22:47:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1772353003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1772353003/40001

[commit-bot: I haz the power, 2016-03-09 23:53:05]

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

[commit-bot: I haz the power, 2016-03-09 23:53:06]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[qiangchen, 2016-03-10 20:51:09]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-03-10 20:51:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1772353003/#ps80001
(title: "Blank line remove")

[commit-bot: I haz the power, 2016-03-10 20:51:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1772353003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1772353003/80001

[commit-bot: I haz the power, 2016-03-10 20:58:00]

Description was changed from

==========
Bug Fix: Remote Video Hang For Apprtc Loopback

On ChromeOS (Pit), we observed remote video hang when running 
apprtc loopback. The cause is that when the tab is inactive,
renderer does not pull frames from WebMediaPlayerMS, and it 
relies on WebMediaPlayerMS to digest old frames. 

In case of hardware decoding, we have only 4 decode buffers.
If we use up all 4 of them, no more decoding can happen, and
the video will hang. The old algorithm digesting old frames is
conservative, and then there is a chance that we use up all 4
buffers. 

In this CL, we switch to an aggressive way. It is safe, as when
the tab is inactive, no one cares about what happens behind the 
scenes.

BUG=588813
==========

to

==========
Bug Fix: Remote Video Hang For Apprtc Loopback

On ChromeOS (Pit), we observed remote video hang when running 
apprtc loopback. The cause is that when the tab is inactive,
renderer does not pull frames from WebMediaPlayerMS, and it 
relies on WebMediaPlayerMS to digest old frames. 

In case of hardware decoding, we have only 4 decode buffers.
If we use up all 4 of them, no more decoding can happen, and
the video will hang. The old algorithm digesting old frames is
conservative, and then there is a chance that we use up all 4
buffers. 

In this CL, we switch to an aggressive way. It is safe, as when
the tab is inactive, no one cares about what happens behind the 
scenes.

BUG=588813
==========

[commit-bot: I haz the power, 2016-03-10 20:58:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-10 21:00:16]

Description was changed from

==========
Bug Fix: Remote Video Hang For Apprtc Loopback

On ChromeOS (Pit), we observed remote video hang when running 
apprtc loopback. The cause is that when the tab is inactive,
renderer does not pull frames from WebMediaPlayerMS, and it 
relies on WebMediaPlayerMS to digest old frames. 

In case of hardware decoding, we have only 4 decode buffers.
If we use up all 4 of them, no more decoding can happen, and
the video will hang. The old algorithm digesting old frames is
conservative, and then there is a chance that we use up all 4
buffers. 

In this CL, we switch to an aggressive way. It is safe, as when
the tab is inactive, no one cares about what happens behind the 
scenes.

BUG=588813
==========

to

==========
Bug Fix: Remote Video Hang For Apprtc Loopback

On ChromeOS (Pit), we observed remote video hang when running
apprtc loopback. The cause is that when the tab is inactive,
renderer does not pull frames from WebMediaPlayerMS, and it
relies on WebMediaPlayerMS to digest old frames.

In case of hardware decoding, we have only 4 decode buffers.
If we use up all 4 of them, no more decoding can happen, and
the video will hang. The old algorithm digesting old frames is
conservative, and then there is a chance that we use up all 4
buffers.

In this CL, we switch to an aggressive way. It is safe, as when
the tab is inactive, no one cares about what happens behind the
scenes.

BUG=588813

Committed: https://crrev.com/aa4011dd349cf4a3f5da97c8a20d55cedbf24ffc
Cr-Commit-Position: refs/heads/master@{#380461}
==========

[commit-bot: I haz the power, 2016-03-10 21:00:18]

Patchset 5 (id:??) landed as
https://crrev.com/aa4011dd349cf4a3f5da97c8a20d55cedbf24ffc
Cr-Commit-Position: refs/heads/master@{#380461}