Add VideoProcessor interface.

Add VideoProcessor interface.

The new VideoProcessor interface abstracts video layer in chromoting client.
RectangleUpdateDecoder is the default implementation. Another implementation
will be added to suppord decoding using <video> and MediaSource API (or
Pepper video decode API if we decide to use it instead in the future).

BUG=321825
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246547

[Wez, 2014-01-14 16:23:14]

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
File remoting/client/rectangle_update_decoder.cc (right):

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.cc:140: ChromotingStats stats_;
If you move ProcessVideoPacket and OnDecodeDone back out to
RectangleUpdateDecoder, since they are called on the main thread, then you can
also move |stats_| out there and so avoid some boilerplate in Core.

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.cc:161: if
(!decode_task_runner_->BelongsToCurrentThread()) {
Since RUD runs on main thread and RUD::Core on decode thread, why not move the
PostTask out into RUD::Initialize(), to remove the need for the
BelongsToCurrentThread() check and make the thread flow easier to reason about?

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.cc:391:
&RectangleUpdateDecoder::Core::OnPacketDone, this, decode_start, done);
You can put all this code in RectangleUpdateDecoder::ProcessVideoPacket, since
it all runs on |main_task_runner_| (though stats will need moving out to
RectangleUpdateDecoder as well).

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.cc:400: if
(!main_task_runner_->BelongsToCurrentThread()) {
Since this method just thread-hops back to |main_task_runner_| to update the
stats and then runs |done| there, it would be cleaner to put this on
RectangleUpdateDecoder, to keep a clean separation between |decoder_task_runner|
and |main_task_runner| code. You'd need to PostTask() through a WeakPtr to the
RUD to do that, though.

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.cc:436:
core_->ProcessVideoPacket(packet.Pass(), done);
As with Initialize, where we're just proxying calls from the main thread into
the Core on the decode thread, why not just PostTask from the RUD::Foo method to
the RUD::Core::Foo implementation, and avoid the BelongsToCurrentThread check?
Only down-side is that we'll PostTask even if the threads are the same, but that
will never happen in practice, and I suspect the performance hit is negligible
compared to decode.

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
File remoting/client/rectangle_update_decoder.h (right):

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.h:26: // thread and then passes decoded
frames to a FrameConsumer./
nit: Lose the trailing /

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.h:28: // TODO(sergeyu): Rename this
class.
e.g. SoftwareVideoRenderer, since it's what we use to drive software decoders?
Or VideoDecodingFrameProducer, FrameProducingVideoRenderer, etc?

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.h:59: scoped_refptr<Core> core_;
refptr makes kitteh sad... !^.^

Can't Core be an owned object used and destroyed on the decode_task_runner?

https://codereview.chromium.org/136763009/diff/30001/remoting/client/video_pr...
File remoting/client/video_processor.h (right):

https://codereview.chromium.org/136763009/diff/30001/remoting/client/video_pr...
remoting/client/video_processor.h:20: class VideoProcessor : public
protocol::VideoStub {
Since implementations of this class are specifically intended to actually render
the supplied video stream, what about VideoOutput or VideoRenderer?

[Sergey Ulanov, 2014-01-15 00:58:17]

Also minor fixes in FrameConsumer/FrameProducer to make them thread-safe

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
File remoting/client/rectangle_update_decoder.cc (right):

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.cc:140: ChromotingStats stats_;
On 2014/01/14 16:23:14, Wez wrote:
> If you move ProcessVideoPacket and OnDecodeDone back out to
> RectangleUpdateDecoder, since they are called on the main thread, then you can
> also move |stats_| out there and so avoid some boilerplate in Core.

Done.

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.cc:161: if
(!decode_task_runner_->BelongsToCurrentThread()) {
On 2014/01/14 16:23:14, Wez wrote:
> Since RUD runs on main thread and RUD::Core on decode thread, why not move the
> PostTask out into RUD::Initialize(), to remove the need for the
> BelongsToCurrentThread() check and make the thread flow easier to reason
about?

Done.

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.cc:391:
&RectangleUpdateDecoder::Core::OnPacketDone, this, decode_start, done);
On 2014/01/14 16:23:14, Wez wrote:
> You can put all this code in RectangleUpdateDecoder::ProcessVideoPacket, since
> it all runs on |main_task_runner_| (though stats will need moving out to
> RectangleUpdateDecoder as well).

Done.

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.cc:400: if
(!main_task_runner_->BelongsToCurrentThread()) {
On 2014/01/14 16:23:14, Wez wrote:
> Since this method just thread-hops back to |main_task_runner_| to update the
> stats and then runs |done| there, it would be cleaner to put this on
> RectangleUpdateDecoder, to keep a clean separation between
|decoder_task_runner|
> and |main_task_runner| code. You'd need to PostTask() through a WeakPtr to the
> RUD to do that, though.

Done.

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.cc:436:
core_->ProcessVideoPacket(packet.Pass(), done);
On 2014/01/14 16:23:14, Wez wrote:
> As with Initialize, where we're just proxying calls from the main thread into
> the Core on the decode thread, why not just PostTask from the RUD::Foo method
to
> the RUD::Core::Foo implementation, and avoid the BelongsToCurrentThread check?
> Only down-side is that we'll PostTask even if the threads are the same, but
that
> will never happen in practice, and I suspect the performance hit is negligible
> compared to decode.

Done.

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
File remoting/client/rectangle_update_decoder.h (right):

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.h:26: // thread and then passes decoded
frames to a FrameConsumer./
On 2014/01/14 16:23:14, Wez wrote:
> nit: Lose the trailing /

Done.

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.h:28: // TODO(sergeyu): Rename this
class.
On 2014/01/14 16:23:14, Wez wrote:
> e.g. SoftwareVideoRenderer, since it's what we use to drive software decoders?
> Or VideoDecodingFrameProducer, FrameProducingVideoRenderer, etc?

Done.

https://codereview.chromium.org/136763009/diff/30001/remoting/client/rectangl...
remoting/client/rectangle_update_decoder.h:59: scoped_refptr<Core> core_;
On 2014/01/14 16:23:14, Wez wrote:
> refptr makes kitteh sad... !^.^
> 
> Can't Core be an owned object used and destroyed on the decode_task_runner?

Done.

https://codereview.chromium.org/136763009/diff/30001/remoting/client/video_pr...
File remoting/client/video_processor.h (right):

https://codereview.chromium.org/136763009/diff/30001/remoting/client/video_pr...
remoting/client/video_processor.h:20: class VideoProcessor : public
protocol::VideoStub {
On 2014/01/14 16:23:14, Wez wrote:
> Since implementations of this class are specifically intended to actually
render
> the supplied video stream, what about VideoOutput or VideoRenderer?

VideoRenderer it is.

[Wez, 2014-01-22 20:17:31]

lgtm

https://codereview.chromium.org/136763009/diff/180001/remoting/client/softwar...
File remoting/client/software_video_renderer.cc (right):

https://codereview.chromium.org/136763009/diff/180001/remoting/client/softwar...
remoting/client/software_video_renderer.cc:88: void
DrawBuffer(webrtc::DesktopFrame* buffer);
nit: Do you really need the blank line above this?

https://codereview.chromium.org/136763009/diff/180001/remoting/client/softwar...
File remoting/client/software_video_renderer.h (right):

https://codereview.chromium.org/136763009/diff/180001/remoting/client/softwar...
remoting/client/software_video_renderer.h:26: // FrameProducer methods can be
called on any thread. All other methods must be
How do we guarantee that that other thread doesn't call FrameProducer methods
after we've deleted this object - can we clarify that guarantee here?

https://codereview.chromium.org/136763009/diff/180001/remoting/client/softwar...
remoting/client/software_video_renderer.h:27: // called in the main thread.
nit: in -> on

https://codereview.chromium.org/136763009/diff/180001/remoting/client/video_r...
File remoting/client/video_renderer.h (right):

https://codereview.chromium.org/136763009/diff/180001/remoting/client/video_r...
remoting/client/video_renderer.h:25: // Return the stats recorded by this
client.
stats -> statistics

[Sergey Ulanov, 2014-01-23 01:02:42]

https://codereview.chromium.org/136763009/diff/180001/remoting/client/softwar...
File remoting/client/software_video_renderer.cc (right):

https://codereview.chromium.org/136763009/diff/180001/remoting/client/softwar...
remoting/client/software_video_renderer.cc:88: void
DrawBuffer(webrtc::DesktopFrame* buffer);
On 2014/01/22 20:17:31, Wez wrote:
> nit: Do you really need the blank line above this?

Done.

https://codereview.chromium.org/136763009/diff/180001/remoting/client/softwar...
File remoting/client/software_video_renderer.h (right):

https://codereview.chromium.org/136763009/diff/180001/remoting/client/softwar...
remoting/client/software_video_renderer.h:26: // FrameProducer methods can be
called on any thread. All other methods must be
On 2014/01/22 20:17:31, Wez wrote:
> How do we guarantee that that other thread doesn't call FrameProducer methods
> after we've deleted this object - can we clarify that guarantee here?

Done.

https://codereview.chromium.org/136763009/diff/180001/remoting/client/softwar...
remoting/client/software_video_renderer.h:27: // called in the main thread.
On 2014/01/22 20:17:31, Wez wrote:
> nit: in -> on

Done.

https://codereview.chromium.org/136763009/diff/180001/remoting/client/video_r...
File remoting/client/video_renderer.h (right):

https://codereview.chromium.org/136763009/diff/180001/remoting/client/video_r...
remoting/client/video_renderer.h:25: // Return the stats recorded by this
client.
On 2014/01/22 20:17:31, Wez wrote:
> stats -> statistics

Done.

[commit-bot: I haz the power, 2014-01-23 01:04:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/136763009/520001

[commit-bot: I haz the power, 2014-01-23 02:14:19]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2014-01-23 03:45:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/136763009/800001

[commit-bot: I haz the power, 2014-01-23 09:32:47]

Change committed as 246547