Remove frame time scheduing in IncomingVideoStream

Remove frame time scheduing in IncomingVideoStream

This is part of the project that makes RTC rendering more
smooth. We've already finished the developement of the
frame selection algorithm in WebMediaPlayerMS, where we
managed a frame pool, and based on the vsync interval, we
actively select the best frame to render in order to
maximize the rendering smoothness.

Thus the frame timeline control in IncomingVideoStream is
no longer needed, because with sophisticated frame
selection algorithm in WebMediaPlayerMS, the time control
in IncomingVideoStream will do nothing but add some extra
delay.

BUG=514873
Committed: https://crrev.com/444682acf9804c5fcbddaded9e900ba3cc6921fc
Cr-Commit-Position: refs/heads/master@{#10781}

[qiangchen, 2015-11-05 19:42:02]

Description was changed from

==========
Remove scheduling algorithm in WebRTC

BUG=
==========

to

==========
Remove frame time scheduing in IncomingVideoStream

This is part of the project that makes RTC rendering more smooth. We've already
finished the developement of the frame selection algorithm in WebMediaPlayerMS,
where we managed a frame pool, and based on the vsync interval, we actively
select the best frame to render in order to maximize the rendering smoothness.

Thus the frame timeline control in IncomingVideoStream is no longer needed,
because with sophisticated frame selection algorithm in WebMediaPlayerMS, the
time control in IncomingVideoStream will do nothing but add some extra delay.

BUG=514873
==========

[qiangchen, 2015-11-05 21:16:00]

Patchset #1 (id:1) has been deleted

[qiangchen, 2015-11-05 21:17:15]

Description was changed from

==========
Remove frame time scheduing in IncomingVideoStream

This is part of the project that makes RTC rendering more smooth. We've already
finished the developement of the frame selection algorithm in WebMediaPlayerMS,
where we managed a frame pool, and based on the vsync interval, we actively
select the best frame to render in order to maximize the rendering smoothness.

Thus the frame timeline control in IncomingVideoStream is no longer needed,
because with sophisticated frame selection algorithm in WebMediaPlayerMS, the
time control in IncomingVideoStream will do nothing but add some extra delay.

BUG=514873
==========

to

==========
Remove frame time scheduing in IncomingVideoStream

This is part of the project that makes RTC rendering more
smooth. We've already finished the developement of the
frame selection algorithm in WebMediaPlayerMS, where we
managed a frame pool, and based on the vsync interval, we
actively select the best frame to render in order to
maximize the rendering smoothness.

Thus the frame timeline control in IncomingVideoStream is
no longer needed, because with sophisticated frame 
election algorithm in WebMediaPlayerMS, the time control in
IncomingVideoStream will do nothing but add some extra 
delay.

BUG=514873
==========

[qiangchen, 2015-11-05 21:17:50]

Description was changed from

==========
Remove frame time scheduing in IncomingVideoStream

This is part of the project that makes RTC rendering more
smooth. We've already finished the developement of the
frame selection algorithm in WebMediaPlayerMS, where we
managed a frame pool, and based on the vsync interval, we
actively select the best frame to render in order to
maximize the rendering smoothness.

Thus the frame timeline control in IncomingVideoStream is
no longer needed, because with sophisticated frame 
election algorithm in WebMediaPlayerMS, the time control in
IncomingVideoStream will do nothing but add some extra 
delay.

BUG=514873
==========

to

==========
Remove frame time scheduing in IncomingVideoStream

This is part of the project that makes RTC rendering more
smooth. We've already finished the developement of the
frame selection algorithm in WebMediaPlayerMS, where we
managed a frame pool, and based on the vsync interval, we
actively select the best frame to render in order to
maximize the rendering smoothness.

Thus the frame timeline control in IncomingVideoStream is
no longer needed, because with sophisticated frame 
selection algorithm in WebMediaPlayerMS, the time control
in IncomingVideoStream will do nothing but add some extra 
delay.

BUG=514873
==========

[qiangchen, 2015-11-05 22:14:01]

qiangchen@chromium.org changed reviewers:
+ niklase@chromium.org

[qiangchen, 2015-11-05 22:14:01]

Hi, Niklas:

    Can you take a preliminary look at this CL. It is doing what we mentioned
yesterday of removing old webrtc smoothness control.

[Niklas Enbom, 2015-11-05 22:34:10]

You can't nuke this functionality completely since it's being used in non-Chrome
use cases. You need to bypass the frame delay for the Chrome use case. Also, use
niklas.enbom@webrtc.org for reviews in this repo.

On 2015/11/05 22:14:01, qiangchenC wrote:
> Hi, Niklas:
> 
>     Can you take a preliminary look at this CL. It is doing what we mentioned
> yesterday of removing old webrtc smoothness control.

[qiangchen, 2015-11-06 17:14:37]

On 2015/11/05 22:34:10, Niklas Enbom wrote:
> You can't nuke this functionality completely since it's being used in
non-Chrome
> use cases. You need to bypass the frame delay for the Chrome use case. Also,
use
> mailto:niklas.enbom@webrtc.org for reviews in this repo.
> 
> On 2015/11/05 22:14:01, qiangchenC wrote:
> > Hi, Niklas:
> > 
> >     Can you take a preliminary look at this CL. It is doing what we
mentioned
> > yesterday of removing old webrtc smoothness control.

Got it. This sounds more complicated than I thought, I'll do more investigation
on
how to do it.

[Niklas Enbom, 2015-11-06 17:15:59]

I suggest you reach out to pbos and mflodman to get some input on how to best
accomplish this.

On 2015/11/06 17:14:37, qiangchenC wrote:
> On 2015/11/05 22:34:10, Niklas Enbom wrote:
> > You can't nuke this functionality completely since it's being used in
> non-Chrome
> > use cases. You need to bypass the frame delay for the Chrome use case. Also,
> use
> > mailto:niklas.enbom@webrtc.org for reviews in this repo.
> > 
> > On 2015/11/05 22:14:01, qiangchenC wrote:
> > > Hi, Niklas:
> > > 
> > >     Can you take a preliminary look at this CL. It is doing what we
> mentioned
> > > yesterday of removing old webrtc smoothness control.
> 
> Got it. This sounds more complicated than I thought, I'll do more
investigation
> on
> how to do it.

[pbos-webrtc, 2015-11-10 18:41:56]

pbos@webrtc.org changed reviewers:
+ mflodman@webrtc.org, pbos@webrtc.org, stefan@webrtc.org
- niklase@chromium.org

[pbos-webrtc, 2015-11-10 18:41:57]

Yeah, this seems a bit drastic for non-Chromium cases. You should be able to get
the frames to be released out of here ASAP?

+cc magjed/perkj@ in case this change starts affecting the number of output
buffers in the pipe.

https://codereview.webrtc.org/1419673014/diff/20001/webrtc/common_video/video...
File webrtc/common_video/video_render_frames.cc (left):

https://codereview.webrtc.org/1419673014/diff/20001/webrtc/common_video/video...
webrtc/common_video/video_render_frames.cc:22: const uint32_t kMinRenderDelayMs
= 10;
Would just removing this min value fix your problem?

[pbos-webrtc, 2015-11-10 18:51:59]

I believe you wanna add a virtual bool HandlesRenderTiming() const; to:
https://chromium.googlesource.com/external/webrtc/+/ed8275a44fc13015e6784bf0a...

How to get it from Chromium and wire it up might be the tricky part.

[qiangchen, 2015-11-13 20:23:01]

Patch 2 is the implementation of 3.C in the design doc.
PTAL.

https://codereview.chromium.org/1419673014/diff/20001/webrtc/common_video/vid...
File webrtc/common_video/video_render_frames.cc (left):

https://codereview.chromium.org/1419673014/diff/20001/webrtc/common_video/vid...
webrtc/common_video/video_render_frames.cc:22: const uint32_t kMinRenderDelayMs
= 10;
On 2015/11/10 18:41:57, pbos-webrtc wrote:
> Would just removing this min value fix your problem?

I do not think so.

IncomingVideoStream will release a frame at Target_Render_Time - RenderDelay. 
So if we decrease the RenderDelay, IncomingVideoStream will hold a frame for
longer.

Anyway, we need to figure out a way specific to Chrome.

[pbos, 2015-11-16 18:38:46]

pbos@chromium.org changed reviewers:
+ pbos@chromium.org, pthatcher@webrtc.org

[pbos, 2015-11-16 18:38:46]

+R pthatcher@, can you look at talk/ parts?

https://codereview.chromium.org/1419673014/diff/40001/talk/media/webrtc/webrt...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.chromium.org/1419673014/diff/40001/talk/media/webrtc/webrt...
talk/media/webrtc/webrtcvideoengine2.cc:1229: config.renderer_has_time_control =
options_.renderer_has_time_control;
I want this to be an option on the renderer for VideoRenderer:

pbos@deimos:~/webrtc/src (master)$ git diff
diff --git a/webrtc/video_renderer.h b/webrtc/video_renderer.h
index fedd28b..058a37a 100644
--- a/webrtc/video_renderer.h
+++ b/webrtc/video_renderer.h
@@ -25,6 +25,8 @@ class VideoRenderer {
 
   virtual bool IsTextureSupported() const = 0;
 
+  virtual bool HasTimeControl() const { return false; }
+
  protected:
   virtual ~VideoRenderer() {}
 };


Then you override that one in the renderer implementation (which IIRC is
WebRtcVideoReceiveStream).

[pbos, 2015-11-16 18:40:55]

https://codereview.chromium.org/1419673014/diff/40001/talk/media/webrtc/webrt...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.chromium.org/1419673014/diff/40001/talk/media/webrtc/webrt...
talk/media/webrtc/webrtcvideoengine2.cc:1229: config.renderer_has_time_control =
options_.renderer_has_time_control;
On 2015/11/16 18:38:46, pbos wrote:
> I want this to be an option on the renderer for VideoRenderer:
> 
> pbos@deimos:~/webrtc/src (master)$ git diff
> diff --git a/webrtc/video_renderer.h b/webrtc/video_renderer.h
> index fedd28b..058a37a 100644
> --- a/webrtc/video_renderer.h
> +++ b/webrtc/video_renderer.h
> @@ -25,6 +25,8 @@ class VideoRenderer {
>  
>    virtual bool IsTextureSupported() const = 0;
>  
> +  virtual bool HasTimeControl() const { return false; }
> +
>   protected:
>    virtual ~VideoRenderer() {}
>  };
> 
> 
> Then you override that one in the renderer implementation (which IIRC is
> WebRtcVideoReceiveStream).

Maybe ::SchedulesFrames(), I dunno, wdyt?

[pthatcher1, 2015-11-16 20:25:28]

lgtm of talk/ parts, except that I think there must be a better name than
"renderer_has_time_control".  How about something like "smooth_frame_output",
where the default is true?

https://codereview.chromium.org/1419673014/diff/40001/talk/media/webrtc/webrt...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.chromium.org/1419673014/diff/40001/talk/media/webrtc/webrt...
talk/media/webrtc/webrtcvideoengine2.cc:1229: config.renderer_has_time_control =
options_.renderer_has_time_control;
On 2015/11/16 18:40:55, pbos wrote:
> On 2015/11/16 18:38:46, pbos wrote:
> > I want this to be an option on the renderer for VideoRenderer:
> > 
> > pbos@deimos:~/webrtc/src (master)$ git diff
> > diff --git a/webrtc/video_renderer.h b/webrtc/video_renderer.h
> > index fedd28b..058a37a 100644
> > --- a/webrtc/video_renderer.h
> > +++ b/webrtc/video_renderer.h
> > @@ -25,6 +25,8 @@ class VideoRenderer {
> >  
> >    virtual bool IsTextureSupported() const = 0;
> >  
> > +  virtual bool HasTimeControl() const { return false; }
> > +
> >   protected:
> >    virtual ~VideoRenderer() {}
> >  };
> > 
> > 
> > Then you override that one in the renderer implementation (which IIRC is
> > WebRtcVideoReceiveStream).
> 
> Maybe ::SchedulesFrames(), I dunno, wdyt?

That gets kind of complicated, because there can be multiple renderers.  So what
do you do if one does its own time control but one doesn't?  We ran into a
similar issue with frame rotation.   It gets pretty messy and quickly leads into
a discussion of refactoring all of our renderers everywhere.

https://codereview.chromium.org/1419673014/diff/40001/webrtc/video/video_rece...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.chromium.org/1419673014/diff/40001/webrtc/video/video_rece...
webrtc/video/video_receive_stream.cc:275: incoming_video_stream_.reset(
Would it make sense to just pass in the whole config object?

[qiangchen, 2015-11-16 20:44:14]

Publish some replies first. 
I'll
1. Move the flag from Config to VideoRenderer, as suggested by pbos.
2. Implement the way to store flag in WebRtcVideoEngine2, as suggested by
pthatcher.

Then we can compare and make a decision.

https://codereview.chromium.org/1419673014/diff/40001/talk/media/webrtc/webrt...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.chromium.org/1419673014/diff/40001/talk/media/webrtc/webrt...
talk/media/webrtc/webrtcvideoengine2.cc:1229: config.renderer_has_time_control =
options_.renderer_has_time_control;
On 2015/11/16 18:40:55, pbos wrote:
> On 2015/11/16 18:38:46, pbos wrote:
> > I want this to be an option on the renderer for VideoRenderer:
> > 
> > pbos@deimos:~/webrtc/src (master)$ git diff
> > diff --git a/webrtc/video_renderer.h b/webrtc/video_renderer.h
> > index fedd28b..058a37a 100644
> > --- a/webrtc/video_renderer.h
> > +++ b/webrtc/video_renderer.h
> > @@ -25,6 +25,8 @@ class VideoRenderer {
> >  
> >    virtual bool IsTextureSupported() const = 0;
> >  
> > +  virtual bool HasTimeControl() const { return false; }
> > +
> >   protected:
> >    virtual ~VideoRenderer() {}
> >  };
> > 
> > 
> > Then you override that one in the renderer implementation (which IIRC is
> > WebRtcVideoReceiveStream).
> 
> Maybe ::SchedulesFrames(), I dunno, wdyt?

The reason I chose Config to carry this piece of info is that it is passed to
downstream class on the current code path, so this way we do not need to add
parameter to any functions.

Just noticed that Config has a back pointer to the VideoRenderer, so it is
feasible to move this property into VideoRenderer.

I'll do it.

https://codereview.chromium.org/1419673014/diff/40001/talk/media/webrtc/webrt...
talk/media/webrtc/webrtcvideoengine2.cc:1229: config.renderer_has_time_control =
options_.renderer_has_time_control;
On 2015/11/16 20:25:27, pthatcher1 wrote:
> On 2015/11/16 18:40:55, pbos wrote:
> > On 2015/11/16 18:38:46, pbos wrote:
> > > I want this to be an option on the renderer for VideoRenderer:
> > > 
> > > pbos@deimos:~/webrtc/src (master)$ git diff
> > > diff --git a/webrtc/video_renderer.h b/webrtc/video_renderer.h
> > > index fedd28b..058a37a 100644
> > > --- a/webrtc/video_renderer.h
> > > +++ b/webrtc/video_renderer.h
> > > @@ -25,6 +25,8 @@ class VideoRenderer {
> > >  
> > >    virtual bool IsTextureSupported() const = 0;
> > >  
> > > +  virtual bool HasTimeControl() const { return false; }
> > > +
> > >   protected:
> > >    virtual ~VideoRenderer() {}
> > >  };
> > > 
> > > 
> > > Then you override that one in the renderer implementation (which IIRC is
> > > WebRtcVideoReceiveStream).
> > 
> > Maybe ::SchedulesFrames(), I dunno, wdyt?
> 
> That gets kind of complicated, because there can be multiple renderers.  So
what
> do you do if one does its own time control but one doesn't?  We ran into a
> similar issue with frame rotation.   It gets pretty messy and quickly leads
into
> a discussion of refactoring all of our renderers everywhere.

For the current data structure, one VideoRenderer (essentially
WebRtcVideoReceiveStream) corresponds to one IncomingVideoStream. In another
word one IncomingVideoStream will serve one VideoRenderer only, so there is no
such problem as "some has time control, some not".

[qiangchen, 2015-11-17 00:01:34]

Patchset #4 (id:80001) has been deleted

[qiangchen, 2015-11-17 00:09:28]

Implemented the other way as pthatcher suggested to store the flag in
WebRtcVideoEngine2 as Patch4.

Patch3 is a modification of Patch2, which moves storage of the flag in Config
into VideoRenderer.
PTAL.

https://codereview.chromium.org/1419673014/diff/100001/talk/app/webrtc/peerco...
File talk/app/webrtc/peerconnectionfactory.cc (right):

https://codereview.chromium.org/1419673014/diff/100001/talk/app/webrtc/peerco...
talk/app/webrtc/peerconnectionfactory.cc:106:
rtc::scoped_refptr<PeerConnectionFactoryInterface> CreatePeerConnectionFactory(
This is the function Chromium will call to create peer connection factory, which
in turn instantiate WebRtcVideoEngine2.

[qiangchen, 2015-11-17 18:58:54]

Patchset #4 (id:100001) has been deleted

[qiangchen, 2015-11-17 19:08:12]

Rename the flag to a more descriptive name: renderer_can_schedule_frames.

mflodman@: Can you take a look at this CL? You are the only reviewer who owns
the "webrtc/" files. The motivation is stated in the design doc. 
This CL implements solution 3.C in the design.

[qiangchen, 2015-11-17 19:27:04]

Patchset #5 (id:140001) has been deleted

[qiangchen, 2015-11-17 19:28:07]

Patchset #5 (id:160001) has been deleted

[qiangchen, 2015-11-17 19:32:35]

Patchset #5 (id:180001) has been deleted

[qiangchen, 2015-11-17 21:17:08]

Patchset #5 (id:130010) has been deleted

[qiangchen, 2015-11-17 22:11:12]

Patchset #5 (id:80002) has been deleted

[qiangchen, 2015-11-17 22:11:21]

Patchset #5 (id:220001) has been deleted

[qiangchen, 2015-11-19 17:34:06]

ping mflodman@ to take a look at it.

[pthatcher1, 2015-11-19 22:19:25]

I'm picky about names, and I'd like to see this one match it's real meaning a
little better.

Otherwise, everything still looks fine.

https://codereview.chromium.org/1419673014/diff/240001/talk/media/base/mediac...
File talk/media/base/mediachannel.h (right):

https://codereview.chromium.org/1419673014/diff/240001/talk/media/base/mediac...
talk/media/base/mediachannel.h:390: rtc::Optional<bool>
renderer_can_schedule_frames;
I don't like this name very much.

The comment makes it seem like a better name would be. 
"disable_prerenderer_smoothing"(default false) or
"smooth_frames_before_renderer" (default true).

https://codereview.chromium.org/1419673014/diff/240001/talk/media/webrtc/webr...
File talk/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.chromium.org/1419673014/diff/240001/talk/media/webrtc/webr...
talk/media/webrtc/webrtcvideoengine2.h:419: bool CanScheduleFrame() const
override;
It should be plural: CanScheduleFrames, but is it that it *can* schedule/smooth
frames, or that it *does*?  In other words, should it be "SchedulesFrames"?  And
should it be "SmoothesFrames"?

[qiangchen, 2015-11-20 17:54:06]

ping mflodman@ again.

Renamed the flag to disable_prerenderer_smoothing, as it is more descriptive.

https://codereview.chromium.org/1419673014/diff/240001/talk/media/base/mediac...
File talk/media/base/mediachannel.h (right):

https://codereview.chromium.org/1419673014/diff/240001/talk/media/base/mediac...
talk/media/base/mediachannel.h:390: rtc::Optional<bool>
renderer_can_schedule_frames;
On 2015/11/19 22:19:25, pthatcher1 wrote:
> I don't like this name very much.
> 
> The comment makes it seem like a better name would be. 
> "disable_prerenderer_smoothing"(default false) or
> "smooth_frames_before_renderer" (default true).
> 

Done.

https://codereview.chromium.org/1419673014/diff/240001/talk/media/webrtc/webr...
File talk/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.chromium.org/1419673014/diff/240001/talk/media/webrtc/webr...
talk/media/webrtc/webrtcvideoengine2.h:419: bool CanScheduleFrame() const
override;
On 2015/11/19 22:19:25, pthatcher1 wrote:
> It should be plural: CanScheduleFrames, but is it that it *can*
schedule/smooth
> frames, or that it *does*?  In other words, should it be "SchedulesFrames"? 
And
> should it be "SmoothesFrames"?

Done.

Not this webrtc::VideoRenderer can schedule frames. So changed to the same name
with other places to disable_prerenderer_smoothing.

[pthatcher1, 2015-11-20 22:09:46]

lgtm

[pbos, 2015-11-23 14:18:12]

pbos@chromium.org changed reviewers:
- pbos@chromium.org

[pbos-webrtc, 2015-11-23 14:28:05]

I'm thinking of maybe calling VideoRenderer::PrerenderSmoothingDisabled() ->
VideoRenderer::SchedulesRenderedFrames() because it's a property of the
VideoRenderer that it performs its own scheduling.

WDYT?

https://codereview.webrtc.org/1419673014/diff/260001/webrtc/common_video/incl...
File webrtc/common_video/include/incoming_video_stream.h (right):

https://codereview.webrtc.org/1419673014/diff/260001/webrtc/common_video/incl...
webrtc/common_video/include/incoming_video_stream.h:35: explicit
IncomingVideoStream(uint32_t stream_id);
Can you just have a bool disable_prerender_smoothing here? We should know this
on construction, so you should be able to pass
renderer->PrerenderSmoothingDisabled(), this shouldn't ever change during
execution. Then also only have one constructor.

https://codereview.webrtc.org/1419673014/diff/260001/webrtc/common_video/inte...
File webrtc/common_video/interface/incoming_video_stream.h (right):

https://codereview.webrtc.org/1419673014/diff/260001/webrtc/common_video/inte...
webrtc/common_video/interface/incoming_video_stream.h:80: VideoRenderer* const
renderer_;
this should be a const bool instead of a pointer that you make vtable calls on
per frame

https://codereview.webrtc.org/1419673014/diff/260001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/1419673014/diff/260001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:273: incoming_video_stream_.reset(new
IncomingVideoStream(0, config.renderer));
config.renderer->PrerenderSmoothingDisabled()

https://codereview.webrtc.org/1419673014/diff/260001/webrtc/video_renderer.h
File webrtc/video_renderer.h (right):

https://codereview.webrtc.org/1419673014/diff/260001/webrtc/video_renderer.h#...
webrtc/video_renderer.h:28: // This function returns true if WebRTC do not delay
frames for smoothness.
s/do not/should not

[mflodman, 2015-11-23 14:43:44]

https://codereview.chromium.org/1419673014/diff/260001/talk/media/webrtc/webr...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.chromium.org/1419673014/diff/260001/talk/media/webrtc/webr...
talk/media/webrtc/webrtcvideoengine2.cc:2348:
WebRtcVideoChannel2::WebRtcVideoReceiveStream::WebRtcVideoReceiveStream(
I'd prefer to only have on constructor.

https://codereview.chromium.org/1419673014/diff/260001/webrtc/common_video/in...
File webrtc/common_video/include/incoming_video_stream.h (right):

https://codereview.chromium.org/1419673014/diff/260001/webrtc/common_video/in...
webrtc/common_video/include/incoming_video_stream.h:35: explicit
IncomingVideoStream(uint32_t stream_id);
On 2015/11/23 14:28:05, pbos-webrtc wrote:
> Can you just have a bool disable_prerender_smoothing here? We should know this
> on construction, so you should be able to pass
> renderer->PrerenderSmoothingDisabled(), this shouldn't ever change during
> execution. Then also only have one constructor.

+1, I assume this won't change during the life time of the renderer. And then
only have one constructor.

[pthatcher1, 2015-11-23 21:39:46]

On 2015/11/23 14:28:05, pbos-webrtc wrote:
> I'm thinking of maybe calling VideoRenderer::PrerenderSmoothingDisabled() ->
> VideoRenderer::SchedulesRenderedFrames() because it's a property of the
> VideoRenderer that it performs its own scheduling.
> 
> WDYT?

I think having the renderer say "I smooth/schedule frames" and the thing that
uses the renderer say "I don't smooth/schedule frames before giving them to the
renderer" makes sense.

So, that sounds good.

Except for one thing:  can we be consistent between using the word "smooth" and
"schedule"?  I prefer the word "smooth" since it explains the intent, not just
the mechanic of what is being done.  But I care more about consistency than
which name is picked.

> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/common_video/incl...
> File webrtc/common_video/include/incoming_video_stream.h (right):
> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/common_video/incl...
> webrtc/common_video/include/incoming_video_stream.h:35: explicit
> IncomingVideoStream(uint32_t stream_id);
> Can you just have a bool disable_prerender_smoothing here? We should know this
> on construction, so you should be able to pass
> renderer->PrerenderSmoothingDisabled(), this shouldn't ever change during
> execution. Then also only have one constructor.
> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/common_video/inte...
> File webrtc/common_video/interface/incoming_video_stream.h (right):
> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/common_video/inte...
> webrtc/common_video/interface/incoming_video_stream.h:80: VideoRenderer* const
> renderer_;
> this should be a const bool instead of a pointer that you make vtable calls on
> per frame
> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/video/video_recei...
> File webrtc/video/video_receive_stream.cc (right):
> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/video/video_recei...
> webrtc/video/video_receive_stream.cc:273: incoming_video_stream_.reset(new
> IncomingVideoStream(0, config.renderer));
> config.renderer->PrerenderSmoothingDisabled()
> 
> https://codereview.webrtc.org/1419673014/diff/260001/webrtc/video_renderer.h
> File webrtc/video_renderer.h (right):
> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/video_renderer.h#...
> webrtc/video_renderer.h:28: // This function returns true if WebRTC do not
delay
> frames for smoothness.
> s/do not/should not

[pthatcher1, 2015-11-23 21:39:47]

On 2015/11/23 14:28:05, pbos-webrtc wrote:
> I'm thinking of maybe calling VideoRenderer::PrerenderSmoothingDisabled() ->
> VideoRenderer::SchedulesRenderedFrames() because it's a property of the
> VideoRenderer that it performs its own scheduling.
> 
> WDYT?

I think having the renderer say "I smooth/schedule frames" and the thing that
uses the renderer say "I don't smooth/schedule frames before giving them to the
renderer" makes sense.

So, that sounds good.

Except for one thing:  can we be consistent between using the word "smooth" and
"schedule"?  I prefer the word "smooth" since it explains the intent, not just
the mechanic of what is being done.  But I care more about consistency than
which name is picked.

> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/common_video/incl...
> File webrtc/common_video/include/incoming_video_stream.h (right):
> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/common_video/incl...
> webrtc/common_video/include/incoming_video_stream.h:35: explicit
> IncomingVideoStream(uint32_t stream_id);
> Can you just have a bool disable_prerender_smoothing here? We should know this
> on construction, so you should be able to pass
> renderer->PrerenderSmoothingDisabled(), this shouldn't ever change during
> execution. Then also only have one constructor.
> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/common_video/inte...
> File webrtc/common_video/interface/incoming_video_stream.h (right):
> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/common_video/inte...
> webrtc/common_video/interface/incoming_video_stream.h:80: VideoRenderer* const
> renderer_;
> this should be a const bool instead of a pointer that you make vtable calls on
> per frame
> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/video/video_recei...
> File webrtc/video/video_receive_stream.cc (right):
> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/video/video_recei...
> webrtc/video/video_receive_stream.cc:273: incoming_video_stream_.reset(new
> IncomingVideoStream(0, config.renderer));
> config.renderer->PrerenderSmoothingDisabled()
> 
> https://codereview.webrtc.org/1419673014/diff/260001/webrtc/video_renderer.h
> File webrtc/video_renderer.h (right):
> 
>
https://codereview.webrtc.org/1419673014/diff/260001/webrtc/video_renderer.h#...
> webrtc/video_renderer.h:28: // This function returns true if WebRTC do not
delay
> frames for smoothness.
> s/do not/should not

[qiangchen, 2015-11-23 23:53:47]

Patchset #8 (id:300001) has been deleted

[qiangchen, 2015-11-24 00:16:32]

Patchset #7 (id:280001) has been deleted

[qiangchen, 2015-11-24 00:16:37]

Patchset #7 (id:320001) has been deleted

[qiangchen, 2015-11-24 00:21:05]

Fixed, and remove the extra thread in the disabled case, as render_buffers_ do a
time check which should be by-passed in the disabled case.

https://codereview.chromium.org/1419673014/diff/260001/talk/media/webrtc/webr...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.chromium.org/1419673014/diff/260001/talk/media/webrtc/webr...
talk/media/webrtc/webrtcvideoengine2.cc:2348:
WebRtcVideoChannel2::WebRtcVideoReceiveStream::WebRtcVideoReceiveStream(
On 2015/11/23 14:43:43, mflodman wrote:
> I'd prefer to only have on constructor.

You are right, this is a private class, thus no worry about compatability break.

https://codereview.chromium.org/1419673014/diff/260001/webrtc/common_video/in...
File webrtc/common_video/include/incoming_video_stream.h (right):

https://codereview.chromium.org/1419673014/diff/260001/webrtc/common_video/in...
webrtc/common_video/include/incoming_video_stream.h:35: explicit
IncomingVideoStream(uint32_t stream_id);
On 2015/11/23 14:43:44, mflodman wrote:
> On 2015/11/23 14:28:05, pbos-webrtc wrote:
> > Can you just have a bool disable_prerender_smoothing here? We should know
this
> > on construction, so you should be able to pass
> > renderer->PrerenderSmoothingDisabled(), this shouldn't ever change during
> > execution. Then also only have one constructor.
> 
> +1, I assume this won't change during the life time of the renderer. And then
> only have one constructor.

Done.

But one concern is the compatibility break, if we remove a signature of
constructor.

https://codereview.chromium.org/1419673014/diff/260001/webrtc/common_video/in...
File webrtc/common_video/interface/incoming_video_stream.h (right):

https://codereview.chromium.org/1419673014/diff/260001/webrtc/common_video/in...
webrtc/common_video/interface/incoming_video_stream.h:80: VideoRenderer* const
renderer_;
On 2015/11/23 14:28:05, pbos-webrtc wrote:
> this should be a const bool instead of a pointer that you make vtable calls on
> per frame

Done.

https://codereview.chromium.org/1419673014/diff/260001/webrtc/video/video_rec...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.chromium.org/1419673014/diff/260001/webrtc/video/video_rec...
webrtc/video/video_receive_stream.cc:273: incoming_video_stream_.reset(new
IncomingVideoStream(0, config.renderer));
On 2015/11/23 14:28:05, pbos-webrtc wrote:
> config.renderer->PrerenderSmoothingDisabled()

Done.

https://codereview.chromium.org/1419673014/diff/260001/webrtc/video_renderer.h
File webrtc/video_renderer.h (right):

https://codereview.chromium.org/1419673014/diff/260001/webrtc/video_renderer....
webrtc/video_renderer.h:28: // This function returns true if WebRTC do not delay
frames for smoothness.
On 2015/11/23 14:28:05, pbos-webrtc wrote:
> s/do not/should not

Done.

[mflodman, 2015-11-24 12:21:18]

LGTM

Anything more from you pbos?

https://codereview.chromium.org/1419673014/diff/260001/webrtc/common_video/in...
File webrtc/common_video/include/incoming_video_stream.h (right):

https://codereview.chromium.org/1419673014/diff/260001/webrtc/common_video/in...
webrtc/common_video/include/incoming_video_stream.h:35: explicit
IncomingVideoStream(uint32_t stream_id);
On 2015/11/24 00:21:05, qiangchenC wrote:
> On 2015/11/23 14:43:44, mflodman wrote:
> > On 2015/11/23 14:28:05, pbos-webrtc wrote:
> > > Can you just have a bool disable_prerender_smoothing here? We should know
> this
> > > on construction, so you should be able to pass
> > > renderer->PrerenderSmoothingDisabled(), this shouldn't ever change during
> > > execution. Then also only have one constructor.
> > 
> > +1, I assume this won't change during the life time of the renderer. And
then
> > only have one constructor.
> 
> Done.
> 
> But one concern is the compatibility break, if we remove a signature of
> constructor.

AFAIK this is only used by internal WebRTC code and should be fine as long as it
builds locally on on the bots.

[pbos, 2015-11-24 13:14:39]

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

[pbos, 2015-11-24 13:14:40]

lgtm if you address my nits

https://codereview.chromium.org/1419673014/diff/340001/webrtc/common_video/in...
File webrtc/common_video/incoming_video_stream.cc (right):

https://codereview.chromium.org/1419673014/diff/340001/webrtc/common_video/in...
webrtc/common_video/incoming_video_stream.cc:238: void
IncomingVideoStream::HandOverFrame(const VideoFrame& video_frame) {
Should this be called DeliverFrame? HandOverFrame doesn't make sense to me.

https://codereview.chromium.org/1419673014/diff/340001/webrtc/video_renderer.h
File webrtc/video_renderer.h (right):

https://codereview.chromium.org/1419673014/diff/340001/webrtc/video_renderer....
webrtc/video_renderer.h:31: virtual bool PrerendererSmoothingDisabled() const {
return false; }
Rename this ::SmoothsRenderedFrames() const, it should be named as a property of
the VideoRenderer (something it does/doesn't do)

[qiangchen, 2015-11-24 18:37:05]

Patchset #9 (id:380001) has been deleted

[qiangchen, 2015-11-24 22:25:08]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2015-11-24 22:25:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, pbos@chromium.org, mflodman@webrtc.org
Link to the patchset: https://codereview.chromium.org/1419673014/#ps420001
(title: "Remove unnecessary if")

[commit-bot: I haz the power, 2015-11-24 22:25:18]

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

[commit-bot: I haz the power, 2015-11-24 23:21:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-24 23:21:36]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[qiangchen, 2015-11-25 00:51:47]

The CQ bit was checked by qiangchen@chromium.org

[commit-bot: I haz the power, 2015-11-25 00:52:02]

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

[qiangchen, 2015-11-25 01:03:20]

https://codereview.chromium.org/1419673014/diff/340001/webrtc/common_video/in...
File webrtc/common_video/incoming_video_stream.cc (right):

https://codereview.chromium.org/1419673014/diff/340001/webrtc/common_video/in...
webrtc/common_video/incoming_video_stream.cc:238: void
IncomingVideoStream::HandOverFrame(const VideoFrame& video_frame) {
On 2015/11/24 13:14:40, pbos wrote:
> Should this be called DeliverFrame? HandOverFrame doesn't make sense to me.

Done.

https://codereview.chromium.org/1419673014/diff/340001/webrtc/video_renderer.h
File webrtc/video_renderer.h (right):

https://codereview.chromium.org/1419673014/diff/340001/webrtc/video_renderer....
webrtc/video_renderer.h:31: virtual bool PrerendererSmoothingDisabled() const {
return false; }
On 2015/11/24 13:14:40, pbos wrote:
> Rename this ::SmoothsRenderedFrames() const, it should be named as a property
of
> the VideoRenderer (something it does/doesn't do)

Done.

[commit-bot: I haz the power, 2015-11-25 02:08:00]

Committed patchset #10 (id:420001)

[commit-bot: I haz the power, 2015-11-25 02:08:12]

Patchset 10 (id:??) landed as
https://crrev.com/444682acf9804c5fcbddaded9e900ba3cc6921fc
Cr-Commit-Position: refs/heads/master@{#10781}