Replace RectVectors with SkRegions in Decoder.

Replace RectVectors with SkRegions in Decoder.

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

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

[Wez, 2012-01-23 19:35:39]

alexeypa: FYI
sergeyu: PTAL

[Sergey Ulanov, 2012-01-23 19:59:53]

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

http://codereview.chromium.org/9277001/diff/1/remoting/base/decoder.h#newcode43
remoting/base/decoder.h:43: // |region|. TODO: |rects| is kept empty if whole
screen needs to be updated.
TODO(wez)?

It's better to move TODO to a separate line. It's not clear from TODO what needs
to be fixed.

s/rects/region/

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

http://codereview.chromium.org/9277001/diff/1/remoting/base/decoder_vp8.cc#ne...
remoting/base/decoder_vp8.cc:149: ConvertRegion(region, &updated_region_);
nit: add { }

http://codereview.chromium.org/9277001/diff/1/remoting/base/decoder_vp8.cc#ne...
remoting/base/decoder_vp8.cc:196: output_region->op(dest_rect,
SkRegion::kUnion_Op);
Comments about SkRegion::setRects() say that it may be faster than union of all
rects. I don't know if it is really the case, but if we don't use setRects()
here, then there is no reason to use it in DecoderVp8::DecodePacket()

http://codereview.chromium.org/9277001/diff/1/remoting/base/decoder_vp8.cc#ne...
remoting/base/decoder_vp8.cc:241: output_region->op(output_rect,
SkRegion::kUnion_Op);
same here

http://codereview.chromium.org/9277001/diff/1/remoting/client/frame_consumer.h
File remoting/client/frame_consumer.h (right):

http://codereview.chromium.org/9277001/diff/1/remoting/client/frame_consumer....
remoting/client/frame_consumer.h:44: // region included in the |rects| list.
s/rects/region/

http://codereview.chromium.org/9277001/diff/1/remoting/client/frame_consumer....
remoting/client/frame_consumer.h:49: SkRegion* region,
should this be a const ref?

[alexeypa (please no reviews), 2012-01-23 20:32:10]

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

http://codereview.chromium.org/9277001/diff/1/remoting/base/decoder.h#newcode43
remoting/base/decoder.h:43: // |region|. TODO: |rects| is kept empty if whole
screen needs to be updated.
TODO: |rect| -> TODO: |region|

[Wez, 2012-01-23 21:37:23]

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

http://codereview.chromium.org/9277001/diff/1/remoting/base/decoder.h#newcode43
remoting/base/decoder.h:43: // |region|. TODO: |rects| is kept empty if whole
screen needs to be updated.
On 2012/01/23 19:59:54, sergeyu wrote:
> TODO(wez)?
> 
> It's better to move TODO to a separate line. It's not clear from TODO what
needs
> to be fixed.
> 
> s/rects/region/

As far as I can tell, that comment just isn't true - the TODO was just a
reminder to remove it.

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

http://codereview.chromium.org/9277001/diff/1/remoting/base/decoder_vp8.cc#ne...
remoting/base/decoder_vp8.cc:149: ConvertRegion(region, &updated_region_);
On 2012/01/23 19:59:54, sergeyu wrote:
> nit: add { }

Done.

http://codereview.chromium.org/9277001/diff/1/remoting/base/decoder_vp8.cc#ne...
remoting/base/decoder_vp8.cc:196: output_region->op(dest_rect,
SkRegion::kUnion_Op);
On 2012/01/23 19:59:54, sergeyu wrote:
> Comments about SkRegion::setRects() say that it may be faster than union of
all
> rects. I don't know if it is really the case, but if we don't use setRects()
> here, then there is no reason to use it in DecoderVp8::DecodePacket()

To use setRects() here, we'd need to create a vector(SkIRect), push the rects
onto it, and then setRects() into the region.  Since SkRegion doesn't provide a
qay to query the rectangle count, we can't reserve() capacity in the vector. 
And although setRects() _could_ optimize things, it actually doesn't.  So I've
reverted to using op(kUnion_Op) elsewhere.

http://codereview.chromium.org/9277001/diff/1/remoting/client/frame_consumer.h
File remoting/client/frame_consumer.h (right):

http://codereview.chromium.org/9277001/diff/1/remoting/client/frame_consumer....
remoting/client/frame_consumer.h:44: // region included in the |rects| list.
On 2012/01/23 19:59:54, sergeyu wrote:
> s/rects/region/

Done.

http://codereview.chromium.org/9277001/diff/1/remoting/client/frame_consumer....
remoting/client/frame_consumer.h:49: SkRegion* region,
On 2012/01/23 19:59:54, sergeyu wrote:
> should this be a const ref?

Probably.  However, I think the FrameConsumer API is due some other changes, so
I'll put those in a follow-up CL.

[alexeypa (please no reviews), 2012-01-23 21:50:52]

lgtm

[Sergey Ulanov, 2012-01-23 22:19:57]

LGTM

[commit-bot: I haz the power, 2012-01-23 22:26:09]

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

[commit-bot: I haz the power, 2012-01-24 00:27:24]

Try job failure for 9277001-5001 (retry) on win_rel for step "base_unittests".
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-01-24 00:55:05]

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

[commit-bot: I haz the power, 2012-01-24 02:53:53]

Change committed as 118790

[commit-bot: I haz the power, 2012-01-27 17:51:09]

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

[commit-bot: I haz the power, 2012-01-27 18:46:41]

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

[commit-bot: I haz the power, 2012-01-27 20:57:19]

Try job failure for 9277001-5001 (retry) on win_rel for step "unit_tests"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-01-28 20:50:25]

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

[commit-bot: I haz the power, 2012-01-29 03:45:18]

Change committed as 119632