WebRtcVoiceEngine refactoring - moving codec configuration code into Send/RecvStreams

WebRtcVoiceEngine minor refactoring - moving codec configuration code into Send/RecvStreams.

This change moves most of the code responsible for configuring the voice engine into WebRtcAudioSend/RecvStreams in preparation for the change to enable setting a per-stream bitrate limit.

This should be a pure refactoring with no functionality or API change; functional changes that enable per-stream bitrate limits will come in a separate CL.

BUG=

[skvlad, 2016-04-01 03:24:17]

Description was changed from

==========
WebRtcVoiceEngine refactoring - moving codec configuration code into
Send/RecvStreams

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

to

==========
WebRtcVoiceEngine minor refactoring - moving codec configuration code into
Send/RecvStreams.

This change moves most of the code responsible for configuring the voice engine
into WebRtcAudioSend/RecvStreams in preparation for the change to enable setting
a per-stream bitrate limit.

This should be a pure refactoring with no functionality or API change;
functional changes that enable per-stream bitrate limits will come in a separate
CL.

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

[skvlad, 2016-04-01 03:24:17]

skvlad@webrtc.org changed reviewers:
+ mflodman@webrtc.org, pthatcher@webrtc.org, solenberg@webrtc.org

[skvlad, 2016-04-01 03:24:28]

Patchset #1 (id:1) has been deleted

[skvlad, 2016-04-01 03:26:14]

Please take a look at this refactoring. It's not supposed to introduce any
functionality changes, but should make it easier to enable per-channel bitrate
controls in a later CL.

[the sun, 2016-04-01 08:29:06]

I appreciate that you're taking the time to refactor code! Can you explain a bit
about how this refactoring makes the work you describe simpler?

It looks to me like it would be possible to add/move the calls to
SetSendBitrateInternal() without changing the relationship between WVoE/MC and
the WebRtcAudioReceive/SendStream classes (see inline comment).

https://codereview.webrtc.org/1841083008/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1841083008/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1144: WebRtcVoiceEngine* engine,
WebRtcAudioReceive/SendStream should be thin proxy classes that help WVoMC to
recreate AudioReceive/SendStream if any of the config settings change.

I'd like to break them out into separate source files, once they have matured.
So, the intention is that WebRtcAudioReceive/SendStream should NOT know about
the engine, and I am not convinced it makes sense for them to take on the
responsibilities of WVoMC in the way proposed here.

Going forward, the streams will be configured with live encoder/decoder objects,
and as I see it, the responsibility of creating those will still fall on WVoMC
(although the calls referencing the voe_channel_id will be gone).

[skvlad, 2016-04-01 20:10:18]

On 2016/04/01 08:29:06, the sun wrote:
> I appreciate that you're taking the time to refactor code! Can you explain a
bit
> about how this refactoring makes the work you describe simpler?
> 
> It looks to me like it would be possible to add/move the calls to
> SetSendBitrateInternal() without changing the relationship between WVoE/MC and
> the WebRtcAudioReceive/SendStream classes (see inline comment).
> 
>
https://codereview.webrtc.org/1841083008/diff/20001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvoiceengine.cc (right):
> 
>
https://codereview.webrtc.org/1841083008/diff/20001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvoiceengine.cc:1144: WebRtcVoiceEngine* engine,
> WebRtcAudioReceive/SendStream should be thin proxy classes that help WVoMC to
> recreate AudioReceive/SendStream if any of the config settings change.
> 
> I'd like to break them out into separate source files, once they have matured.
> So, the intention is that WebRtcAudioReceive/SendStream should NOT know about
> the engine, and I am not convinced it makes sense for them to take on the
> responsibilities of WVoMC in the way proposed here.
> 
> Going forward, the streams will be configured with live encoder/decoder
objects,
> and as I see it, the responsibility of creating those will still fall on WVoMC
> (although the calls referencing the voe_channel_id will be gone).

I have the CL that adds per-channel bitrate limits for code review - you can see
how the refactoring in this CL makes that change almost trivial.

If my understanding of your plan is correct, calls that use the voe_channel_id
will still be made from inside WebRtcAudioSendStream? If so - this change does
exactly that, it makes every stream hold on to its own configuration. If your
plan is different, let's discuss it offline to make sure I'm not making changes
that move it in the opposite direction.
Is there a plan to make the audio and video channels follow similar patterns?
From what I saw in WebRtcVideoChannel2 and its respective send/receive streams,
the streams maintain their own configuration, and the channel only
creates/destroys streams and dispatches all other operations to the correct
stream based on SSRC.

[the sun, 2016-04-04 14:36:02]

