Allow shaped-desktop hosts to send shape only when it changes.

Allow shaped-desktop hosts to send shape only when it changes.

Previously hosts supplying a shaped desktop needed to attach the desktop
shape to every frame, wasting bandwidth since the shape changes
relatively infrequently.

This CL updates the VideoRenderer implementations to preserve the shape
(or lack of one) from the preceding frame if the VideoPacket does not
include the use_desktop_shape field.

Also simplifies FrameConsumerProxy to remove the need for ref-counting,
updates NULL->nullptr throughout remoting/codec/, and removes
unnecessary transparency logic from VideoDecoderVpx.

BUG=446288
Committed: https://crrev.com/070889be6cd0ff3425d6260fbaac59ed24f34627
Cr-Commit-Position: refs/heads/master@{#339212}

[Wez, 2015-07-10 21:05:53]

wez@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2015-07-10 21:51:44]

lgtm

https://codereview.chromium.org/1236663002/diff/1/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1236663002/diff/1/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_video_renderer_3d.cc:237: // Process the frame
shape, if supplied
nit: add . at the end

https://codereview.chromium.org/1236663002/diff/1/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_video_renderer_3d.cc:254: frame_shape_.reset();
nit: You are using frame_shape_ = make_scoped_ptr(..) above. For consistency I
think it's better to write frame_shape_ = nullptr here or use reset() instead of
assignment above

https://codereview.chromium.org/1236663002/diff/1/remoting/client/software_vi...
File remoting/client/software_video_renderer.cc (right):

https://codereview.chromium.org/1236663002/diff/1/remoting/client/software_vi...
remoting/client/software_video_renderer.cc:246: decoder_->GetImageShape()
Maybe fix FrameConsumer interface to take pointer for frame shape?

[Wez, 2015-07-13 19:56:24]

PTAL - lots of changes due to the shape & FrameConsumerProxy refactoring.

I expect there will be follow-up changes to the iOS and Android client builds -
let me know if you'd like me to direct those reviews to you or to the relevant
owners?

https://codereview.chromium.org/1236663002/diff/1/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1236663002/diff/1/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_video_renderer_3d.cc:237: // Process the frame
shape, if supplied
On 2015/07/10 21:51:43, Sergey Ulanov wrote:
> nit: add . at the end

Done.

https://codereview.chromium.org/1236663002/diff/1/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_video_renderer_3d.cc:254: frame_shape_.reset();
On 2015/07/10 21:51:43, Sergey Ulanov wrote:
> nit: You are using frame_shape_ = make_scoped_ptr(..) above. For consistency I
> think it's better to write frame_shape_ = nullptr here or use reset() instead
of
> assignment above

Done.

https://codereview.chromium.org/1236663002/diff/1/remoting/client/software_vi...
File remoting/client/software_video_renderer.cc (right):

https://codereview.chromium.org/1236663002/diff/1/remoting/client/software_vi...
remoting/client/software_video_renderer.cc:246: decoder_->GetImageShape()
On 2015/07/10 21:51:43, Sergey Ulanov wrote:
> Maybe fix FrameConsumer interface to take pointer for frame shape?

Done.

As part of doing that I've also simplified FrameConsumerProxy and allowed the
PepperVideoRenderer EventHandler API to handle NULL shapes as well, to reduce
complexity.

[Sergey Ulanov, 2015-07-13 21:00:32]

> I expect there will be follow-up changes to the iOS and Android client builds
-
> let me know if you'd like me to direct those reviews to you or to the relevant
> owners?

Actually you need to update android code in this CL - see the failures on the
Android.

https://codereview.chromium.org/1236663002/diff/60001/remoting/client/frame_c...
File remoting/client/frame_consumer_proxy.cc (right):

https://codereview.chromium.org/1236663002/diff/60001/remoting/client/frame_c...
remoting/client/frame_consumer_proxy.cc:42: scoped_ptr<webrtc::DesktopRegion>
shape_ptr;
maybe call it shape_copy?.

https://codereview.chromium.org/1236663002/diff/60001/remoting/client/frame_c...
remoting/client/frame_consumer_proxy.cc:44: shape_ptr = make_scoped_ptr(new
webrtc::DesktopRegion(*shape));
might be better to initialize it in the constructor:
  scoped_ptr<webrtc::DesktopRegion> shape_ptr(shape ? new
webrtc::DesktopRegion(*shape) : nullptr);

