Update VideoFramePump to pass un-changed frames to encoders.

Update VideoFramePump to pass un-changed frames to encoders.

This allows VP9/lossy mode to "top-off" previously encoded imagery to
improve visual quality at the client, by continuing to deliver data even
when nothing has actually changed on the host desktop.

This CL makes several other minor changes:
- Fix the host's --enable-i444 flag, which was broken by refactoring.
- Tweak VP9/lossy mode to more conservative quantization settings.
- Tweak VP9/lossy mode to encode up to two unchanged frames for top-off.
- Update VideoEncoderVerbatim for the new VideoFramePump semantics.
- Update the VideoFrameRecorder & FakeDesktopCapturer.
- Adds simple codec & VideoFramePump unit-tests for the new behaviour.

BUG=134202
Committed: https://crrev.com/43ac266109c2cb29aca8a4858726a3b369c1f9db
Cr-Commit-Position: refs/heads/master@{#333764}

[Wez, 2015-05-23 01:11:00]

wez@chromium.org changed reviewers:
+ aconverse@chromium.org, sergeyu@chromium.org

[Sergey Ulanov, 2015-05-26 21:57:22]

This change actually enables top-off in the encoder, while description says that
the change is in VideoFramePump just to allow top-off. Update description?

https://codereview.chromium.org/1150163002/diff/40001/remoting/codec/video_en...
File remoting/codec/video_encoder_vpx.cc (right):

https://codereview.chromium.org/1150163002/diff/40001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.cc:275: if (!--topoff_frame_count_) {
nit: --topoff_frame_count > 0 (or != 0) would be more readable here

https://codereview.chromium.org/1150163002/diff/40001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.cc:280: topoff_frame_count_ = 2;
topoff_frame_count_ is set even when using VP8, but it's ignored later. Maybe
wrap this while block in if(use_vp9_) {}, to make it clear that topoff applies
only for VP9.

https://codereview.chromium.org/1150163002/diff/40001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.cc:297: SetActiveMapFromRegion(updated_region);
Do we need to supply active map for top-off, or does VPX keep track of the
changes in the previous frames?

https://codereview.chromium.org/1150163002/diff/40001/remoting/codec/video_en...
File remoting/codec/video_encoder_vpx.h (right):

https://codereview.chromium.org/1150163002/diff/40001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.h:81: int topoff_frame_count_;
nit: initialize the value here instead of constructor (and maybe move the
initializers for other fields here, except use_vp9_)

[Wez, 2015-06-03 23:56:55]

On 2015/05/26 21:57:22, Sergey Ulanov wrote:
> This change actually enables top-off in the encoder, while description says
that
> the change is in VideoFramePump just to allow top-off. Update description?

Top-off is already enabled via the Cyclic Refresh option set on the VP9 encoder
when in lossy mode - this CL is updating the VpxVideoEncoder to handle empty
frames by sending the first few on to the codec, and updating the VideoFramePump
to pass all frames, even empty ones, to the codec. I'll update the description
re the VpxVideoEncoder change, though.

[Sergey Ulanov, 2015-06-06 00:19:58]

lgtm

https://codereview.chromium.org/1150163002/diff/60001/remoting/codec/video_en...
File remoting/codec/video_encoder.h (right):

https://codereview.chromium.org/1150163002/diff/60001/remoting/codec/video_en...
remoting/codec/video_encoder.h:28: // Encode an image stored in |frame|. If
|frame.uupdated_region()| is empty
typo: uupdated

https://codereview.chromium.org/1150163002/diff/60001/remoting/codec/video_en...
File remoting/codec/video_encoder_vpx.cc (right):

https://codereview.chromium.org/1150163002/diff/60001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.cc:277: if (topoff_frame_count_ > 0) {
nit: might be more readable if you handle the return case first:
  if (topoff_frame_count == 0)
    return nullptr;
  topoff_frame_count--;

[Wez, 2015-06-09 01:46:21]

https://codereview.chromium.org/1150163002/diff/40001/remoting/codec/video_en...
File remoting/codec/video_encoder_vpx.cc (right):

https://codereview.chromium.org/1150163002/diff/40001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.cc:275: if (!--topoff_frame_count_) {
On 2015/05/26 21:57:22, Sergey Ulanov wrote:
> nit: --topoff_frame_count > 0 (or != 0) would be more readable here

Plus, in cleaning it up, I fixed a bug in my implementation... :-/

https://codereview.chromium.org/1150163002/diff/40001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.cc:280: topoff_frame_count_ = 2;
On 2015/05/26 21:57:22, Sergey Ulanov wrote:
> topoff_frame_count_ is set even when using VP8, but it's ignored later. Maybe
> wrap this while block in if(use_vp9_) {}, to make it clear that topoff applies
> only for VP9.

Done.

https://codereview.chromium.org/1150163002/diff/40001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.cc:297: SetActiveMapFromRegion(updated_region);
On 2015/05/26 21:57:22, Sergey Ulanov wrote:
> Do we need to supply active map for top-off, or does VPX keep track of the
> changes in the previous frames?

Cyclic Refresh will top-off even during "active" frames, so we still need to set
the active map to cover the active areas.

https://codereview.chromium.org/1150163002/diff/40001/remoting/codec/video_en...
File remoting/codec/video_encoder_vpx.h (right):

https://codereview.chromium.org/1150163002/diff/40001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.h:81: int topoff_frame_count_;
On 2015/05/26 21:57:22, Sergey Ulanov wrote:
> nit: initialize the value here instead of constructor (and maybe move the
> initializers for other fields here, except use_vp9_)

Done.

Although the compiler was quite happy with me double-initializing. :-/

[Wez, 2015-06-09 01:46:36]

Please take a look at the new tests.

[Sergey Ulanov, 2015-06-09 05:32:44]

lgtm

https://codereview.chromium.org/1150163002/diff/60001/remoting/codec/video_en...
File remoting/codec/video_encoder_vpx.cc (right):

https://codereview.chromium.org/1150163002/diff/60001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.cc:277: if (topoff_frame_count_ > 0) {
On 2015/06/06 00:19:58, Sergey Ulanov wrote:
> nit: might be more readable if you handle the return case first:
>   if (topoff_frame_count == 0)
>     return nullptr;
>   topoff_frame_count--;

not addressed in the last patch set.

https://codereview.chromium.org/1150163002/diff/70001/remoting/codec/codec_te...
File remoting/codec/codec_test.cc (right):

https://codereview.chromium.org/1150163002/diff/70001/remoting/codec/codec_te...
remoting/codec/codec_test.cc:296: do {
nit: this can be a simpler for loop:
  for (int i = 0; i < max_topoff_frames; ++i) {
    ASSERT(encoder->Encode(*frame) != nullptr);
  }
  ASSERT(encoder->Encode(*frame) == nullptr);

https://codereview.chromium.org/1150163002/diff/70001/remoting/codec/video_en...
File remoting/codec/video_encoder.h (right):

https://codereview.chromium.org/1150163002/diff/70001/remoting/codec/video_en...
remoting/codec/video_encoder.h:28: // Encode an image stored in |frame|. If
|frame.uupdated_region()| is empty
typo: uupdated

https://codereview.chromium.org/1150163002/diff/70001/remoting/host/video_fra...
File remoting/host/video_frame_pump_unittest.cc (right):

https://codereview.chromium.org/1150163002/diff/70001/remoting/host/video_fra...
remoting/host/video_frame_pump_unittest.cc:205: // Run MessageLoop until the
first frame is received..
typo: ..

[Wez, 2015-06-09 22:00:38]

https://codereview.chromium.org/1150163002/diff/60001/remoting/codec/video_en...
File remoting/codec/video_encoder_vpx.cc (right):

https://codereview.chromium.org/1150163002/diff/60001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.cc:277: if (topoff_frame_count_ > 0) {
On 2015/06/09 05:32:43, Sergey Ulanov wrote:
> On 2015/06/06 00:19:58, Sergey Ulanov wrote:
> > nit: might be more readable if you handle the return case first:
> >   if (topoff_frame_count == 0)
> >     return nullptr;
> >   topoff_frame_count--;
> 
> not addressed in the last patch set.

Done.

https://codereview.chromium.org/1150163002/diff/70001/remoting/codec/codec_te...
File remoting/codec/codec_test.cc (right):

https://codereview.chromium.org/1150163002/diff/70001/remoting/codec/codec_te...
remoting/codec/codec_test.cc:296: do {
On 2015/06/09 05:32:43, Sergey Ulanov wrote:
> nit: this can be a simpler for loop:
>   for (int i = 0; i < max_topoff_frames; ++i) {
>     ASSERT(encoder->Encode(*frame) != nullptr);
>   }
>   ASSERT(encoder->Encode(*frame) == nullptr);
>  

That implementation would enforce that there are exactly |max_topoff_frames|
top-off frames, though - this implementation is supposed to allow up to
|max_topoff_frames|. I'll change it to test for an exact number of top-off
frames, though.

https://codereview.chromium.org/1150163002/diff/70001/remoting/codec/video_en...
File remoting/codec/video_encoder.h (right):

https://codereview.chromium.org/1150163002/diff/70001/remoting/codec/video_en...
remoting/codec/video_encoder.h:28: // Encode an image stored in |frame|. If
|frame.uupdated_region()| is empty
On 2015/06/09 05:32:44, Sergey Ulanov wrote:
> typo: uupdated

Done.

https://codereview.chromium.org/1150163002/diff/70001/remoting/host/video_fra...
File remoting/host/video_frame_pump_unittest.cc (right):

https://codereview.chromium.org/1150163002/diff/70001/remoting/host/video_fra...
remoting/host/video_frame_pump_unittest.cc:205: // Run MessageLoop until the
first frame is received..
On 2015/06/09 05:32:44, Sergey Ulanov wrote:
> typo: ..

Done.

[Wez, 2015-06-10 17:44:29]

The CQ bit was checked by wez@chromium.org

[Wez, 2015-06-10 17:44:30]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1150163002/#ps110001
(title: "Work-around Visual Studio issue w/ undefined references")

[commit-bot: I haz the power, 2015-06-10 17:44:36]

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

[commit-bot: I haz the power, 2015-06-10 18:22:35]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-06-10 18:25:18]

Patchset 7 (id:??) landed as
https://crrev.com/43ac266109c2cb29aca8a4858726a3b369c1f9db
Cr-Commit-Position: refs/heads/master@{#333764}