Cleanup RectangleUpdateDecoder and VideoStub

Cleanup RectangleUpdateDecoder and VideoStub

Previously RectangleUpdateDecoder maintaned list of pending packets. That's not really necessary.
Simplified it by queue tasks on the decode thread. Also some minor cleanups for the VideoStub 
interface - removed GetPendingVideoPackets() that wasn't used anywhere.

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

[Wez, 2012-08-28 16:42:27]

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
File remoting/client/rectangle_update_decoder.cc (right):

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
remoting/client/rectangle_update_decoder.cc:61:
DCHECK(main_task_runner_->BelongsToCurrentThread());
nit: Blank line after DCHECK.

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
remoting/client/rectangle_update_decoder.cc:64: // if the decode thread sees
false instead of true.
Why not just re-post the Stop() to set this on the decode thread, so you know
that it'll be seen as soon as that gets processed?

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
remoting/client/rectangle_update_decoder.cc:274: base::Closure callback =
nit: callback -> decode_done? (More consistent with |done|).

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
File remoting/client/rectangle_update_decoder.h (right):

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
remoting/client/rectangle_update_decoder.h:116: // Set to true after Stop() has
been called.
nit: True after Stop() has been called.
The current wording sounds like an instruction. :)

http://codereview.chromium.org/10879085/diff/1/remoting/protocol/video_stub.h
File remoting/protocol/video_stub.h (right):

http://codereview.chromium.org/10879085/diff/1/remoting/protocol/video_stub.h...
remoting/protocol/video_stub.h:23: // struct and use it to represent video
packets internally.
nit: Remove this comment; the stubs are defined in the protocol layer right now,
and even if they weren't, they'd be in a higher layer that can reasonably depend
on the protocol layer. Protobufs already abstract sufficiently from the
on-the-wire layout that it's not worth the extra layer of abstraction.

[Sergey Ulanov, 2012-08-28 18:30:26]

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
File remoting/client/rectangle_update_decoder.cc (right):

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
remoting/client/rectangle_update_decoder.cc:61:
DCHECK(main_task_runner_->BelongsToCurrentThread());
On 2012/08/28 16:42:27, Wez wrote:
> nit: Blank line after DCHECK.

Done.

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
remoting/client/rectangle_update_decoder.cc:64: // if the decode thread sees
false instead of true.
On 2012/08/28 16:42:27, Wez wrote:
> Why not just re-post the Stop() to set this on the decode thread, so you know
> that it'll be seen as soon as that gets processed?

Posting a task on the decoder thread will not do what we want here. We want to
stop decoding of the frames that are already in the task queue for the decode
thread. If I post a task to the decode thread it will only be executed after all
pending decoder tasks.

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
remoting/client/rectangle_update_decoder.cc:274: base::Closure callback =
On 2012/08/28 16:42:27, Wez wrote:
> nit: callback -> decode_done? (More consistent with |done|).

Done.

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
File remoting/client/rectangle_update_decoder.h (right):

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
remoting/client/rectangle_update_decoder.h:116: // Set to true after Stop() has
been called.
On 2012/08/28 16:42:27, Wez wrote:
> nit: True after Stop() has been called.
> The current wording sounds like an instruction. :)

Done.

http://codereview.chromium.org/10879085/diff/1/remoting/protocol/video_stub.h
File remoting/protocol/video_stub.h (right):

http://codereview.chromium.org/10879085/diff/1/remoting/protocol/video_stub.h...
remoting/protocol/video_stub.h:23: // struct and use it to represent video
packets internally.
On 2012/08/28 16:42:27, Wez wrote:
> nit: Remove this comment; the stubs are defined in the protocol layer right
now,
> and even if they weren't, they'd be in a higher layer that can reasonably
depend
> on the protocol layer. Protobufs already abstract sufficiently from the
> on-the-wire layout that it's not worth the extra layer of abstraction.

Done.

[Sergey Ulanov, 2012-08-29 23:01:11]

wez: ping

[Wez, 2012-08-29 23:18:05]

lgtm

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
File remoting/client/rectangle_update_decoder.cc (right):

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
remoting/client/rectangle_update_decoder.cc:64: // if the decode thread sees
false instead of true.
On 2012/08/28 18:30:27, sergeyu wrote:
> On 2012/08/28 16:42:27, Wez wrote:
> > Why not just re-post the Stop() to set this on the decode thread, so you
know
> > that it'll be seen as soon as that gets processed?
> 
> Posting a task on the decoder thread will not do what we want here. We want to
> stop decoding of the frames that are already in the task queue for the decode
> thread. If I post a task to the decode thread it will only be executed after
all
> pending decoder tasks.

OK, but you've stated that we don't need to lock here because the it's not an
issue if the thread sees false instead of true; if the optimization of stopping
processing on the other thread is valuable then it's worth putting a lock around
this to create a memory barrier and ensure that the other thread sees the
change.

[Sergey Ulanov, 2012-08-29 23:31:10]

PTAL

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
File remoting/client/rectangle_update_decoder.cc (right):

http://codereview.chromium.org/10879085/diff/1/remoting/client/rectangle_upda...
remoting/client/rectangle_update_decoder.cc:64: // if the decode thread sees
false instead of true.
On 2012/08/29 23:18:05, Wez wrote:
> On 2012/08/28 18:30:27, sergeyu wrote:
> > On 2012/08/28 16:42:27, Wez wrote:
> > > Why not just re-post the Stop() to set this on the decode thread, so you
> know
> > > that it'll be seen as soon as that gets processed?
> > 
> > Posting a task on the decoder thread will not do what we want here. We want
to
> > stop decoding of the frames that are already in the task queue for the
decode
> > thread. If I post a task to the decode thread it will only be executed after
> all
> > pending decoder tasks.
> 
> OK, but you've stated that we don't need to lock here because the it's not an
> issue if the thread sees false instead of true; if the optimization of
stopping
> processing on the other thread is valuable then it's worth putting a lock
around
> this to create a memory barrier and ensure that the other thread sees the
> change.

Actually after thinking more about it I think we don't need this method at all.
Normally MessageReader class doesn't read from the socket if there are any
pending messages. The only case when it may allow several messages to be
processed concurrently is when they were read in a single Read() call - in that
case they are likely to be empty packets, so we don't win much by canceling them
anyway.
Removed this method.

[Wez, 2012-08-30 00:32:55]

lgtm

[commit-bot: I haz the power, 2012-08-30 17:52:35]

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

[commit-bot: I haz the power, 2012-08-30 20:56:22]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 154200, job name
was 10879085-12002 (previous was lost) (previous was lost) (previous was lost)
(retry).