Refactoring of the client-side input pipeline and scaling dimension management.

Refactoring of the client-side input pipeline and scaling dimension management.

The main changes are:
* Express key-release, mouse coordinate scaling and clamping as InputStubs.
* KeyEventTracker handles key release.
* MouseInputFilter handles mouse scaling & clamping.
* PepperInputHandler converts Pepper events to InputStub events.
* Replace scaling ratios with host and view dimensions.

This resulted in some related changes
* The DecoderVp8 enforces a <=1:1 output-size before converting the frame.
* The setScaleToFit() API now has no effect.
* ChromotingView has become a pure interface again.

Things this CL is currently missing:
* Unit-tests for the new components.

Future work:
* Move the non-Pepper-specific input pipeline components to ChromotingClient.
* Rework the decode / convert / scale / render pipeline.

BUG=93552
TEST=remoting_unittests, and manual testing.


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

[Wez, 2011-12-17 00:42:02]

sergeyu: Please review. :)

jamiewalch: FYI WRT the input pipeline layout.

[Sergey Ulanov, 2011-12-20 00:05:34]

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

http://codereview.chromium.org/8985007/diff/1/remoting/base/decoder_vp8.cc#ne...
remoting/base/decoder_vp8.cc:24: output_size_.setEmpty();
Can we initialize output_size_ in the same way clip_rect_ is initialized?

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

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.cc#newcode153
remoting/base/util.cc:153: int x1 = (rect.fLeft * out_size.width()) /
in_size.width();
here and below, left() instead of fLeft?

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.cc#newcode155
remoting/base/util.cc:155: int x2 = (rect.fRight * out_size.width() +
out_size.width()-1) /
nit: spaces around minus here and in the next line

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.cc#newcode157
remoting/base/util.cc:157: int y2 = (rect.fBottom * out_size.height() +
out_size.height()-1) /
maybe call it right and bottom instead of x2 and y2 - for consistency with other
functions

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.cc#newcode157
remoting/base/util.cc:157: int y2 = (rect.fBottom * out_size.height() +
out_size.height()-1) /
I think it would be useful to add a comment saying that we want scaled rect to
cover the non-scaled rect, to explain why we need  "+out_size.height()-1" here.

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.cc#newcode159
remoting/base/util.cc:159: return SkIRect::MakeXYWH(x1, y1, x2-x1, y2-y1);
Use MakeLTRB() here?

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.h
File remoting/base/util.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.h#newcode55
remoting/base/util.h:55: // Return a scaled rectangle using the horizontal and
vertical scale
please update this comment

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
File remoting/client/mouse_input_filter.cc (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.cc:13: {
Here and in all other methods: { should not be wrapped.

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.cc:34: out_event.set_x((out_event.x() *
output_size_.width()) /
I think this code would be more readable if you defined int x = ... instead of
calling set_x() and then x(). It would also probably perform better.

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.cc:36: out_event.set_x(std::max(0,
std::min(output_size_.width()-1,
spaces around -

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
File remoting/client/mouse_input_filter.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.h:15: class MouseInputFilter : public
protocol::InputStub {
Add comments that explain what this class does

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.h:25: void SetInputDimensions(const SkISize&
size);
Maybe set_input_size()?

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.h:29: void SetOutputDimensions(const SkISize&
size);
set_output_size()?

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.h:31: protected:
private?

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/chromoti...
File remoting/client/plugin/chromoting_scriptable_object.cc (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/chromoti...
remoting/client/plugin/chromoting_scriptable_object.cc:424: VLOG(1) << "Ignoring
setScaleToFit";
Do we need to keep this method just for backward compatibility? If so, please
add comments about it.

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_i...
File remoting/client/plugin/pepper_input_handler.cc (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_i...
remoting/client/plugin/pepper_input_handler.cc:105: #if 0
Remove this code?

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_i...
File remoting/client/plugin/pepper_input_handler.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_i...
remoting/client/plugin/pepper_input_handler.h:12: }
// namespace pp

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_i...
remoting/client/plugin/pepper_input_handler.h:18: }
// namepsace protocol

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_v...
File remoting/client/plugin/pepper_view.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_v...
remoting/client/plugin/pepper_view.h:61: SkISize GetViewDimensions() const;
can this be inline view_dimentions()? Also, I think it's better to return const
reference here.

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_v...
File remoting/client/plugin/pepper_view_proxy.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_v...
remoting/client/plugin/pepper_view_proxy.h:30: class PepperViewProxy : public
base::RefCountedThreadSafe<PepperViewProxy>,
FYI: We don't need this class anymore - network and UI work on the main plugin
thread anyway.

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
File remoting/protocol/key_event_tracker.cc (right):

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.cc:11: 
remove this line

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.cc:16: {
must be on the previous line.

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
File remoting/protocol/key_event_tracker.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.h:15: 
remove this empty line

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.h:18: class KeyEventTracker : public
InputStub {
Add comments

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.h:28: void ReleaseAllKeys();
IMHO, it's better to put this  before InputStub definition, but it's not very
important.

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.h:30: protected:
private?

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.h:39: 
remove empty line

[Jamie, 2011-12-20 00:51:56]

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

http://codereview.chromium.org/8985007/diff/1/remoting/base/decoder_vp8.cc#ne...
remoting/base/decoder_vp8.cc:154: !output_size_.equals(last_image_->d_w,
last_image_->d_h);
This will change the behaviour if DoScaling is ever called after
SetOutputDimentions but before last_image is set. Are you sure that doesn't
matter?

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
File remoting/client/mouse_input_filter.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.h:15: class MouseInputFilter : public
protocol::InputStub {
On 2011/12/20 00:05:35, sergeyu wrote:
> Add comments that explain what this class does

...And "Filters mouse input" is not sufficient :)

[Wez, 2011-12-20 07:14:14]

Thanks for catching all those issues; a fair bit has changed so PTAL.

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

http://codereview.chromium.org/8985007/diff/1/remoting/base/decoder_vp8.cc#ne...
remoting/base/decoder_vp8.cc:24: output_size_.setEmpty();
On 2011/12/20 00:05:35, sergeyu wrote:
> Can we initialize output_size_ in the same way clip_rect_ is initialized?

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/base/decoder_vp8.cc#ne...
remoting/base/decoder_vp8.cc:154: !output_size_.equals(last_image_->d_w,
last_image_->d_h);
On 2011/12/20 00:51:57, Jamie wrote:
> This will change the behaviour if DoScaling is ever called after
> SetOutputDimentions but before last_image is set. Are you sure that doesn't
> matter?

DoScaling() determines whether we use the ConvertRects() or
ScaleAndConvertRects() method, so it shouldn't actually ever be called when
|last_image_| is NULL.  I've changed the code to DCHECK |last_image_|.

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

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.cc#newcode153
remoting/base/util.cc:153: int x1 = (rect.fLeft * out_size.width()) /
in_size.width();
On 2011/12/20 00:05:35, sergeyu wrote:
> here and below, left() instead of fLeft? 

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.cc#newcode155
remoting/base/util.cc:155: int x2 = (rect.fRight * out_size.width() +
out_size.width()-1) /
On 2011/12/20 00:05:35, sergeyu wrote:
> nit: spaces around minus here and in the next line

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.cc#newcode157
remoting/base/util.cc:157: int y2 = (rect.fBottom * out_size.height() +
out_size.height()-1) /
On 2011/12/20 00:05:35, sergeyu wrote:
> maybe call it right and bottom instead of x2 and y2 - for consistency with
other
> functions

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.cc#newcode157
remoting/base/util.cc:157: int y2 = (rect.fBottom * out_size.height() +
out_size.height()-1) /
On 2011/12/20 00:05:35, sergeyu wrote:
> I think it would be useful to add a comment saying that we want scaled rect to
> cover the non-scaled rect, to explain why we need  "+out_size.height()-1"
here.

I've updated the comment on ScaleRect() in the header to explain that subtlety.

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.cc#newcode159
remoting/base/util.cc:159: return SkIRect::MakeXYWH(x1, y1, x2-x1, y2-y1);
On 2011/12/20 00:05:35, sergeyu wrote:
> Use MakeLTRB() here?

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.h
File remoting/base/util.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/base/util.h#newcode55
remoting/base/util.h:55: // Return a scaled rectangle using the horizontal and
vertical scale
On 2011/12/20 00:05:35, sergeyu wrote:
> please update this comment

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
File remoting/client/mouse_input_filter.cc (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.cc:13: {
On 2011/12/20 00:05:35, sergeyu wrote:
> Here and in all other methods: { should not be wrapped.

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.cc:13: {
On 2011/12/20 00:05:35, sergeyu wrote:
> Here and in all other methods: { should not be wrapped.

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.cc:34: out_event.set_x((out_event.x() *
output_size_.width()) /
On 2011/12/20 00:05:35, sergeyu wrote:
> I think this code would be more readable if you defined int x = ... instead of
> calling set_x() and then x(). It would also probably perform better.

Performance shouldn't be affected, since x() and set_x() are inline, but I agree
re readability.

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.cc:36: out_event.set_x(std::max(0,
std::min(output_size_.width()-1,
On 2011/12/20 00:05:35, sergeyu wrote:
> spaces around -

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
File remoting/client/mouse_input_filter.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.h:15: class MouseInputFilter : public
protocol::InputStub {
On 2011/12/20 00:05:35, sergeyu wrote:
> Add comments that explain what this class does

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.h:25: void SetInputDimensions(const SkISize&
size);
On 2011/12/20 00:05:35, sergeyu wrote:
> Maybe set_input_size()?

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.h:29: void SetOutputDimensions(const SkISize&
size);
On 2011/12/20 00:05:35, sergeyu wrote:
> set_output_size()?

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/client/mouse_input_fil...
remoting/client/mouse_input_filter.h:31: protected:
On 2011/12/20 00:05:35, sergeyu wrote:
> private?

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/chromoti...
File remoting/client/plugin/chromoting_scriptable_object.cc (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/chromoti...
remoting/client/plugin/chromoting_scriptable_object.cc:424: VLOG(1) << "Ignoring
setScaleToFit";
On 2011/12/20 00:05:35, sergeyu wrote:
> Do we need to keep this method just for backward compatibility? If so, please
> add comments about it.

I've replaced this method with DoNothing().  I *think* we need to have an
implementation for setScaleToFit(), otherwise JavaScript will throw an exception
because the method is missing.

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_i...
File remoting/client/plugin/pepper_input_handler.cc (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_i...
remoting/client/plugin/pepper_input_handler.cc:105: #if 0
On 2011/12/20 00:05:35, sergeyu wrote:
> Remove this code?

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_i...
File remoting/client/plugin/pepper_input_handler.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_i...
remoting/client/plugin/pepper_input_handler.h:12: }
On 2011/12/20 00:05:35, sergeyu wrote:
> // namespace pp

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_i...
remoting/client/plugin/pepper_input_handler.h:18: }
On 2011/12/20 00:05:35, sergeyu wrote:
> // namepsace protocol

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_v...
File remoting/client/plugin/pepper_view.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_v...
remoting/client/plugin/pepper_view.h:61: SkISize GetViewDimensions() const;
On 2011/12/20 00:05:35, sergeyu wrote:
> can this be inline view_dimentions()? Also, I think it's better to return
const
> reference here.

Done.

I've also renamed member |plugin_size_| to |view_size_| and SetPluginSize() to
SetViewSize().  I stopped short of going to set_view_size() and set_host_size()
on the basis that they're more than just setters; what do you think?

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_v...
File remoting/client/plugin/pepper_view_proxy.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/client/plugin/pepper_v...
remoting/client/plugin/pepper_view_proxy.h:30: class PepperViewProxy : public
base::RefCountedThreadSafe<PepperViewProxy>,
On 2011/12/20 00:05:35, sergeyu wrote:
> FYI: We don't need this class anymore - network and UI work on the main plugin
> thread anyway.

OK, gone!

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
File remoting/protocol/key_event_tracker.cc (right):

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.cc:11: 
On 2011/12/20 00:05:35, sergeyu wrote:
> remove this line

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.cc:16: {
On 2011/12/20 00:05:35, sergeyu wrote:
> must be on the previous line.

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
File remoting/protocol/key_event_tracker.h (right):

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.h:15: 
On 2011/12/20 00:05:35, sergeyu wrote:
> remove this empty line

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.h:18: class KeyEventTracker : public
InputStub {
On 2011/12/20 00:05:35, sergeyu wrote:
> Add comments

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.h:28: void ReleaseAllKeys();
On 2011/12/20 00:05:35, sergeyu wrote:
> IMHO, it's better to put this  before InputStub definition, but it's not very
> important.

Done.

http://codereview.chromium.org/8985007/diff/1/remoting/protocol/key_event_tra...
remoting/protocol/key_event_tracker.h:30: protected:
On 2011/12/20 00:05:35, sergeyu wrote:
> private?

Done.

[Wez, 2011-12-21 01:18:10]

sergeyu: Ping :)

[Sergey Ulanov, 2011-12-21 02:38:19]

Looks like there is a merge issue. Other than that LGTM

http://codereview.chromium.org/8985007/diff/12001/remoting/base/decoder_vp8.cc
File remoting/base/decoder_vp8.cc (right):

http://codereview.chromium.org/8985007/diff/12001/remoting/base/decoder_vp8.c...
remoting/base/decoder_vp8.cc:210: if (!clip_rect.intersect(output_size_))
Does this compile given that |output_size_| is a not a rect? Should be written
as intersect(SkIRect::MakeWH(output_size_))?

[Wez, 2011-12-21 17:58:58]

On 2011/12/21 02:38:19, sergeyu wrote:
> Looks like there is a merge issue. Other than that LGTM
> 
> http://codereview.chromium.org/8985007/diff/12001/remoting/base/decoder_vp8.cc
> File remoting/base/decoder_vp8.cc (right):
> 
>
http://codereview.chromium.org/8985007/diff/12001/remoting/base/decoder_vp8.c...
> remoting/base/decoder_vp8.cc:210: if (!clip_rect.intersect(output_size_))
> Does this compile given that |output_size_| is a not a rect? Should be written
> as intersect(SkIRect::MakeWH(output_size_))?

Yes, there were merge issues, and there's some basic-types issue on Windows,
too.  :(

[commit-bot: I haz the power, 2011-12-22 02:15:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/8985007/16071

[commit-bot: I haz the power, 2011-12-22 10:20:35]

Change committed as 115511