Update TestVideoRenderer with video decoding capabilities

Update TestVideoRenderer with video decoding capabilities

Spin up a separate thread to decode video packets.
Decode and Render video packets to a buffer in memory.
Add unit tests to verify video decoding for VP8, VP9, VERBATIM, as well as video packets size change.
Add unit tests to verify thread start and stop.

BUG=

Committed: https://crrev.com/8109cbcef96531823a65ccccddbc91108105bc53
Cr-Commit-Position: refs/heads/master@{#336897}

[liaoyuke, 2015-06-30 17:03:46]

liaoyuke@chromium.org changed reviewers:
+ joedow@chromium.org

[liaoyuke, 2015-06-30 17:03:47]

Hey Joe,

Please review a changelist from liaoyuke.

Thank you very much!

[joedow, 2015-06-30 17:52:27]

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
File remoting/test/test_video_renderer.cc (right):

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
remoting/test/test_video_renderer.cc:58:
scoped_refptr<base::SingleThreadTaskRunner> renderer_task_runner_;
Can you rename 'renderer_task_runner_' to 'main_task_runner_'.  The Renderer may
not own the thread.

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
remoting/test/test_video_renderer.cc:161: FROM_HERE,
base::Bind(&TestVideoRenderer::OnPacketDone, renderer_));
This would be a lot cleaner if we reuse the done callback passed in from the
caller.

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
File remoting/test/test_video_renderer.h (right):

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
remoting/test/test_video_renderer.h:44: void SetDecoder(const
protocol::ChannelConfig::Codec codec);
Should this be renamed "SetCodecForDecoding"?

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
remoting/test/test_video_renderer.h:46: // Runs a RunLoop until the video packet
is processed.
Interface comments shouldn't document internal implementations.  Can you update
this comment such that it describes what the method does (instead of how)?

Actually, now that I think about it, this method may not be necessary.  A better
mechanism would be for ProcessVideoPacket to pass the done closure to the
decoding thread and then have the decoding thread (once it has done its work)
post the done callback back to the main thread.  (Check out how this is done in
SoftwareVideoRenderer.cc (lines 349, 165)

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
remoting/test/test_video_renderer.h:61: base::Closure main_closure_;
I think we should go with the approach I mention above, then you can get rid of
special case members like this which are only used for testing.

[liaoyuke, 2015-06-30 19:35:00]

Hey Joe,

Please take another look at patch 2.

Thank you very much!

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
File remoting/test/test_video_renderer.cc (right):

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
remoting/test/test_video_renderer.cc:58:
scoped_refptr<base::SingleThreadTaskRunner> renderer_task_runner_;
On 2015/06/30 17:52:27, joedow wrote:
> Can you rename 'renderer_task_runner_' to 'main_task_runner_'.  The Renderer
may
> not own the thread.

Done.

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
remoting/test/test_video_renderer.cc:161: FROM_HERE,
base::Bind(&TestVideoRenderer::OnPacketDone, renderer_));
On 2015/06/30 17:52:27, joedow wrote:
> This would be a lot cleaner if we reuse the done callback passed in from the
> caller.

Done.

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
File remoting/test/test_video_renderer.h (right):

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
remoting/test/test_video_renderer.h:44: void SetDecoder(const
protocol::ChannelConfig::Codec codec);
On 2015/06/30 17:52:27, joedow wrote:
> Should this be renamed "SetCodecForDecoding"?

