Removed all RTP dependencies from ViEChannel and renamed class.

Removed all RTP dependencies from ViEChannel and renamed class.

ViEChannel is now called VideoStreamReceiver.

There will be a follow up CL removing all rtp references from VideoReceiveStream, but that made this CL to big and it will be done separately.

BUG=webrtc:5079
Committed: https://crrev.com/cfc8e3b9ef25eb977e55960c7bac2172298f4d27
Cr-Commit-Position: refs/heads/master@{#12619}

[mflodman, 2016-04-29 12:47:26]

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

[mflodman, 2016-04-29 12:47:27]

Peter,
Here's the followup on the CL from yesterday.

Stefan,
Can you take an extra look at the NACK/FEC logic so I didn't miss anything.

[pbos-webrtc, 2016-05-02 00:59:03]

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_receive_st...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.cc:176: &stats_proxy_,
Can you make the RtpStreamReceiver take a stats proxy as argument instead?

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.cc:180: &rtp_stream_receiver_,
Same here, can this take rtp_stream_receiver_ as one argument and split up
internally?

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
File webrtc/video/video_stream_decoder.cc (right):

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.cc:21: #include
"webrtc/common_video/libyuv/include/webrtc_libyuv.h"
Don't think this include is used.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.cc:31: // Helper class receiving statistics
callbacks.
I don't see how this one is useful, just make VideoStreamEncoder inherit
CallStatsObserver and receive OnRttUpdate directly without a jump.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.cc:83: CallStatsObserver*
VideoStreamDecoder::GetStatsObserver() {
Remove this function, just make use of that VideoStreamDecoder is a
CallStatsObserver (after you make it one)

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.cc:98:
incoming_video_stream_->RenderFrame(0xFFFFFFFF, video_frame);
Provide this one on construction, then you don't need to acquire crit_ here.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.cc:129: receive_frame_counts_ = frame_counts;
Never read, remove and drop crit_ from this function.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.cc:163: void
VideoStreamDecoder::SetIncomingVideoStream(
Remove and set through ctor.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
File webrtc/video/video_stream_decoder.h (left):

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.h:90: // Implements VCMReceiveCallback.
Add back, this still implements that?

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
File webrtc/video/video_stream_decoder.h (right):

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.h:107: // Not owned.
This comment bothers me, can you remove it? :)

[mflodman, 2016-05-02 19:01:27]

Peter,
All comments addressed or answered, ptal.

Stefan,
Any comments?

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_receive_st...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.cc:176: &stats_proxy_,
On 2016/05/02 00:59:02, pbos-webrtc wrote:
> Can you make the RtpStreamReceiver take a stats proxy as argument instead?

Done.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.cc:180: &rtp_stream_receiver_,
On 2016/05/02 00:59:02, pbos-webrtc wrote:
> Same here, can this take rtp_stream_receiver_ as one argument and split up
> internally?

This will very likely change to be signalled to VideoReceiveStream instead of
directly to RtpStreamReceiver, so I'd prefer to not change this. Ok?

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
File webrtc/video/video_stream_decoder.cc (right):

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.cc:21: #include
"webrtc/common_video/libyuv/include/webrtc_libyuv.h"
On 2016/05/02 00:59:02, pbos-webrtc wrote:
> Don't think this include is used.

Done.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.cc:31: // Helper class receiving statistics
callbacks.
On 2016/05/02 00:59:02, pbos-webrtc wrote:
> I don't see how this one is useful, just make VideoStreamEncoder inherit
> CallStatsObserver and receive OnRttUpdate directly without a jump.

Legacy, but removed.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.cc:83: CallStatsObserver*
VideoStreamDecoder::GetStatsObserver() {
On 2016/05/02 00:59:02, pbos-webrtc wrote:
> Remove this function, just make use of that VideoStreamDecoder is a
> CallStatsObserver (after you make it one)

Done.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.cc:98:
incoming_video_stream_->RenderFrame(0xFFFFFFFF, video_frame);
On 2016/05/02 00:59:02, pbos-webrtc wrote:
> Provide this one on construction, then you don't need to acquire crit_ here.

Done.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.cc:129: receive_frame_counts_ = frame_counts;
On 2016/05/02 00:59:02, pbos-webrtc wrote:
> Never read, remove and drop crit_ from this function.

Done.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.cc:163: void
VideoStreamDecoder::SetIncomingVideoStream(
On 2016/05/02 00:59:02, pbos-webrtc wrote:
> Remove and set through ctor.

Done. And for the pre render callback above.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
File webrtc/video/video_stream_decoder.h (left):

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.h:90: // Implements VCMReceiveCallback.
On 2016/05/02 00:59:03, pbos-webrtc wrote:
> Add back, this still implements that?

That is already said above FrameToRender and doesn't have to be stated here too.

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
File webrtc/video/video_stream_decoder.h (right):

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_stream_dec...
webrtc/video/video_stream_decoder.h:107: // Not owned.
On 2016/05/02 00:59:03, pbos-webrtc wrote:
> This comment bothers me, can you remove it? :)

Done.

[pbos-webrtc, 2016-05-02 20:30:30]

lgtm, thanks

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_receive_st...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.cc:180: &rtp_stream_receiver_,
On 2016/05/02 19:01:27, mflodman wrote:
> On 2016/05/02 00:59:02, pbos-webrtc wrote:
> > Same here, can this take rtp_stream_receiver_ as one argument and split up
> > internally?
> 
> This will very likely change to be signalled to VideoReceiveStream instead of
> directly to RtpStreamReceiver, so I'd prefer to not change this. Ok?

Gotcha, that's fine.

[pbos-webrtc, 2016-05-02 21:03:12]

Description was changed from

==========
Removed all RTP dependencies from ViEChannel and renamed class.

ViEChannelis now called VideoStreamReceiver.

There will be a follow up CL removing all rtp references from
VideoReceiveStream, but that made this CL to big and it will be done separately.

BUG=webrtc:5079
==========

to

==========
Removed all RTP dependencies from ViEChannel and renamed class.

ViEChannel is now called VideoStreamReceiver.

There will be a follow up CL removing all rtp references from
VideoReceiveStream, but that made this CL to big and it will be done separately.

BUG=webrtc:5079
==========

[mflodman, 2016-05-03 04:49:26]

On 2016/05/02 20:30:30, pbos-webrtc wrote:
> lgtm, thanks
> 
>
https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_receive_st...
> File webrtc/video/video_receive_stream.cc (right):
> 
>
https://codereview.webrtc.org/1929313002/diff/1/webrtc/video/video_receive_st...
> webrtc/video/video_receive_stream.cc:180: &rtp_stream_receiver_,
> On 2016/05/02 19:01:27, mflodman wrote:
> > On 2016/05/02 00:59:02, pbos-webrtc wrote:
> > > Same here, can this take rtp_stream_receiver_ as one argument and split up
> > > internally?
> > 
> > This will very likely change to be signalled to VideoReceiveStream instead
of
> > directly to RtpStreamReceiver, so I'd prefer to not change this. Ok?
> 
> Gotcha, that's fine.

Well, a prepared design doc could have helped in this case...

Good review, thanks!

[stefan-webrtc, 2016-05-03 07:48:08]

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:120: rtp_rtcp_->SetGenericFECStatus(false,
0, 0);
This is disabled because this is a receiver, right? Maybe comment on that, or
simply don't call it (I don't think you have to).

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.cc (left):

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:192: -1);
Where's this happening now?

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_stream...
File webrtc/video/video_stream_decoder.cc (right):

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_stream...
webrtc/video/video_stream_decoder.cc:57: : kProtectionNone;
We should inform about FEC here as well if both NACK and FEC is enabled.

philipel has a CL which does that here https://codereview.webrtc.org/1942683003/
which will conflict with your CL.

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_stream...
File webrtc/video/video_stream_decoder.h (right):

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_stream...
webrtc/video/video_stream_decoder.h:61: IncomingVideoStream*
incoming_video_stream,
Can this be a VideoRenderCallback instead?

[mflodman, 2016-05-03 07:56:52]

Answered your comments Stefan.

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:120: rtp_rtcp_->SetGenericFECStatus(false,
0, 0);
On 2016/05/03 07:48:08, stefan-webrtc (holmer) wrote:
> This is disabled because this is a receiver, right? Maybe comment on that, or
> simply don't call it (I don't think you have to).

