Remove PayloadRouter dependency from ViEEncoder

Remove PayloadRouter dependency from ViEEncoder.

BUG=webrtc:5687
Committed: https://crrev.com/bc75d97c32ec8149e9f2e4e03d16933c771f7347
Cr-Commit-Position: refs/heads/master@{#12593}

[perkj_webrtc, 2016-04-21 13:08:42]

Description was changed from

==========
Remove PayLoadRouter dependency from ViEChannel.

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

to

==========
Remove PayLoadRouter dependency from ViEChannel.

BUG=webrtc:5687
==========

[perkj_webrtc, 2016-04-21 13:08:42]

perkj@webrtc.org changed reviewers:
+ pbos@webrtc.org

[perkj_webrtc, 2016-04-21 16:10:35]

Can you please review.

[perkj_webrtc, 2016-04-22 11:14:28]

perkj@webrtc.org changed reviewers:
+ sprang@webrtc.org

[perkj_webrtc, 2016-04-22 11:14:29]

+ sprang for bit rate allocation to the rtp module.

[sprang_webrtc, 2016-04-22 11:38:21]

lgtm with nit
I've a pending change which does conflict, but it'll be a minor change to
resolve it, so go ahead.

https://codereview.webrtc.org/1912653002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/1912653002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:398: sink_->Encoded(encoded_image,
codec_specific_info, fragmentation);
succeess = sink_->Encoded() ?
Or remove success if it shouldn't be used.

[pbos-webrtc, 2016-04-22 15:24:20]

https://codereview.webrtc.org/1912653002/diff/40001/webrtc/video/payload_rout...
File webrtc/video/payload_router.h (right):

https://codereview.webrtc.org/1912653002/diff/40001/webrtc/video/payload_rout...
webrtc/video/payload_router.h:39: void SetSendCodec(const VideoCodec& codec);
Can this take a std::vector<VideoStream> instead? I don't like the VideoCodec
struct in general.

https://codereview.webrtc.org/1912653002/diff/40001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1912653002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:544:
payload_router_.SetSendCodec(encoder_settings->video_codec);
Perhaps the encoder settings' VideoStreams can be added into encoder_settings,
that way payload_router doesn't have to deal with the whole VideoCodec struct?

https://codereview.webrtc.org/1912653002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:605: // |encoded_frame_proxy_| forwards frames
to |config_.post_encode_callback|;
Should encoded_frame_proxy_ also contain payload_router_ and forward to both?

[perkj_webrtc, 2016-04-27 08:00:57]

PTAL

https://codereview.webrtc.org/1912653002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/1912653002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:398: sink_->Encoded(encoded_image,
codec_specific_info, fragmentation);
On 2016/04/22 11:38:20, språng wrote:
> succeess = sink_->Encoded() ?
> Or remove success if it shouldn't be used.

oops. I think it one day should be removed.

https://codereview.webrtc.org/1912653002/diff/40001/webrtc/video/payload_rout...
File webrtc/video/payload_router.h (right):

https://codereview.webrtc.org/1912653002/diff/40001/webrtc/video/payload_rout...
webrtc/video/payload_router.h:39: void SetSendCodec(const VideoCodec& codec);
On 2016/04/22 15:24:20, pbos-webrtc wrote:
> Can this take a std::vector<VideoStream> instead? I don't like the VideoCodec
> struct in general.

ok- but I had to add the streams to the EncoderConfig. We should really try to
create one codec struct shared with libjingle, webrtc and chrome....

https://codereview.webrtc.org/1912653002/diff/40001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1912653002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:605: // |encoded_frame_proxy_| forwards frames
to |config_.post_encode_callback|;
On 2016/04/22 15:24:20, pbos-webrtc wrote:
> Should encoded_frame_proxy_ also contain payload_router_ and forward to both?

EncodedFrameProxy is used on the receive side and created EncodedFrames  that
does not the information we need. So no.

[perkj_webrtc, 2016-04-29 09:53:17]

Description was changed from

==========
Remove PayLoadRouter dependency from ViEChannel.

BUG=webrtc:5687
==========

to

==========
Remove PayLoadRouter dependency from VieEncoder.

BUG=webrtc:5687
==========

[pbos-webrtc, 2016-05-02 00:45:44]

lgtm

https://codereview.webrtc.org/1912653002/diff/60001/webrtc/video/payload_rout...
File webrtc/video/payload_router.cc (right):

https://codereview.webrtc.org/1912653002/diff/60001/webrtc/video/payload_rout...
webrtc/video/payload_router.cc:123: streams_ = streams;
Perhaps this one should also reallocate bitrates, feel free to add a TODO here.

https://codereview.webrtc.org/1912653002/diff/60001/webrtc/video/payload_rout...
webrtc/video/payload_router.cc:172: // Allocate the bandwidth between the
streams.
I think this should be target_bitrate_bps for all streams except max, perhaps
this could go on top of https://codereview.webrtc.org/1913073002/ to have one
allocation function.

I don't think this is a regression though, but please put a TODO and feel free
to follow up. :)

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

Description was changed from

==========
Remove PayLoadRouter dependency from VieEncoder.

BUG=webrtc:5687
==========

to

==========
Remove PayloadRouter dependency from ViEEncoder.

BUG=webrtc:5687
==========

[perkj_webrtc, 2016-05-02 12:33:19]

https://codereview.webrtc.org/1912653002/diff/60001/webrtc/video/payload_rout...
File webrtc/video/payload_router.cc (right):

https://codereview.webrtc.org/1912653002/diff/60001/webrtc/video/payload_rout...
webrtc/video/payload_router.cc:123: streams_ = streams;
On 2016/05/02 00:45:44, pbos-webrtc wrote:
> Perhaps this one should also reallocate bitrates, feel free to add a TODO
here.

Done.

https://codereview.webrtc.org/1912653002/diff/60001/webrtc/video/payload_rout...
webrtc/video/payload_router.cc:172: // Allocate the bandwidth between the
streams.
On 2016/05/02 00:45:44, pbos-webrtc wrote:
> I think this should be target_bitrate_bps for all streams except max, perhaps
> this could go on top of https://codereview.webrtc.org/1913073002/ to have one
> allocation function.
> 
> I don't think this is a regression though, but please put a TODO and feel free
> to follow up. :)

Done.

[perkj_webrtc, 2016-05-02 12:33:23]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-05-02 12:33:24]

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

[commit-bot: I haz the power, 2016-05-02 12:33:26]

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

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

Description was changed from

==========
Remove PayloadRouter dependency from ViEEncoder.

BUG=webrtc:5687
==========

to

==========
Remove PayloadRouter dependency from ViEEncoder.

BUG=webrtc:5687
==========

[commit-bot: I haz the power, 2016-05-02 13:31:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-02 13:31:36]

Description was changed from

==========
Remove PayloadRouter dependency from ViEEncoder.

BUG=webrtc:5687
==========

to

==========
Remove PayloadRouter dependency from ViEEncoder.

BUG=webrtc:5687

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

[commit-bot: I haz the power, 2016-05-02 13:31:37]

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