https://codereview.chromium.org/1236663002/diff/60001/remoting/client/plugin/...
File remoting/client/plugin/pepper_video_renderer_2d.cc (right):

https://codereview.chromium.org/1236663002/diff/60001/remoting/client/plugin/...
remoting/client/plugin/pepper_video_renderer_2d.cc:103:
context.main_task_runner(), context.decode_task_runner(), this));
I think we still need FrameConsumerProxy. SoftwareVideoRenderer calls the
consumer on the decode thread, while this class doesn't expect it.

[Wez, 2015-07-14 02:26:44]

https://codereview.chromium.org/1236663002/diff/60001/remoting/client/plugin/...
File remoting/client/plugin/pepper_video_renderer_2d.cc (right):

https://codereview.chromium.org/1236663002/diff/60001/remoting/client/plugin/...
remoting/client/plugin/pepper_video_renderer_2d.cc:103:
context.main_task_runner(), context.decode_task_runner(), this));
On 2015/07/13 21:00:32, Sergey Ulanov wrote:
> I think we still need FrameConsumerProxy. SoftwareVideoRenderer calls the
> consumer on the decode thread, while this class doesn't expect it.

SoftwareVideoRenderer delegates decode-thread work to an internal Core object -
the outer object is non-thread-safe, so I moved the FrameConsumerProxy inside
it, to clean up the interface.

[Sergey Ulanov, 2015-07-14 16:12:24]

lgtm

https://codereview.chromium.org/1236663002/diff/60001/remoting/client/plugin/...
File remoting/client/plugin/pepper_video_renderer_2d.cc (right):

https://codereview.chromium.org/1236663002/diff/60001/remoting/client/plugin/...
remoting/client/plugin/pepper_video_renderer_2d.cc:103:
context.main_task_runner(), context.decode_task_runner(), this));
On 2015/07/14 02:26:44, Wez wrote:
> On 2015/07/13 21:00:32, Sergey Ulanov wrote:
> > I think we still need FrameConsumerProxy. SoftwareVideoRenderer calls the
> > consumer on the decode thread, while this class doesn't expect it.
> 
> SoftwareVideoRenderer delegates decode-thread work to an internal Core object
-
> the outer object is non-thread-safe, so I moved the FrameConsumerProxy inside
> it, to clean up the interface.

I see, sorry I messed. This makes sense.

[Sergey Ulanov, 2015-07-14 16:13:22]

https://codereview.chromium.org/1236663002/diff/60001/remoting/client/plugin/...
File remoting/client/plugin/pepper_video_renderer_2d.cc (right):

https://codereview.chromium.org/1236663002/diff/60001/remoting/client/plugin/...
remoting/client/plugin/pepper_video_renderer_2d.cc:103:
context.main_task_runner(), context.decode_task_runner(), this));
On 2015/07/14 16:12:24, Sergey Ulanov wrote:
> On 2015/07/14 02:26:44, Wez wrote:
> > On 2015/07/13 21:00:32, Sergey Ulanov wrote:
> > > I think we still need FrameConsumerProxy. SoftwareVideoRenderer calls the
> > > consumer on the decode thread, while this class doesn't expect it.
> > 
> > SoftwareVideoRenderer delegates decode-thread work to an internal Core
object
> -
> > the outer object is non-thread-safe, so I moved the FrameConsumerProxy
inside
> > it, to clean up the interface.
> 
> I see, sorry I messed. This makes sense.

I meant I mIssed it.

[Wez, 2015-07-14 17:11:28]

The CQ bit was checked by wez@chromium.org

[Wez, 2015-07-14 17:11:31]

The CQ bit was unchecked by wez@chromium.org

[Wez, 2015-07-16 21:45:10]

PTAL - had to revert the changes that moved FrameConsumerProxy to be owned by
SoftwareVideoRenderer, because the JNI implementations (ChromotingJniInstance)
has pretty broken threading that will need more significant refactoring to fix
properly. :(

[Sergey Ulanov, 2015-07-17 00:09:22]

lgtm

[Wez, 2015-07-17 01:30:44]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2015-07-17 01:32:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236663002/100001

[commit-bot: I haz the power, 2015-07-17 03:19:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-17 03:20:13]

Patchset 6 (id:??) landed as
https://crrev.com/070889be6cd0ff3425d6260fbaac59ed24f34627
Cr-Commit-Position: refs/heads/master@{#339212}