[Chromoting] Add unit tests for up- and down-scaling.

[Chromoting] Add unit tests for up- and down-scaling.


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

[simonmorris, 2012-07-27 21:20:07]

ptal

[Sergey Ulanov, 2012-07-27 22:29:37]

LGTM provided my nits are addressed.

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

http://codereview.chromium.org/10828058/diff/1/remoting/base/codec_test.cc#ne...
remoting/base/codec_test.cc:24: std::vector<SkIRect> makeTestRects(const
SkISize& size) {
MakeTestRects()

http://codereview.chromium.org/10828058/diff/1/remoting/base/codec_test.cc#ne...
remoting/base/codec_test.cc:33: };
nit: don't need semicolon here.

http://codereview.chromium.org/10828058/diff/1/remoting/base/codec_test.cc#ne...
remoting/base/codec_test.cc:365: TestEncodingRects(encoder, &tester, data,
&test_rects[0], 1);
Would it be better to have for loop to iterate over test_rects?

http://codereview.chromium.org/10828058/diff/1/remoting/base/codec_test.cc#ne...
remoting/base/codec_test.cc:421: TestEncodeDecodeRects(encoder, &encoder_tester,
&decoder_tester, data,
for loop here in case we add some other rects in makeTestRects.

http://codereview.chromium.org/10828058/diff/1/remoting/base/codec_test.cc#ne...
remoting/base/codec_test.cc:485:
decoder_tester.VerifyResultsApprox(expected_view_data.get(),
Not sure why we need this. Is it to make sure that the decoder can rerender the
frame when we ask it too? Add comments please

http://codereview.chromium.org/10828058/diff/1/remoting/base/decoder_vp8_unit...
File remoting/base/decoder_vp8_unittest.cc (right):

http://codereview.chromium.org/10828058/diff/1/remoting/base/decoder_vp8_unit...
remoting/base/decoder_vp8_unittest.cc:62: // different from the error values
after DecodePacket, which is unexpected.
Open a bug?

[simonmorris, 2012-07-27 23:14:11]

fyi

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

http://codereview.chromium.org/10828058/diff/1/remoting/base/codec_test.cc#ne...
remoting/base/codec_test.cc:24: std::vector<SkIRect> makeTestRects(const
SkISize& size) {
On 2012/07/27 22:29:37, sergeyu wrote:
> MakeTestRects()

Done.

http://codereview.chromium.org/10828058/diff/1/remoting/base/codec_test.cc#ne...
remoting/base/codec_test.cc:33: };
On 2012/07/27 22:29:37, sergeyu wrote:
> nit: don't need semicolon here.

Done.

http://codereview.chromium.org/10828058/diff/1/remoting/base/codec_test.cc#ne...
remoting/base/codec_test.cc:365: TestEncodingRects(encoder, &tester, data,
&test_rects[0], 1);
On 2012/07/27 22:29:37, sergeyu wrote:
> Would it be better to have for loop to iterate over test_rects?

Done.

http://codereview.chromium.org/10828058/diff/1/remoting/base/codec_test.cc#ne...
remoting/base/codec_test.cc:421: TestEncodeDecodeRects(encoder, &encoder_tester,
&decoder_tester, data,
On 2012/07/27 22:29:37, sergeyu wrote:
> for loop here in case we add some other rects in makeTestRects.

Done.

http://codereview.chromium.org/10828058/diff/1/remoting/base/codec_test.cc#ne...
remoting/base/codec_test.cc:485:
decoder_tester.VerifyResultsApprox(expected_view_data.get(),
On 2012/07/27 22:29:37, sergeyu wrote:
> Not sure why we need this. Is it to make sure that the decoder can rerender
the
> frame when we ask it too? Add comments please

Done.

http://codereview.chromium.org/10828058/diff/1/remoting/base/decoder_vp8_unit...
File remoting/base/decoder_vp8_unittest.cc (right):

http://codereview.chromium.org/10828058/diff/1/remoting/base/decoder_vp8_unit...
remoting/base/decoder_vp8_unittest.cc:62: // different from the error values
after DecodePacket, which is unexpected.
On 2012/07/27 22:29:37, sergeyu wrote:
> Open a bug?

Done.

[commit-bot: I haz the power, 2012-07-27 23:14:40]

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

[commit-bot: I haz the power, 2012-07-28 00:48:24]

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

[commit-bot: I haz the power, 2012-07-28 06:40:47]

Change committed as 148888