Yes, this is copied from ViEChannel and how it was pre this CL. I can remove if
it will be ok without it.

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.cc (left):

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:192: -1);
On 2016/05/03 07:48:08, stefan-webrtc (holmer) wrote:
> Where's this happening now?

See line 113 in rtp_stream_receiver.cc and line 56 in video_stream_decoder.cc.
This is the most sensitive part of this CL and would be good to get a second
look at.

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_stream...
File webrtc/video/video_stream_decoder.cc (right):

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_stream...
webrtc/video/video_stream_decoder.cc:57: : kProtectionNone;
On 2016/05/03 07:48:08, stefan-webrtc (holmer) wrote:
> We should inform about FEC here as well if both NACK and FEC is enabled.
> 
> philipel has a CL which does that here
https://codereview.webrtc.org/1942683003/
> which will conflict with your CL.

Acknowledged.

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_stream...
File webrtc/video/video_stream_decoder.h (right):

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_stream...
webrtc/video/video_stream_decoder.h:61: IncomingVideoStream*
incoming_video_stream,
On 2016/05/03 07:48:08, stefan-webrtc (holmer) wrote:
> Can this be a VideoRenderCallback instead?

That is being removed in another CL by nisse and I'd prefer to now change at
this point.

[stefan-webrtc, 2016-05-03 08:58:40]

