[Chromoting] Add a unit test to verify that the VP8 codec doesn't change color values too much.

[Chromoting] Add a unit test to verify that the VP8 codec doesn't change color values too much.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148779

[simonmorris, 2012-07-25 23:19:35]

ptal

[Sergey Ulanov, 2012-07-26 04:00:17]

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

http://codereview.chromium.org/10833022/diff/1/remoting/base/codec_test.cc#ne...
remoting/base/codec_test.cc:215: UpdateErrorMetrics(original, decoded,
max_error, sum_error,
PSNR is a standard metric that is used to measure image quality. Would it be
better to reuse it here?
One problem with the current approach is that it doesn't account for how visible
the errors to the human eye. E.g. eye is more sensitive to green than blue, but
here you use the same math for all three channels. PSNR is not perfect in that
respect, but it's much better.
There are already several implementations of PSNR calculation in our codebase,
e.g. here is one in libyuv:
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/libyuv/include...

One problem with reusing that code here is that you would have to convert the
image to YUV, but it should be easy as we already have libraries for that.

[simonmorris, 2012-07-26 17:32:42]

On 2012/07/26 04:00:17, sergeyu wrote:
> http://codereview.chromium.org/10833022/diff/1/remoting/base/codec_test.cc
> File remoting/base/codec_test.cc (right):
> 
>
http://codereview.chromium.org/10833022/diff/1/remoting/base/codec_test.cc#ne...
> remoting/base/codec_test.cc:215: UpdateErrorMetrics(original, decoded,
> max_error, sum_error,
> PSNR is a standard metric that is used to measure image quality. Would it be
> better to reuse it here?
> One problem with the current approach is that it doesn't account for how
visible
> the errors to the human eye. E.g. eye is more sensitive to green than blue,
but
> here you use the same math for all three channels. PSNR is not perfect in that
> respect, but it's much better.
> There are already several implementations of PSNR calculation in our codebase,
> e.g. here is one in libyuv:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/libyuv/include...
> 
> One problem with reusing that code here is that you would have to convert the
> image to YUV, but it should be easy as we already have libraries for that.

Sophisticated error metrics are useful for comparing codecs in detail, but I
just want to check that our use of VP8 doesn't add gross errors to the image. A
simple error metric is OK for that.

But I've made it use squared errors, not absolute errors. Wikipedia says PSNR
can be used on RGB images, and it's then equivalent to this code.

The max error is different, but that's to check we're not adding errors in e.g.
the last row of an image.

[simonmorris, 2012-07-27 01:11:34]

On 2012/07/26 17:32:42, simonmorris wrote:
> On 2012/07/26 04:00:17, sergeyu wrote:
> > http://codereview.chromium.org/10833022/diff/1/remoting/base/codec_test.cc
> > File remoting/base/codec_test.cc (right):
> > 
> >
>
http://codereview.chromium.org/10833022/diff/1/remoting/base/codec_test.cc#ne...
> > remoting/base/codec_test.cc:215: UpdateErrorMetrics(original, decoded,
> > max_error, sum_error,
> > PSNR is a standard metric that is used to measure image quality. Would it be
> > better to reuse it here?
> > One problem with the current approach is that it doesn't account for how
> visible
> > the errors to the human eye. E.g. eye is more sensitive to green than blue,
> but
> > here you use the same math for all three channels. PSNR is not perfect in
that
> > respect, but it's much better.
> > There are already several implementations of PSNR calculation in our
codebase,
> > e.g. here is one in libyuv:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/libyuv/include...
> > 
> > One problem with reusing that code here is that you would have to convert
the
> > image to YUV, but it should be easy as we already have libraries for that.
> 
> Sophisticated error metrics are useful for comparing codecs in detail, but I
> just want to check that our use of VP8 doesn't add gross errors to the image.
A
> simple error metric is OK for that.
> 
> But I've made it use squared errors, not absolute errors. Wikipedia says PSNR
> can be used on RGB images, and it's then equivalent to this code.
> 
> The max error is different, but that's to check we're not adding errors in
e.g.
> the last row of an image.

ping

[Sergey Ulanov, 2012-07-27 02:55:00]

LGTM when my nits are addressed.

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

http://codereview.chromium.org/10833022/diff/4001/remoting/base/codec_test.cc...
remoting/base/codec_test.cc:197: void VerifyResultsApprox(double
max_error_limit, double mean_error_limit) {
add short description of how the error is calculated (i.e. that it's average of
root mean square of 3 colors over all pixels in the image).

Also it might be worth separating error calculation from the tests - i.e. add
CalculateError() methods that would do all math separately from verification.

http://codereview.chromium.org/10833022/diff/4001/remoting/base/codec_test.cc...
remoting/base/codec_test.cc:403: uint8* p = memory + ((j * frame_size.width()) +
i) * 4;
nit: move this outside of the inner loop. Or alternatively assign to p[0], p[1],
p[2] and p[3] instead of incrementing p for each channel.

http://codereview.chromium.org/10833022/diff/4001/remoting/base/codec_test.cc...
remoting/base/codec_test.cc:413: void TestEncoderDecoderGradient(Encoder*
encoder, Decoder* decoder,
nit: move second parameter to a separate line

http://codereview.chromium.org/10833022/diff/4001/remoting/base/decoder_vp8_u...
File remoting/base/decoder_vp8_unittest.cc (right):

http://codereview.chromium.org/10833022/diff/4001/remoting/base/decoder_vp8_u...
remoting/base/decoder_vp8_unittest.cc:19: TEST(DecoderVp8Test, Gradient) {
add short description of the purpose of that test.

[simonmorris, 2012-07-27 16:42:23]

fyi

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

http://codereview.chromium.org/10833022/diff/4001/remoting/base/codec_test.cc...
remoting/base/codec_test.cc:197: void VerifyResultsApprox(double
max_error_limit, double mean_error_limit) {
On 2012/07/27 02:55:01, sergeyu wrote:
> add short description of how the error is calculated (i.e. that it's average
of
> root mean square of 3 colors over all pixels in the image).
> 
> Also it might be worth separating error calculation from the tests - i.e. add
> CalculateError() methods that would do all math separately from verification.

Done.

http://codereview.chromium.org/10833022/diff/4001/remoting/base/codec_test.cc...
remoting/base/codec_test.cc:403: uint8* p = memory + ((j * frame_size.width()) +
i) * 4;
On 2012/07/27 02:55:01, sergeyu wrote:
> nit: move this outside of the inner loop. Or alternatively assign to p[0],
p[1],
> p[2] and p[3] instead of incrementing p for each channel.

Done.

http://codereview.chromium.org/10833022/diff/4001/remoting/base/codec_test.cc...
remoting/base/codec_test.cc:413: void TestEncoderDecoderGradient(Encoder*
encoder, Decoder* decoder,
On 2012/07/27 02:55:01, sergeyu wrote:
> nit: move second parameter to a separate line

Done.

http://codereview.chromium.org/10833022/diff/4001/remoting/base/decoder_vp8_u...
File remoting/base/decoder_vp8_unittest.cc (right):

http://codereview.chromium.org/10833022/diff/4001/remoting/base/decoder_vp8_u...
remoting/base/decoder_vp8_unittest.cc:19: TEST(DecoderVp8Test, Gradient) {
On 2012/07/27 02:55:01, sergeyu wrote:
> add short description of the purpose of that test.

Done.

[commit-bot: I haz the power, 2012-07-27 16:42:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10833022/9003

[commit-bot: I haz the power, 2012-07-27 18:35:33]

Change committed as 148779