|
|
Created:
4 years, 8 months ago by skvlad Modified:
4 years, 8 months ago Reviewers:
the sun, pthatcher1, mflodman CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, ossu, kwiberg-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionWebRtcVoiceEngine 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=
Patch Set 1 #
Total comments: 1
Created: 4 years, 8 months ago
Messages
Total messages: 8 (3 generated)
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@webrtc.org changed reviewers: + mflodman@webrtc.org, pthatcher@webrtc.org, solenberg@webrtc.org
Patchset #1 (id:1) has been deleted
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.
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).
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.
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.
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. |