Update VP8 encode options to speed up encoding for remoting

Update VP8 encode options to speed up encoding for remoting

More details changes made in this patch:
g_profile: 1 -> 2
  Will use a more simplified profile similar to H264 base profile
g_threads: 1 -> 2
  Use 2 threads for encoding, it was always intended to do this but disabled before due to a bug in libvpx
cpuused: 13->16
  Disable half pixel motion vector and use simple loop filtering

Quality for static pictures will stay roughly the same but encoding time will be halved for the most common scenario. Quality of motion picture will suffer with this patch, but not by a large factor due to the fact that we were using real time encoding mode already. So This really trims the parts that we don't need.

BUG=None
TEST=None

[Jamie, 2011-05-28 00:04:44]

When you say "disable more VP8 quality related features", does this mean that
the image quality will suffer? Do we have any guidelines regarding speed/quality
trade-offs? The image quality is already pretty poor for some text colour
combinations, I'd hate to see it get worse.

[Wez, 2011-05-28 00:09:05]

http://codereview.chromium.org/7084013/diff/1/remoting/base/encoder_vp8.cc
File remoting/base/encoder_vp8.cc (right):

http://codereview.chromium.org/7084013/diff/1/remoting/base/encoder_vp8.cc#ne...
remoting/base/encoder_vp8.cc:102: // TODO(hclam): Tune the parameters to better
suit the application.
Can we elaborate more here on what tweaks we're making, please?

http://codereview.chromium.org/7084013/diff/1/remoting/base/encoder_vp8.cc#ne...
remoting/base/encoder_vp8.cc:118: // More advanced options of vp8.
Again, it will be helpful for others if the comment gives a vague idea of what
these options are intended to change.

[hclam, 2011-05-28 00:11:31]

The 'quality' that is referred here is with motion vector and loop
filtering. It is different than quantization which is most noticable by
eyes.

We were already using level 13, now changing it to 16 will use simple loop
filtering and disable half pixel motion vector. These options really just
affect the quality of motion pictures but not with text, for example simple
loop filtering will have more blocky images for motion.

In terms of trade-offs, it really is based on visual inspection.

2011年5月27日17:04 <jamiewalch@chromium.org>:

> When you say "disable more VP8 quality related features", does this mean
> that
> the image quality will suffer? Do we have any guidelines regarding
> speed/quality
> trade-offs? The image quality is already pretty poor for some text colour
> combinations, I'd hate to see it get worse.
>
>
> http://codereview.chromium.org/7084013/
>



-- 
α

[dmac, 2011-05-28 00:16:33]

http://codereview.chromium.org/7084013/diff/4001/remoting/base/encoder_vp8.cc
File remoting/base/encoder_vp8.cc (right):

http://codereview.chromium.org/7084013/diff/4001/remoting/base/encoder_vp8.cc...
remoting/base/encoder_vp8.cc:116: if (vpx_codec_control(codec_.get(),
VP8E_SET_CPUUSED, 16))
Can we put some comments here as to why these values were chosen? (in both
cases).

[Alpha Left Google, 2011-05-28 00:21:42]

On 2011/05/28 00:16:33, dmac wrote:
> http://codereview.chromium.org/7084013/diff/4001/remoting/base/encoder_vp8.cc
> File remoting/base/encoder_vp8.cc (right):
> 
>
http://codereview.chromium.org/7084013/diff/4001/remoting/base/encoder_vp8.cc...
> remoting/base/encoder_vp8.cc:116: if (vpx_codec_control(codec_.get(),
> VP8E_SET_CPUUSED, 16))
> Can we put some comments here as to why these values were chosen? (in both
> cases).

I'd prefer to explain the changes in the CL comments rather than in the code,
see the update in the description of this CL.

[Wez, 2011-05-28 00:59:14]

On 2011/05/28 00:21:42, Alpha wrote:
> On 2011/05/28 00:16:33, dmac wrote:
> >
http://codereview.chromium.org/7084013/diff/4001/remoting/base/encoder_vp8.cc
> > File remoting/base/encoder_vp8.cc (right):
> > 
> >
>
http://codereview.chromium.org/7084013/diff/4001/remoting/base/encoder_vp8.cc...
> > remoting/base/encoder_vp8.cc:116: if (vpx_codec_control(codec_.get(),
> > VP8E_SET_CPUUSED, 16))
> > Can we put some comments here as to why these values were chosen? (in both
> > cases).
> 
> I'd prefer to explain the changes in the CL comments rather than in the code,
> see the update in the description of this CL.

It seems better to document the intent of the configuration in comments, rather
than rely on folks reading the code to read this CL.  Without comments, it'll be
hard for someone coming to the code in future to work out whether these
configuration changes make sense, e.g. if VPX is updated in future to
dynamically manage a thread pool.

