Add manual tests to benchmark VP8 & VP9 encode performance.

Add manual tests to benchmark VP8 & VP9 encode performance.

BUG=260879
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280753

[Wez, 2014-06-23 22:15:40]

PTAL

[Sergey Ulanov, 2014-06-24 00:58:25]

https://codereview.chromium.org/349223007/diff/20001/remoting/codec/codec_tes...
File remoting/codec/codec_test.cc (right):

https://codereview.chromium.org/349223007/diff/20001/remoting/codec/codec_tes...
remoting/codec/codec_test.cc:392: return
MeasureVideoEncoderFpsWithFrames(encoder, frames);
This will just reencode the same frame multiple times, i.e. there are no changes
between consecutive frames. Would it be better to add at least couple of frames
into the queue?

https://codereview.chromium.org/349223007/diff/20001/remoting/codec/codec_tes...
remoting/codec/codec_test.cc:399: // Warm up the encoder.
Why is this necessary? Why do we need 10 frames for warmup (I thought 1 should
be enough).

https://codereview.chromium.org/349223007/diff/20001/remoting/codec/codec_tes...
remoting/codec/codec_test.cc:413: for (frame_count = 0;
The body of this loop is not much different from the loop in line 402. Maybe
merge them together? You'll just need to set start_time on 10th frame.

https://codereview.chromium.org/349223007/diff/20001/remoting/codec/codec_test.h
File remoting/codec/codec_test.h (right):

https://codereview.chromium.org/349223007/diff/20001/remoting/codec/codec_tes...
remoting/codec/codec_test.h:51: VideoEncoder* encoder, const
webrtc::DesktopSize& size);
one argument per line please

[Wez, 2014-06-24 01:52:48]

https://codereview.chromium.org/349223007/diff/20001/remoting/codec/codec_tes...
File remoting/codec/codec_test.cc (right):

https://codereview.chromium.org/349223007/diff/20001/remoting/codec/codec_tes...
remoting/codec/codec_test.cc:392: return
MeasureVideoEncoderFpsWithFrames(encoder, frames);
On 2014/06/24 00:58:25, Sergey Ulanov wrote:
> This will just reencode the same frame multiple times, i.e. there are no
changes
> between consecutive frames. Would it be better to add at least couple of
frames
> into the queue?

Encoding the same frame repeatedly with the updated-region set to the whole
frame is a valid benchmark - to properly test performance encoding sequences of
frames, I'd prefer to play back recorded sequences rather than making up frames,
which wouldn't necessarily be representative either.

https://codereview.chromium.org/349223007/diff/20001/remoting/codec/codec_tes...
remoting/codec/codec_test.cc:399: // Warm up the encoder.
On 2014/06/24 00:58:25, Sergey Ulanov wrote:
> Why is this necessary? Why do we need 10 frames for warmup (I thought 1 should
> be enough).

One frame should be sufficient to get us past the first key-frame, doing more is
intended to help ensure that e.g. the OS' view of the process' working-set has
stabilised.

https://codereview.chromium.org/349223007/diff/20001/remoting/codec/codec_tes...
remoting/codec/codec_test.cc:413: for (frame_count = 0;
On 2014/06/24 00:58:25, Sergey Ulanov wrote:
> The body of this loop is not much different from the loop in line 402. Maybe
> merge them together? You'll just need to set start_time on 10th frame.

Done, though it ends up making the code less readable, IMHO

https://codereview.chromium.org/349223007/diff/20001/remoting/codec/codec_test.h
File remoting/codec/codec_test.h (right):

https://codereview.chromium.org/349223007/diff/20001/remoting/codec/codec_tes...
remoting/codec/codec_test.h:51: VideoEncoder* encoder, const
webrtc::DesktopSize& size);
On 2014/06/24 00:58:25, Sergey Ulanov wrote:
> one argument per line please

Done.

[Sergey Ulanov, 2014-06-24 02:00:51]

lgtm

https://codereview.chromium.org/349223007/diff/40001/remoting/codec/codec_tes...
File remoting/codec/codec_test.cc (right):

https://codereview.chromium.org/349223007/diff/40001/remoting/codec/codec_tes...
remoting/codec/codec_test.cc:387: VideoEncoder* encoder, const DesktopSize&
size) {
nit: one argument per line please

https://codereview.chromium.org/349223007/diff/40001/remoting/codec/video_enc...
File remoting/codec/video_encoder_vpx_unittest.cc (right):

https://codereview.chromium.org/349223007/diff/40001/remoting/codec/video_enc...
remoting/codec/video_encoder_vpx_unittest.cc:175: for (int lossless_mode = 0;
lossless_mode < 4; ++lossless_mode) {
nit: it might be more readable have list of tests cases and loop through them,
e.g.

 struct TestCase {
   bool lossless_color;
   bool lossless_encode;
   DesktopSize size;
 } test_cases[] = {
   {0, 0, DesktopSize(1280, 1024)},
   ....
 };

[Wez, 2014-06-24 02:14:13]

https://codereview.chromium.org/349223007/diff/40001/remoting/codec/codec_tes...
File remoting/codec/codec_test.cc (right):

https://codereview.chromium.org/349223007/diff/40001/remoting/codec/codec_tes...
remoting/codec/codec_test.cc:387: VideoEncoder* encoder, const DesktopSize&
size) {
On 2014/06/24 02:00:51, Sergey Ulanov wrote:
> nit: one argument per line please

Done.

https://codereview.chromium.org/349223007/diff/40001/remoting/codec/video_enc...
File remoting/codec/video_encoder_vpx_unittest.cc (right):

https://codereview.chromium.org/349223007/diff/40001/remoting/codec/video_enc...
remoting/codec/video_encoder_vpx_unittest.cc:175: for (int lossless_mode = 0;
lossless_mode < 4; ++lossless_mode) {
On 2014/06/24 02:00:51, Sergey Ulanov wrote:
> nit: it might be more readable have list of tests cases and loop through them,
> e.g.
> 
>  struct TestCase {
>    bool lossless_color;
>    bool lossless_encode;
>    DesktopSize size;
>  } test_cases[] = {
>    {0, 0, DesktopSize(1280, 1024)},
>    ....

I prefer to iterate over sizes and iterate over lossless modes orthogonally, I
think, but I reserve the right to change my mind and to follow your suggestion
next time I touch this test. :P

[Wez, 2014-06-24 02:14:17]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2014-06-24 02:15:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wez@chromium.org/349223007/60001

[commit-bot: I haz the power, 2014-06-24 05:54:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-24 07:35:57]

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

[commit-bot: I haz the power, 2014-06-24 07:35:58]

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

[Wez, 2014-06-25 03:42:20]

Looks like Chromium's test framework doesn't support MANUAL_ for normal
unit-tests; we can either break this down into a series of ~2s run-time
unit-tests, or create a separate remoting_perftests target.

[Wez, 2014-07-01 00:26:22]

I've moved the MeasureFPS tests into a new remoting_perftests target - WDYT?

[Sergey Ulanov, 2014-07-01 01:31:21]

still lgtm

[Wez, 2014-07-01 01:39:56]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2014-07-01 01:41:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wez@chromium.org/349223007/100001

[commit-bot: I haz the power, 2014-07-01 06:44:38]

Change committed as 280753