VideoTrackRecorder: limit VP9 cpu usage and quality

VideoTrackRecorder: limit VP9 cpu usage and quality

Also this CL simplifies the way VideoTrackRecorderTests
deals with the expectations, in the same way as
MediaRecorderHandlerTest was simplified before in
https://codereview.chromium.org/1579693006

BUG=589162
Committed: https://crrev.com/5033045bedc860eaddbd3b5f26b1777de829d83c
Cr-Commit-Position: refs/heads/master@{#378085}

[mcasas, 2016-02-24 20:02:21]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-02-24 20:02:43]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-02-24 20:02:59]

Patchset #1 (id:40001) has been deleted

[mcasas, 2016-02-25 00:18:39]

Patchset #2 (id:80001) has been deleted

[mcasas, 2016-02-25 00:54:27]

Patchset #2 (id:100001) has been deleted

[mcasas, 2016-02-25 01:20:27]

Description was changed from

==========
VideoTrackRecorder: limit VP9 cpu usage and quality

BUG=589162
==========

to

==========
VideoTrackRecorder: limit VP9 cpu usage and quality

Also this CL simplifies the way VideoTrackRecorderTests 
deals with the expectations, in the same way as 
MediaRecorderHandlerTest was simplified before in
https://codereview.chromium.org/1579693006

BUG=589162
==========

[mcasas, 2016-02-25 02:04:24]

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

[mcasas, 2016-02-25 02:04:25]

miu@ PTAL

[miu, 2016-02-25 20:22:09]

https://codereview.chromium.org/1732083002/diff/140001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/1732083002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:292: result =
vpx_codec_control(encoder_.get(), VP8E_SET_CPUUSED, kCpuUsage);
6 is actually a high-end setting (4 being the highest, IIRC).  Most users
machines will not be able to encode in real-time at this setting.  If your goal
is to reduce CPU usage, you probably will want to set this to 12, but then
you'll take a quality hit.

(In media/cast code, we actually micro-manage this setting, instead of using the
codec's built-in logic, to make the trade-off for each frame.)

Also, I noticed you're not setting |codec_config_.gthreads|, so perhaps you're
not leveraging multi-threaded encode right now?  For media/cast, we set this to
ceil(num processor cores / 2).

https://codereview.chromium.org/1732083002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:296:
codec_config_.rc_min_quantizer = 20;
When you clamp the quantizer values this way, the encoder can and will use
waaaay more bandwidth than your target bandwidth setting (could be 5X or more!).
 It's generally best to keep these at their defaults.

[mcasas, 2016-02-26 01:34:32]

PTAL

https://codereview.chromium.org/1732083002/diff/140001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/1732083002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:292: result =
vpx_codec_control(encoder_.get(), VP8E_SET_CPUUSED, kCpuUsage);
On 2016/02/25 20:22:09, miu wrote:
> 6 is actually a high-end setting (4 being the highest, IIRC).  Most users
> machines will not be able to encode in real-time at this setting.  If your
goal
> is to reduce CPU usage, you probably will want to set this to 12, but then
> you'll take a quality hit.

Let's go for a compromise of 10. Users can still use
the bitrate parameter.

> 
> (In media/cast code, we actually micro-manage this setting, instead of using
the
> codec's built-in logic, to make the trade-off for each frame.)
> 
> Also, I noticed you're not setting |codec_config_.gthreads|, so perhaps you're
> not leveraging multi-threaded encode right now?  For media/cast, we set this
to
> ceil(num processor cores / 2).
It's done in l.327.

https://codereview.chromium.org/1732083002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:296:
codec_config_.rc_min_quantizer = 20;
On 2016/02/25 20:22:09, miu wrote:
> When you clamp the quantizer values this way, the encoder can and will use
> waaaay more bandwidth than your target bandwidth setting (could be 5X or
more!).
>  It's generally best to keep these at their defaults.

Done.

[miu, 2016-02-26 19:27:49]

I'll lgtm to unblock, but do consider the following:

https://codereview.chromium.org/1732083002/diff/160001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/1732083002/diff/160001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:291: const int kCpuUsage = 10;
It's important you understand what this does: Internally, this is a parameter
(X) to the following formula:

  Target Encode CPU Time = Frame Duration * (16 - X) / 16

For example, if the frame is 33 ms in duration (30 FPS content) and you use a
CPU speed of 10, the encoder would TARGET spending 12.4 ms of CPU time to encode
the frame.  It would skip steps that improve quality and reduce bandwidth
utilization in order to avoid going over that target.

If CPU speed were 6, all other values the same, it would spend 20.6 ms (almost
double) to encode the frame.

So, the question is: What behavior do you really want here?  You'll want to
consider what you need to be leaving CPU available for (e.g., Chrome needs some
CPU for compositing, web rendering, and other things; plus we need to leave
slack for applications other than Chrome running on the system).  One possible
scheme:

  if (number_of_proc_cores == 1)
    vpx_codec_control(set cpu speed = 12);
  else if (number of proc cores <= 2)
    vpx_codec_control(set cpu speed = 10);
  else if (number_of_proc_cores <= 4)
    vpx_codec_control(set cpu speed = 8);
  else
    vpx_codec_control(set cpu speed = 6);

[mcasas, 2016-02-26 22:18:22]

https://codereview.chromium.org/1732083002/diff/160001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/1732083002/diff/160001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:291: const int kCpuUsage = 10;
On 2016/02/26 19:27:49, miu wrote:
> It's important you understand what this does: Internally, this is a parameter
> (X) to the following formula:
> 
>   Target Encode CPU Time = Frame Duration * (16 - X) / 16
> 
> For example, if the frame is 33 ms in duration (30 FPS content) and you use a
> CPU speed of 10, the encoder would TARGET spending 12.4 ms of CPU time to
encode
> the frame.  It would skip steps that improve quality and reduce bandwidth
> utilization in order to avoid going over that target.
> 
> If CPU speed were 6, all other values the same, it would spend 20.6 ms (almost
> double) to encode the frame.
> 
> So, the question is: What behavior do you really want here?  You'll want to
> consider what you need to be leaving CPU available for (e.g., Chrome needs
some
> CPU for compositing, web rendering, and other things; plus we need to leave
> slack for applications other than Chrome running on the system).  One possible
> scheme:
> 
>   if (number_of_proc_cores == 1)
>     vpx_codec_control(set cpu speed = 12);
>   else if (number of proc cores <= 2)
>     vpx_codec_control(set cpu speed = 10);
>   else if (number_of_proc_cores <= 4)
>     vpx_codec_control(set cpu speed = 8);
>   else
>     vpx_codec_control(set cpu speed = 6);

Hmmm talking about implementation details
leaking... :)

Ok, the goal here is to move the dial away
from the 0, which basically means "take the
whole frame interval" to "leave a bit of slack
if there are few cores". If possible, without
degrading the quality significantly. Since we
are dealing with relatively static scenes 
(vid calls) we should achieve this results
easily, and I think the new set of numbers
would make sense. SGTY?

[miu, 2016-02-26 23:55:22]

On 2016/02/26 22:18:22, mcasas wrote:
> Hmmm talking about implementation details
> leaking... :)

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libvpx...

