[cast] Add optional VideoEncoder method to flush frames.

[cast] Add optional VideoEncoder method to flush frames.

This patch adds EmitFrames() to VideoEncoder. The method is documented
as indicating that the encoder should produce all the frames that it may
be buffering for analysis.

This patch modifies VideoSender::InsertRawVideoFrame() to call
EmitFrames() when a frames is dropped because the in-flight frames
duration exceeds the target playout delay.

This patch provides an implementation of EmitFrames() for the
VideoToolbox H.264 encoder, which buffers frames for analysis as
described in the bug. It also uses the existing
max_number_of_video_buffers_used field in VideoConfig to set the
upper bound on the encoder's frame window.

BUG=450798
R=hclam, miu, DaleCurtis, Robert Sesek

Committed: https://crrev.com/2c176764c9300863a30e028417ee5c6a5aadc6ad
Cr-Commit-Position: refs/heads/master@{#313643}

[jfroy, 2015-01-21 23:20:00]

jfroy@chromium.org changed reviewers:
+ dalecurtis@chromium.org, rsesek@chromium.org

[jfroy, 2015-01-21 23:20:00]

+ rsesek, DaleCurtis

[miu, 2015-01-21 23:24:27]

https://codereview.chromium.org/863083002/diff/1/media/cast/sender/h264_vt_en...
File media/cast/sender/h264_vt_encoder.cc (right):

https://codereview.chromium.org/863083002/diff/1/media/cast/sender/h264_vt_en...
media/cast/sender/h264_vt_encoder.cc:340:
video_config.max_number_of_video_buffers_used);
VideoSenderConfig::max_number_of_video_buffers_used means something completely
different from how you're using it here.

However, it's a codec-specific property.  I think it would be reasonable if you
just document (in cast_config.h) how the H264 codec interprets this value.

https://codereview.chromium.org/863083002/diff/1/media/cast/sender/video_send...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/863083002/diff/1/media/cast/sender/video_send...
media/cast/sender/video_sender.cc:195: video_encoder_->EmitFrames();
Some questions:

Will the video encoder emit frames if EmitFrames() is never called?  

Will it do so in a timely manner so as to not cause ShouldDropNextFrame() to
trigger because of a "hold-up" within the encoder?

Is this feature of the toolbox encoder meant for real-time use?  Or, is it only
meant for the batch/file streaming use cases?

Are there any other toolbox config options to distinguish that the encoder is
being used for the real-time use case?

[jfroy, 2015-01-21 23:36:58]

On 2015/01/21 23:24:27, miu wrote:
>
https://codereview.chromium.org/863083002/diff/1/media/cast/sender/h264_vt_en...
> File media/cast/sender/h264_vt_encoder.cc (right):
> 
>
https://codereview.chromium.org/863083002/diff/1/media/cast/sender/h264_vt_en...
> media/cast/sender/h264_vt_encoder.cc:340:
> video_config.max_number_of_video_buffers_used);
> VideoSenderConfig::max_number_of_video_buffers_used means something completely
> different from how you're using it here.
> 
> However, it's a codec-specific property.  I think it would be reasonable if
you
> just document (in cast_config.h) how the H264 codec interprets this value.

Yes, I used this field because it is codec-specific. It's a hack for sure. I'm
happy to
document its behavior for the VT encoder.

> 
>
https://codereview.chromium.org/863083002/diff/1/media/cast/sender/video_send...
> File media/cast/sender/video_sender.cc (right):
> 
>
https://codereview.chromium.org/863083002/diff/1/media/cast/sender/video_send...
> media/cast/sender/video_sender.cc:195: video_encoder_->EmitFrames();
> Some questions:
> 
> Will the video encoder emit frames if EmitFrames() is never called?

Yes, if new input frames are submitted to the encoder.

> 
> Will it do so in a timely manner so as to not cause ShouldDropNextFrame() to
> trigger because of a "hold-up" within the encoder?

