Allow the Chromoting client plugin to support up-scaling, albeit slowly.

Allow the Chromoting client plugin to support up-scaling, albeit slowly.

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

[Wez, 2012-06-14 01:06:05]

PTAL

[Wez, 2012-06-14 01:06:40]

On 2012/06/14 01:06:05, Wez wrote:
> PTAL

If we're happy with this as-is then I'll land it and try to clean the
implementation up later.

[Jamie, 2012-06-14 01:14:29]

I don't think I'm very well qualified to review this. It looks plausible, apart
from one comment, but I'd be more comfortable with someone more familiar with
this code doing the review.

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

http://codereview.chromium.org/9568048/diff/1003/remoting/base/decoder_vp8.cc...
remoting/base/decoder_vp8.cc:155: SkIRect source_rect = ScaleRect(clip_area,
view_size, screen_size_);
It's not obvious to me that ScaleRect will return even coordinates as required
by the DCHECKs.

[alexeypa (please no reviews), 2012-06-14 17:00:16]

http://codereview.chromium.org/9568048/diff/1003/remoting/base/decoder_vp8.cc
File remoting/base/decoder_vp8.cc (left):

http://codereview.chromium.org/9568048/diff/1003/remoting/base/decoder_vp8.cc...
remoting/base/decoder_vp8.cc:140: if (source_clip.width() < view_size.width() ||
It is the only place where this check is done? I remember there were a couple of
places.

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

http://codereview.chromium.org/9568048/diff/1003/remoting/base/decoder_vp8.cc...
remoting/base/decoder_vp8.cc:122: static int CalculateYOffset(int x, int y, int
stride) {
CalculateYOffset and CalculateUVOffset are defined in remoting/base/util.cc

[Sergey Ulanov, 2012-06-14 18:04:03]

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

http://codereview.chromium.org/9568048/diff/1003/remoting/base/decoder_vp8.cc...
remoting/base/decoder_vp8.cc:123: DCHECK(((x & 1) == 0) && ((y & 1) == 0));
nit: would be more readable as x % 2 == 0 && y % 2 == 0

[Wez, 2012-06-18 21:51:34]

http://codereview.chromium.org/9568048/diff/1003/remoting/base/decoder_vp8.cc
File remoting/base/decoder_vp8.cc (left):

http://codereview.chromium.org/9568048/diff/1003/remoting/base/decoder_vp8.cc...
remoting/base/decoder_vp8.cc:140: if (source_clip.width() < view_size.width() ||
On 2012/06/14 17:00:16, alexeypa wrote:
> It is the only place where this check is done? I remember there were a couple
of
> places.

This should be the only place.

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

http://codereview.chromium.org/9568048/diff/1003/remoting/base/decoder_vp8.cc...
remoting/base/decoder_vp8.cc:122: static int CalculateYOffset(int x, int y, int
stride) {
On 2012/06/14 17:00:16, alexeypa wrote:
> CalculateYOffset and CalculateUVOffset are defined in remoting/base/util.cc

I've made them visible and used them.

(The intent is to fold this logic into an API in base and simplify DecoderVP8
again)

http://codereview.chromium.org/9568048/diff/1003/remoting/base/decoder_vp8.cc...
remoting/base/decoder_vp8.cc:123: DCHECK(((x & 1) == 0) && ((y & 1) == 0));
On 2012/06/14 18:04:03, sergeyu wrote:
> nit: would be more readable as x % 2 == 0 && y % 2 == 0

This is how they're implemented in remoting/base/util.cc. Using % 2 assumes that
the compiler will optimize the check to & 1, otherwise performance will be a
problem.

http://codereview.chromium.org/9568048/diff/1003/remoting/base/decoder_vp8.cc...
remoting/base/decoder_vp8.cc:155: SkIRect source_rect = ScaleRect(clip_area,
view_size, screen_size_);
On 2012/06/14 01:14:29, Jamie wrote:
> It's not obvious to me that ScaleRect will return even coordinates as required
> by the DCHECKs.

It won't - fixed!

[alexeypa (please no reviews), 2012-06-18 21:55:23]

LGTM

[commit-bot: I haz the power, 2012-06-18 22:03:55]

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

[Sergey Ulanov, 2012-06-18 22:29:50]

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

http://codereview.chromium.org/9568048/diff/1003/remoting/base/decoder_vp8.cc...
remoting/base/decoder_vp8.cc:123: DCHECK(((x & 1) == 0) && ((y & 1) == 0));
On 2012/06/18 21:51:34, Wez wrote:
> On 2012/06/14 18:04:03, sergeyu wrote:
> > nit: would be more readable as x % 2 == 0 && y % 2 == 0
> 
> This is how they're implemented in remoting/base/util.cc. Using % 2 assumes
that
> the compiler will optimize the check to & 1, otherwise performance will be a
> problem.

All compilers we compile this code with should be smart enough to optimize it
for this case. If you don't assume that the compiler is smart then /2 should be
replaced with >>1 in line 129 as will as in many other places.

[commit-bot: I haz the power, 2012-06-18 23:17:54]

Change committed as 142853