Simplify FrameConsumer interface. Remove FrameProducer interface.

remoting: Simplify FrameConsumer interface and remove FrameProducer interface.

Now FrameConsumer implementation is responsible for scaling. This makes
interaction with FrameConsumer much simpler. Also removed FrameProducer
interface as it's no longer needed.

Currently video scaling is only used in the plugin when Graphics3D is
not available. In that case Graphics2D::SetScale() performs better than
trying to scale the image in the plugin, especially given that the
scaling code is not optimized for PNaCl.

This refactoring also allowed to simplify threading logic in the
rendering both on Android and in the plugin.

BUG=256850, 486917, 509914
Committed: https://crrev.com/13bca69cfa261ec8eb6bffecf59c486b683eb8db
Cr-Commit-Position: refs/heads/master@{#344404}

[Sergey Ulanov, 2015-08-17 21:12:26]

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

[Lambros, 2015-08-17 22:17:44]

lambroslambrou@chromium.org changed reviewers:
+ lambroslambrou@chromium.org

[Lambros, 2015-08-17 22:17:45]

https://codereview.chromium.org/1288063004/diff/1/remoting/client/jni/jni_fra...
File remoting/client/jni/jni_frame_consumer.cc (left):

https://codereview.chromium.org/1288063004/diff/1/remoting/client/jni/jni_fra...
remoting/client/jni/jni_frame_consumer.cc:80: jni_instance_->RecordPaintTime(
Are we recording the paint time stats somewhere else now?

[Sergey Ulanov, 2015-08-17 22:36:41]

https://codereview.chromium.org/1288063004/diff/1/remoting/client/jni/jni_fra...
File remoting/client/jni/jni_frame_consumer.cc (left):

https://codereview.chromium.org/1288063004/diff/1/remoting/client/jni/jni_fra...
remoting/client/jni/jni_frame_consumer.cc:80: jni_instance_->RecordPaintTime(
On 2015/08/17 22:17:44, Lambros wrote:
> Are we recording the paint time stats somewhere else now?

Yes, see SoftwareVideoRenderer, so this logic is no longer duplicated between
multiple FrameConsumer implementations.

[Jamie, 2015-08-18 20:46:11]

On 2015/08/17 22:36:41, Sergey Ulanov wrote:
>
https://codereview.chromium.org/1288063004/diff/1/remoting/client/jni/jni_fra...
> File remoting/client/jni/jni_frame_consumer.cc (left):
> 
>
https://codereview.chromium.org/1288063004/diff/1/remoting/client/jni/jni_fra...
> remoting/client/jni/jni_frame_consumer.cc:80: jni_instance_->RecordPaintTime(
> On 2015/08/17 22:17:44, Lambros wrote:
> > Are we recording the paint time stats somewhere else now?
> 
> Yes, see SoftwareVideoRenderer, so this logic is no longer duplicated between
> multiple FrameConsumer implementations.

This is a lot of code to review in one go. Is it possible to break it into more
manageable chunks? For example, could the simplified thread logic be reviewed
separately?

[Fady Samuel, 2015-08-18 20:48:16]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2015-08-18 20:48:16]

Drive-by nit (feel free to ignore): As per discussion on chrome-gpu@, could you
use a prefix to disambiguate what "Frame" is here?

[Sergey Ulanov, 2015-08-18 22:45:37]

On 2015/08/18 20:48:16, Fady Samuel wrote:
> Drive-by nit (feel free to ignore): As per discussion on chrome-gpu@, could
you
> use a prefix to disambiguate what "Frame" is here?

This code has nothing to do with the compositor in chrome (it's not even
compiled in chrome) and I don't think there is any confusion about frames in
src/remoting code. 

I've updated the description to make it clear that this is a remoting-specific
change.

[Sergey Ulanov, 2015-08-18 23:22:10]

On 2015/08/18 20:46:11, Jamie wrote:
> On 2015/08/17 22:36:41, Sergey Ulanov wrote:
> >
>
https://codereview.chromium.org/1288063004/diff/1/remoting/client/jni/jni_fra...
> > File remoting/client/jni/jni_frame_consumer.cc (left):
> > 
> >
>
https://codereview.chromium.org/1288063004/diff/1/remoting/client/jni/jni_fra...
> > remoting/client/jni/jni_frame_consumer.cc:80:
jni_instance_->RecordPaintTime(
> > On 2015/08/17 22:17:44, Lambros wrote:
> > > Are we recording the paint time stats somewhere else now?
> > 
> > Yes, see SoftwareVideoRenderer, so this logic is no longer duplicated
between
> > multiple FrameConsumer implementations.
> 
> This is a lot of code to review in one go. Is it possible to break it into
more
> manageable chunks? For example, could the simplified thread logic be reviewed
> separately?

I don't think it's worth trying to separate threading logic from interface
refactoring as these issues are interrelated: we'd get 2 separate CL that would
be almost as big as this one. 
As an alternative I can land the new version of FrameConsumer and
SoftwareVideoRenderer with a new names and remove old code later. I.e.
 1. Add FrameConsumer2 and SoftwareVideoRenderer2, without any changes in
existing code (I'll need to come up with better names, please let me know if you
have suggestions).
 2. Refactor PepperVideoRenderer2D to implement FrameConsumer2 and use
SoftwareVideoRenderer2.
 3. Refactor rendering on android to use SoftwareVideoRenderer2.
 4. remove FrameConsumer and SoftwareVideoRenderer, FrameProducer,
FrameProducerProxy

WDYT?

[Sergey Ulanov, 2015-08-19 00:08:30]

Moved VideoDecoder::Initialize() removal here:
https://codereview.chromium.org/1298863003/

[Jamie, 2015-08-19 00:11:12]

On 2015/08/19 00:08:30, Sergey Ulanov wrote:
> Moved VideoDecoder::Initialize() removal here:
> https://codereview.chromium.org/1298863003/

If there aren't any obvious seams you can split along, I'll review it as-is. It
might take me a while though.

[Sergey Ulanov, 2015-08-19 18:13:57]

Patchset #3 (id:40001) has been deleted

[Jamie, 2015-08-19 20:52:10]

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_frame_consumer.cc:103:
base::Bind(&Renderer::RenderFrame, base::Unretained(renderer_.get()),
Why is base::Unretained safe here, when you're using a weak pointer to post the
reply back to |this|?

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_frame_consumer.h (right):

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_frame_consumer.h:25: class FrameProducer;
This class no longer exists.

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_frame_consumer.h:39: void
DrawFrame(scoped_ptr<webrtc::DesktopFrame> frame) override;
This is missing the |done| parameter. I'm surprised it compiles since you've
specified "override".

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:563: //   video_renderer_.reset();
Did you leave this commented out intentionally? If so, please also add a TODO to
either reinstate or remove it.

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

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/plugin/...
remoting/client/plugin/pepper_video_renderer_2d.cc:205:
flushing_frames_done_callbacks_ = pending_frames_done_callbacks_.Pass();
Add DCHECK(flushing_frames_don_callbacks_.empty())?

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/softwar...
File remoting/client/software_video_renderer.cc (right):

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/softwar...
remoting/client/software_video_renderer.cc:82: frame.reset();
Won't this cause a null deref on the next line?

[Sergey Ulanov, 2015-08-19 23:37:13]

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_frame_consumer.cc:103:
base::Bind(&Renderer::RenderFrame, base::Unretained(renderer_.get()),
On 2015/08/19 20:52:10, Jamie wrote:
> Why is base::Unretained safe here, when you're using a weak pointer to post
the
> reply back to |this|?

This task is posted for the renderer_ and the renderer_ is deleted on the
display thread - see ~JniFrameConsumer().

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_frame_consumer.h (right):

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_frame_consumer.h:25: class FrameProducer;
On 2015/08/19 20:52:10, Jamie wrote:
> This class no longer exists.

Done.

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_frame_consumer.h:39: void
DrawFrame(scoped_ptr<webrtc::DesktopFrame> frame) override;
On 2015/08/19 20:52:10, Jamie wrote:
> This is missing the |done| parameter. I'm surprised it compiles since you've
> specified "override".

Done.

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:563: //   video_renderer_.reset();
On 2015/08/19 20:52:10, Jamie wrote:
> Did you leave this commented out intentionally? If so, please also add a TODO
to
> either reinstate or remove it.

No. Commented it just to test the other changes in this CL and forget to revert.

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

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/plugin/...
remoting/client/plugin/pepper_video_renderer_2d.cc:205:
flushing_frames_done_callbacks_ = pending_frames_done_callbacks_.Pass();
On 2015/08/19 20:52:10, Jamie wrote:
> Add DCHECK(flushing_frames_don_callbacks_.empty())?

Done.

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/softwar...
File remoting/client/software_video_renderer.cc (right):

https://codereview.chromium.org/1288063004/diff/20001/remoting/client/softwar...
remoting/client/software_video_renderer.cc:82: frame.reset();
On 2015/08/19 20:52:10, Jamie wrote:
> Won't this cause a null deref on the next line?

It would. Thanks for catching it!

[Jamie, 2015-08-20 00:01:46]

lgtm

[Sergey Ulanov, 2015-08-20 00:22:22]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-08-20 00:22:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288063004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288063004/80001

[commit-bot: I haz the power, 2015-08-20 02:02:27]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-08-20 02:03:08]

Patchset 4 (id:??) landed as
https://crrev.com/13bca69cfa261ec8eb6bffecf59c486b683eb8db
Cr-Commit-Position: refs/heads/master@{#344404}