[dmac, 2011-06-16 17:48:03]

On 2011/05/28 00:59:14, Wez wrote:
> On 2011/05/28 00:21:42, Alpha wrote:
> > On 2011/05/28 00:16:33, dmac wrote:
> > >
> http://codereview.chromium.org/7084013/diff/4001/remoting/base/encoder_vp8.cc
> > > File remoting/base/encoder_vp8.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7084013/diff/4001/remoting/base/encoder_vp8.cc...
> > > remoting/base/encoder_vp8.cc:116: if (vpx_codec_control(codec_.get(),
> > > VP8E_SET_CPUUSED, 16))
> > > Can we put some comments here as to why these values were chosen? (in both
> > > cases).
> > 
> > I'd prefer to explain the changes in the CL comments rather than in the
code,
> > see the update in the description of this CL.
> 
> It seems better to document the intent of the configuration in comments,
rather
> than rely on folks reading the code to read this CL.  Without comments, it'll
be
> hard for someone coming to the code in future to work out whether these
> configuration changes make sense, e.g. if VPX is updated in future to
> dynamically manage a thread pool.

Anything happening with this?

[Alpha Left Google, 2011-06-16 17:50:13]

Will update this patch shortly.

[Alpha Left Google, 2011-06-20 18:49:02]

Patch updated with comments in the code.

[Wez, 2011-06-20 19:55:00]

LGTM, but please check the comments below.

http://codereview.chromium.org/7084013/diff/9001/remoting/base/encoder_vp8.cc
File remoting/base/encoder_vp8.cc (right):

http://codereview.chromium.org/7084013/diff/9001/remoting/base/encoder_vp8.cc...
remoting/base/encoder_vp8.cc:111: config.g_profile = 2;
Is the value 2 not defined somewhere as e.g. kVpxRealTimeProfile or something?

If we explicitly select real-time mode when doing encoding, why do we need this
here?  Does the mode we select when encoding override the config, in which case
should we get rid of that override and just set the profile here?

http://codereview.chromium.org/7084013/diff/9001/remoting/base/encoder_vp8.cc...
remoting/base/encoder_vp8.cc:114: config.g_threads = 2;
nit: Why use 2, not 3 or 4 or 7?  I'd expect a comment like:

// Using two threads gives N% speedup on multi-core systems, with only M% loss
on single-core systems.

or

// Using two threads gives N% speedup; more than two gives less significant
benefit.  We don't care about single-core systems.

[Lambros, 2011-06-20 20:02:07]

http://codereview.chromium.org/7084013/diff/9001/remoting/base/encoder_vp8.cc
File remoting/base/encoder_vp8.cc (right):

http://codereview.chromium.org/7084013/diff/9001/remoting/base/encoder_vp8.cc...
remoting/base/encoder_vp8.cc:114: config.g_threads = 2;
Drive-by: I'd also like to know where the numbers below (20, 30, 1, 20) came
from.
On 2011/06/20 19:55:01, Wez wrote:
> nit: Why use 2, not 3 or 4 or 7?  I'd expect a comment like:
> 
> // Using two threads gives N% speedup on multi-core systems, with only M% loss
> on single-core systems.
> 
> or
> 
> // Using two threads gives N% speedup; more than two gives less significant
> benefit.  We don't care about single-core systems.

[Alpha Left Google, 2011-06-20 20:02:07]

http://codereview.chromium.org/7084013/diff/9001/remoting/base/encoder_vp8.cc
File remoting/base/encoder_vp8.cc (right):

http://codereview.chromium.org/7084013/diff/9001/remoting/base/encoder_vp8.cc...
remoting/base/encoder_vp8.cc:111: config.g_profile = 2;
On 2011/06/20 19:55:01, Wez wrote:
> Is the value 2 not defined somewhere as e.g. kVpxRealTimeProfile or something?
> 
> If we explicitly select real-time mode when doing encoding, why do we need
this
> here?  Does the mode we select when encoding override the config, in which
case
> should we get rid of that override and just set the profile here?

The value is not defined in the VP8 API. It basically changes context for
different codecs in libvpx. I set it here because this is one of the basic
settings, even though it is overrided by VPX_DL_REALTIME below so we know what
is in the settings.

[Timur Iskhodzhanov, 2011-06-21 07:53:50]

Mr. TSan-bot didn't doesn't like it.
http://build.chromium.org/p/chromium.memory/builders/Linux%20Tests%20%28tsan%...
http://build.chromium.org/p/chromium.memory/builders/Chromium%20Mac%20%28tsan...