lgtm

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.cc (left):

https://codereview.webrtc.org/1929313002/diff/20001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:192: -1);
On 2016/05/03 07:56:52, mflodman wrote:
> On 2016/05/03 07:48:08, stefan-webrtc (holmer) wrote:
> > Where's this happening now?
> 
> See line 113 in rtp_stream_receiver.cc and line 56 in video_stream_decoder.cc.
> This is the most sensitive part of this CL and would be good to get a second
> look at.

Yes, looked at that after and forgot to remove this comment.

[mflodman, 2016-05-03 09:29:58]

The CQ bit was checked by mflodman@webrtc.org

[mflodman, 2016-05-03 09:29:59]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1929313002/#ps40001 (title:
"Removed SetFEC(false) call.")

[commit-bot: I haz the power, 2016-05-03 09:30:09]

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

[commit-bot: I haz the power, 2016-05-03 09:30:58]

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

[commit-bot: I haz the power, 2016-05-03 09:30:59]

Try jobs failed on following builders:
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/7161)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/14701)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/13355)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2846)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/9067)

[mflodman, 2016-05-03 11:14:00]

The CQ bit was checked by mflodman@webrtc.org

[mflodman, 2016-05-03 11:14:01]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1929313002/#ps60001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-05-03 11:14:21]

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

[commit-bot: I haz the power, 2016-05-03 11:16:12]

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

[commit-bot: I haz the power, 2016-05-03 11:16:13]

Try jobs failed on following builders:
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/7183)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/9506)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/9431)
  mac_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/14334)
  mac_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/11061)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/9075)

[mflodman, 2016-05-03 11:30:57]

The CQ bit was checked by mflodman@webrtc.org

[mflodman, 2016-05-03 11:30:58]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1929313002/#ps80001 (title:
" ")

[commit-bot: I haz the power, 2016-05-03 11:31:05]

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

[commit-bot: I haz the power, 2016-05-03 13:31:28]

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

[commit-bot: I haz the power, 2016-05-03 13:31:29]

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

[mflodman, 2016-05-04 04:16:37]

The CQ bit was checked by mflodman@webrtc.org

[commit-bot: I haz the power, 2016-05-04 04:16:47]

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

[commit-bot: I haz the power, 2016-05-04 04:22:08]

Description was changed from

==========
Removed all RTP dependencies from ViEChannel and renamed class.

ViEChannel is now called VideoStreamReceiver.

There will be a follow up CL removing all rtp references from
VideoReceiveStream, but that made this CL to big and it will be done separately.

BUG=webrtc:5079
==========

to

==========
Removed all RTP dependencies from ViEChannel and renamed class.

ViEChannel is now called VideoStreamReceiver.

There will be a follow up CL removing all rtp references from
VideoReceiveStream, but that made this CL to big and it will be done separately.

BUG=webrtc:5079
==========

[commit-bot: I haz the power, 2016-05-04 04:22:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-04 04:22:20]

Description was changed from

==========
Removed all RTP dependencies from ViEChannel and renamed class.

ViEChannel is now called VideoStreamReceiver.

There will be a follow up CL removing all rtp references from
VideoReceiveStream, but that made this CL to big and it will be done separately.

BUG=webrtc:5079
==========

to

==========
Removed all RTP dependencies from ViEChannel and renamed class.

ViEChannel is now called VideoStreamReceiver.

There will be a follow up CL removing all rtp references from
VideoReceiveStream, but that made this CL to big and it will be done separately.

BUG=webrtc:5079

Committed: https://crrev.com/cfc8e3b9ef25eb977e55960c7bac2172298f4d27
Cr-Commit-Position: refs/heads/master@{#12619}
==========

[commit-bot: I haz the power, 2016-05-04 04:22:22]

Patchset 5 (id:??) landed as
https://crrev.com/cfc8e3b9ef25eb977e55960c7bac2172298f4d27
Cr-Commit-Position: refs/heads/master@{#12619}