> easily, and I think the new set of numbers
> would make sense. SGTY?

lgtm!

[mcasas, 2016-02-27 00:13:23]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-02-27 00:19:40]

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

[commit-bot: I haz the power, 2016-02-27 01:20:52]

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

[commit-bot: I haz the power, 2016-02-27 01:20:53]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mcasas, 2016-02-27 01:24:34]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-02-27 01:33:16]

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

[commit-bot: I haz the power, 2016-02-27 02:22:41]

Description was changed from

==========
VideoTrackRecorder: limit VP9 cpu usage and quality

Also this CL simplifies the way VideoTrackRecorderTests 
deals with the expectations, in the same way as 
MediaRecorderHandlerTest was simplified before in
https://codereview.chromium.org/1579693006

BUG=589162
==========

to

==========
VideoTrackRecorder: limit VP9 cpu usage and quality

Also this CL simplifies the way VideoTrackRecorderTests 
deals with the expectations, in the same way as 
MediaRecorderHandlerTest was simplified before in
https://codereview.chromium.org/1579693006

BUG=589162
==========

[commit-bot: I haz the power, 2016-02-27 02:22:43]

Committed patchset #5 (id:180001)

[commit-bot: I haz the power, 2016-02-27 02:24:06]

Description was changed from

==========
VideoTrackRecorder: limit VP9 cpu usage and quality

Also this CL simplifies the way VideoTrackRecorderTests 
deals with the expectations, in the same way as 
MediaRecorderHandlerTest was simplified before in
https://codereview.chromium.org/1579693006

BUG=589162
==========

to

==========
VideoTrackRecorder: limit VP9 cpu usage and quality

Also this CL simplifies the way VideoTrackRecorderTests
deals with the expectations, in the same way as
MediaRecorderHandlerTest was simplified before in
https://codereview.chromium.org/1579693006

BUG=589162

Committed: https://crrev.com/5033045bedc860eaddbd3b5f26b1777de829d83c
Cr-Commit-Position: refs/heads/master@{#378085}
==========

[commit-bot: I haz the power, 2016-02-27 02:24:07]

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