In my experience, yes. I've only seen the issue described in the bug when there
is a delay in submitting frames or the target playout delay is too low. In such
a
case, frame N's timestamp + in-flight frame durations > target delay, which
causes N to be dropped. Then N + 1 comes with a yet-larger timestamp, which
causes it to be dropped, etc. Previous frames never get flushed so VideoSender
never goes through the estimated duration code path (when there are no in-flight
frames).

> 
> Is this feature of the toolbox encoder meant for real-time use?  Or, is it
only
> meant for the batch/file streaming use cases?

VT always uses a frame window, unless you set the max delay to 1. The real-time
mode reduces the window considerably compared to the default mode (which is
tuned for offline encoding). Disabling frame re-ordering further reduces the
window.

The specific value is encoder-specific. In practice, I've only hit this issue
with the
simulator, which relies on a software encoder.

> 
> Are there any other toolbox config options to distinguish that the encoder is
> being used for the real-time use case?

The current implementation properly configures the encoder for real-time usage,
with the exception of the frame window, in so far as what is available through
the
public API.

[miu, 2015-01-22 01:07:18]

lgtm

On 2015/01/21 23:36:58, jfroy wrote:
> On 2015/01/21 23:24:27, miu wrote:
> > Will it do so in a timely manner so as to not cause ShouldDropNextFrame() to
> > trigger because of a "hold-up" within the encoder?
> 
> In my experience, yes. I've only seen the issue described in the bug when
there
> is a delay in submitting frames or the target playout delay is too low. In
such
> a
> case, frame N's timestamp + in-flight frame durations > target delay, which
> causes N to be dropped. Then N + 1 comes with a yet-larger timestamp, which
> causes it to be dropped, etc. Previous frames never get flushed so VideoSender
> never goes through the estimated duration code path (when there are no
in-flight
> frames).

Okay.  I just wanted to be sure the system doesn't get into a harmful state
where frames are getting dropped only because the encoder wants to hold on to
them too long for its own quality analysis.  In general, Cast streaming takes
the philosophy of "drop frames as a last resort," where decreasing the quality
of individual frame encodes is a preferred way to deal with CPU or network
bottlenecks.

https://codereview.chromium.org/863083002/diff/40001/media/cast/sender/video_...
File media/cast/sender/video_encoder.h (right):

https://codereview.chromium.org/863083002/diff/40001/media/cast/sender/video_...
media/cast/sender/video_encoder.h:56: // submitted frames so that eventually new
frames may be encoded.
Also in this comment, please add something to the tune of "This is NOT a
synchronous caller-blocking operation."

[jfroy, 2015-01-22 18:03:54]

The CQ bit was checked by jfroy@chromium.org

[commit-bot: I haz the power, 2015-01-22 18:05:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/863083002/50001

[commit-bot: I haz the power, 2015-01-22 18:14:10]

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

[commit-bot: I haz the power, 2015-01-22 18:14:11]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jfroy, 2015-01-22 19:45:54]

Missing LGTM from media/base owner, unrelated test
(PolicyPathParserTests.AllPlatformVariables) is failing on mac_chromium_rel_ng,
and unrelated tests failing on linux bots. I'll wait for missing approval and
try again later.

[jfroy, 2015-01-26 18:52:28]

The CQ bit was checked by jfroy@chromium.org

[commit-bot: I haz the power, 2015-01-26 18:53:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/863083002/70001

[commit-bot: I haz the power, 2015-01-26 18:59:57]

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

[commit-bot: I haz the power, 2015-01-26 18:59:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jfroy, 2015-01-26 19:01:22]

Woops, I didn't mean to CQ this :/

[DaleCurtis, 2015-01-28 23:41:33]

Lgtm

[jfroy, 2015-01-28 23:43:47]

The CQ bit was checked by jfroy@chromium.org

[commit-bot: I haz the power, 2015-01-28 23:44:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/863083002/90001

[commit-bot: I haz the power, 2015-01-29 00:49:00]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2015-01-29 00:49:54]

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