Done.

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
remoting/test/test_video_renderer.h:46: // Runs a RunLoop until the video packet
is processed.
On 2015/06/30 17:52:27, joedow wrote:
> Interface comments shouldn't document internal implementations.  Can you
update
> this comment such that it describes what the method does (instead of how)?
> 
> Actually, now that I think about it, this method may not be necessary.  A
better
> mechanism would be for ProcessVideoPacket to pass the done closure to the
> decoding thread and then have the decoding thread (once it has done its work)
> post the done callback back to the main thread.  (Check out how this is done
in
> SoftwareVideoRenderer.cc (lines 349, 165)

Done.

https://codereview.chromium.org/1214113009/diff/1/remoting/test/test_video_re...
remoting/test/test_video_renderer.h:61: base::Closure main_closure_;
On 2015/06/30 17:52:27, joedow wrote:
> I think we should go with the approach I mention above, then you can get rid
of
> special case members like this which are only used for testing.

Done.

[joedow, 2015-06-30 20:07:12]

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
File remoting/test/test_video_renderer.cc (right):

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer.cc:36: void SetDecoderForDecoding(const
protocol::ChannelConfig::Codec codec);
SetCodec or SetCodecForDecoding

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer.cc:43: // thread.
nit: Can you shorten the comment so it fits on one line.

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
File remoting/test/test_video_renderer.h (right):

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer.h:28: // Processes video packets as they are
received from the remote host.
Can you add a comment here about how threads and TaskRunners are managed by this
class.

Basically it is important to say that this must be created on a thread with an
existing MessageLoop and that this calss will use that message loop to execute
the done callbacks passed by the caller of ProcessVideoPacket. 
remote_host_info_fetcher.h has an example of the type of info that should be
included.

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer.h:44: void SetDecoderForDecoding(const
protocol::ChannelConfig::Codec codec);
SetCodecForDecoding (or just SetCodec)

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
File remoting/test/test_video_renderer_unittest.cc (right):

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer_unittest.cc:104:
EXPECT_TRUE(original_frame.get());
no need for .get() here

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer_unittest.cc:109: // wait for the video packet
to be processed and rendered to buffer.
capitalize W in wait

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer_unittest.cc:135: // R, G and B components,
each normalized to [0, 1]
add ending period to comment

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer_unittest.cc:237: int task_num = 20;
should task_num be const?

[liaoyuke, 2015-06-30 21:15:02]

Hey Joe,

Please take another look at Patch 3.

Thank you very much!

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
File remoting/test/test_video_renderer.cc (right):

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer.cc:36: void SetDecoderForDecoding(const
protocol::ChannelConfig::Codec codec);
On 2015/06/30 20:07:11, joedow wrote:
> SetCodec or SetCodecForDecoding

Done.

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer.cc:43: // thread.
On 2015/06/30 20:07:12, joedow wrote:
> nit: Can you shorten the comment so it fits on one line.

Done.

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
File remoting/test/test_video_renderer.h (right):

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer.h:44: void SetDecoderForDecoding(const
protocol::ChannelConfig::Codec codec);
On 2015/06/30 20:07:12, joedow wrote:
> SetCodecForDecoding (or just SetCodec)

Done.

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
File remoting/test/test_video_renderer_unittest.cc (right):

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer_unittest.cc:104:
EXPECT_TRUE(original_frame.get());
On 2015/06/30 20:07:12, joedow wrote:
> no need for .get() here

Done.

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer_unittest.cc:109: // wait for the video packet
to be processed and rendered to buffer.
On 2015/06/30 20:07:12, joedow wrote:
> capitalize W in wait

Done.

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer_unittest.cc:135: // R, G and B components,
each normalized to [0, 1]
On 2015/06/30 20:07:12, joedow wrote:
> add ending period to comment

Done.

https://codereview.chromium.org/1214113009/diff/20001/remoting/test/test_vide...
remoting/test/test_video_renderer_unittest.cc:237: int task_num = 20;
On 2015/06/30 20:07:12, joedow wrote:
> should task_num be const?

Done.

[joedow, 2015-06-30 21:25:16]

lgtm

[liaoyuke, 2015-06-30 22:24:26]

The CQ bit was checked by liaoyuke@chromium.org

[commit-bot: I haz the power, 2015-06-30 22:27:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1214113009/40001

[commit-bot: I haz the power, 2015-06-30 22:38:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-30 22:39:25]

Patchset 3 (id:??) landed as
https://crrev.com/8109cbcef96531823a65ccccddbc91108105bc53
Cr-Commit-Position: refs/heads/master@{#336897}