Added the desktop shape fields to VideoPacket.

Added the desktop shape fields to VideoPacket.

If the host passes the desktop shape region in a VideoPacket the client will use it to draw transparent regions. The client also informs the web-app of the desktop shape changes, so it can set the input passthrough region correctly.

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

[alexeypa (please no reviews), 2013-06-20 20:46:01]

Jamie or Wez, PTAL.

[Jamie, 2013-06-20 20:55:47]

On 2013/06/20 20:46:01, alexeypa wrote:
> Jamie or Wez, PTAL.

I'll take a look.

[Jamie, 2013-06-20 21:15:57]

lgtm with nits.

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
File remoting/codec/video_decoder.h (right):

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
remoting/codec/video_decoder.h:74: virtual const SkRegion* GetImageShape() {
return NULL; }
Everything else here is pure virtual. Does this need a default implementation?
If it's convenient to leave it like this for now, can you add a TODO to make it
pure virtual once all subclasses inplemented it?

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
File remoting/codec/video_decoder_vp8.cc (right):

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
remoting/codec/video_decoder_vp8.cc:287:
transparent_region_.op(desktop_shape_region_, SkRegion::kUnion_Op);
It took me a while to understand what was going on here. I think it might be
clearer expressed as (pseudocode):

  newly_transparent = old_desktop_shape - new_desktop_shape;
  transparent = transparent + newly_transparent;

WDYT? It's closely analogous to the operations below for computing the new
repaint region.

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
remoting/codec/video_decoder_vp8.cc:292: SkRegion region =
*new_desktop_shape_region;
Can you use a more descriptive name for this? Maybe repaint_region?

[Sergey Ulanov, 2013-06-20 21:27:21]

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
File remoting/codec/video_decoder.h (right):

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
remoting/codec/video_decoder.h:74: virtual const SkRegion* GetImageShape() {
return NULL; }
Can you use webrtc::DesktopRegion here instead of SkRegion? mainly for
consistency between client and host code. I'm planning to switch all client code
to using webrtc::DesktopRegion.

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/proto/video.p...
File remoting/proto/video.proto (right):

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/proto/video.p...
remoting/proto/video.proto:103: repeated Rect desktop_shape_rects = 10;
Potentially we could store regions a bit more efficiently if we add support for
serialization/deserialization in the region class.

[alexeypa (please no reviews), 2013-06-20 22:05:11]

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
File remoting/codec/video_decoder.h (right):

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
remoting/codec/video_decoder.h:74: virtual const SkRegion* GetImageShape() {
return NULL; }
On 2013/06/20 21:15:57, Jamie wrote:
> Everything else here is pure virtual. Does this need a default implementation?
> If it's convenient to leave it like this for now, can you add a TODO to make
it
> pure virtual once all subclasses inplemented it?

Done.

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
remoting/codec/video_decoder.h:74: virtual const SkRegion* GetImageShape() {
return NULL; }
On 2013/06/20 21:27:21, Sergey Ulanov wrote:
> Can you use webrtc::DesktopRegion here instead of SkRegion? mainly for
> consistency between client and host code. I'm planning to switch all client
code
> to using webrtc::DesktopRegion.

I will not be able to do it today. We would really like to submit it before the
branch point.

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
File remoting/codec/video_decoder_vp8.cc (right):

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
remoting/codec/video_decoder_vp8.cc:287:
transparent_region_.op(desktop_shape_region_, SkRegion::kUnion_Op);
On 2013/06/20 21:15:57, Jamie wrote:
> It took me a while to understand what was going on here. I think it might be
> clearer expressed as (pseudocode):
> 
>   newly_transparent = old_desktop_shape - new_desktop_shape;
>   transparent = transparent + newly_transparent;
> 
> WDYT? It's closely analogous to the operations below for computing the new
> repaint region.

Done.

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/codec/video_d...
remoting/codec/video_decoder_vp8.cc:292: SkRegion region =
*new_desktop_shape_region;
On 2013/06/20 21:15:57, Jamie wrote:
> Can you use a more descriptive name for this? Maybe repaint_region?

Done.

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/proto/video.p...
File remoting/proto/video.proto (right):

https://chromiumcodereview.appspot.com/17511004/diff/1/remoting/proto/video.p...
remoting/proto/video.proto:103: repeated Rect desktop_shape_rects = 10;
On 2013/06/20 21:27:21, Sergey Ulanov wrote:
> Potentially we could store regions a bit more efficiently if we add support
for
> serialization/deserialization in the region class.

It does not sound like something that we could do in this CL.

[commit-bot: I haz the power, 2013-06-20 22:53:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/17511004/11001

[commit-bot: I haz the power, 2013-06-21 10:16:42]

Change committed as 207789