On 2016/04/01 20:10:18, skvlad wrote:
> On 2016/04/01 08:29:06, the sun wrote:
> > I appreciate that you're taking the time to refactor code! Can you explain a
> bit
> > about how this refactoring makes the work you describe simpler?
> > 
> > It looks to me like it would be possible to add/move the calls to
> > SetSendBitrateInternal() without changing the relationship between WVoE/MC
and
> > the WebRtcAudioReceive/SendStream classes (see inline comment).
> > 
> >
>
https://codereview.webrtc.org/1841083008/diff/20001/webrtc/media/engine/webrt...
> > File webrtc/media/engine/webrtcvoiceengine.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1841083008/diff/20001/webrtc/media/engine/webrt...
> > webrtc/media/engine/webrtcvoiceengine.cc:1144: WebRtcVoiceEngine* engine,
> > WebRtcAudioReceive/SendStream should be thin proxy classes that help WVoMC
to
> > recreate AudioReceive/SendStream if any of the config settings change.
> > 
> > I'd like to break them out into separate source files, once they have
matured.
> > So, the intention is that WebRtcAudioReceive/SendStream should NOT know
about
> > the engine, and I am not convinced it makes sense for them to take on the
> > responsibilities of WVoMC in the way proposed here.
> > 
> > Going forward, the streams will be configured with live encoder/decoder
> objects,
> > and as I see it, the responsibility of creating those will still fall on
WVoMC
> > (although the calls referencing the voe_channel_id will be gone).
> 
> I have the CL that adds per-channel bitrate limits for code review - you can
see
> how the refactoring in this CL makes that change almost trivial.

Ah, thanks for sharing!

> If my understanding of your plan is correct, calls that use the voe_channel_id
> will still be made from inside WebRtcAudioSendStream? If so - this change does
> exactly that, it makes every stream hold on to its own configuration. If your
> plan is different, let's discuss it offline to make sure I'm not making
changes
> that move it in the opposite direction.

That's not entirely correct. The plan is that WebRtcAudioSendStream will be
responsible for keeping the config state and for creating/destroying
AudioSendStreams. Calls to VoE through the legacy APIs and "engine()" should not
be made from here (the voe_channel_id will be remove too - it's a stop gap
solution).

So if it is possible to move on with the bitrate limiting work without changing
that relationship, please make it so instead. I have no problems with storing
the RtpParameters in the stream object though. Let me know if you'd like to
discuss more offline.

> Is there a plan to make the audio and video channels follow similar patterns?
> From what I saw in WebRtcVideoChannel2 and its respective send/receive
streams,
> the streams maintain their own configuration, and the channel only
> creates/destroys streams and dispatches all other operations to the correct
> stream based on SSRC.

Yup, that's basically it.

[skvlad, 2016-04-04 20:52:11]

On 2016/04/04 14:36:02, the sun wrote:
> On 2016/04/01 20:10:18, skvlad wrote:
> > On 2016/04/01 08:29:06, the sun wrote:
> > > I appreciate that you're taking the time to refactor code! Can you explain
a
> > bit
> > > about how this refactoring makes the work you describe simpler?
> > > 
> > > It looks to me like it would be possible to add/move the calls to
> > > SetSendBitrateInternal() without changing the relationship between WVoE/MC
> and
> > > the WebRtcAudioReceive/SendStream classes (see inline comment).
> > > 
> > >
> >
>
https://codereview.webrtc.org/1841083008/diff/20001/webrtc/media/engine/webrt...
> > > File webrtc/media/engine/webrtcvoiceengine.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/1841083008/diff/20001/webrtc/media/engine/webrt...
> > > webrtc/media/engine/webrtcvoiceengine.cc:1144: WebRtcVoiceEngine* engine,
> > > WebRtcAudioReceive/SendStream should be thin proxy classes that help WVoMC
> to
> > > recreate AudioReceive/SendStream if any of the config settings change.
> > > 
> > > I'd like to break them out into separate source files, once they have
> matured.
> > > So, the intention is that WebRtcAudioReceive/SendStream should NOT know
> about
> > > the engine, and I am not convinced it makes sense for them to take on the
> > > responsibilities of WVoMC in the way proposed here.
> > > 
> > > Going forward, the streams will be configured with live encoder/decoder
> > objects,
> > > and as I see it, the responsibility of creating those will still fall on
> WVoMC
> > > (although the calls referencing the voe_channel_id will be gone).
> > 
> > I have the CL that adds per-channel bitrate limits for code review - you can
> see
> > how the refactoring in this CL makes that change almost trivial.
> 
> Ah, thanks for sharing!
> 
> > If my understanding of your plan is correct, calls that use the
voe_channel_id
> > will still be made from inside WebRtcAudioSendStream? If so - this change
does
> > exactly that, it makes every stream hold on to its own configuration. If
your
> > plan is different, let's discuss it offline to make sure I'm not making
> changes
> > that move it in the opposite direction.
> 
> That's not entirely correct. The plan is that WebRtcAudioSendStream will be
> responsible for keeping the config state and for creating/destroying
> AudioSendStreams. Calls to VoE through the legacy APIs and "engine()" should
not
> be made from here (the voe_channel_id will be remove too - it's a stop gap
> solution).
> 
> So if it is possible to move on with the bitrate limiting work without
changing
> that relationship, please make it so instead. I have no problems with storing
> the RtpParameters in the stream object though. Let me know if you'd like to
> discuss more offline.
> 
> > Is there a plan to make the audio and video channels follow similar
patterns?
> > From what I saw in WebRtcVideoChannel2 and its respective send/receive
> streams,
> > the streams maintain their own configuration, and the channel only
> > creates/destroys streams and dispatches all other operations to the correct
> > stream based on SSRC.
> 
> Yup, that's basically it.

I've uploaded a version of the change that keeps the code in the MediaChannel,
https://codereview.webrtc.org/1847353004/. Please take a look and let me know if

this is what you had in mind.