Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1757683002:
Make the audio channel communicate network state changes to the call. (Closed)
|
Created:
4 years, 9 months ago by skvlad Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, zhuangzesen_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMake the audio channel communicate network state changes to the call.
This change enables voice-only calls to keep track of the network state.
This is only a partial fix - the last modality to change state controls
the state for the entire call, so a call with a failed video transport
will also stop sending audio packets. Handling this condition correctly
would require the call to keep track of network state for each media
type separately, and take care of conditions such as a failed video
channel getting removed, while a functioning audio channel remains.
BUG=webrtc:5307
Committed: https://crrev.com/7a43d253f99f3dce7123cdabb7c99a7985dbb021
Cr-Commit-Position: refs/heads/master@{#12093}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Reenabled the workaround to keep sending audio packets when the pacer is stopped so that audio does… #
Total comments: 2
Patch Set 3 : Made the Call class keep track of network state for audio and video separately #
Total comments: 4
Patch Set 4 : Use the presence of send/receive streams to infer which media types are active #
Total comments: 16
Patch Set 5 : Updated with code review feedback #
Total comments: 19
Patch Set 6 : Fixing code review issues #
Total comments: 8
Patch Set 7 : Added end-to-end tests for all network states #
Total comments: 4
Patch Set 8 : Rebased on top of the latest version; listed the possible switch cases explicitly #Patch Set 9 : Added handling for the case where the enum class value is outside of the valid range #
Messages
Total messages: 46 (9 generated)
skvlad@webrtc.org changed reviewers: + mflodman@webrtc.org, pthatcher@webrtc.org, stefan@webrtc.org
This is my first CL so if I'm doing something wrong - please let me know!
pthatcher@webrtc.org changed reviewers: + tina.legrand@webrtc.org
lgtm I added Tina as a reviewer in case she wants to double-check the voice engine parts. But I think this is correct, and we mostly just need to verify that turning back the changes to the SendPacer is OK.
Description was changed from ========== Make the audio channel communicate network state changes to the call. This change enables voice-only calls to keep track of the network state. This is only a partial fix - the last modality to change state controls the state for the entire call, so a call with a failed video transport will also stop sending audio packets. Handling this condition correctly would require the call to keep track of network state for each media type separately, and take care of conditions such as a failed video channel getting removed, while a functioning audio channel remains. BUG=webrtc:5307 ========== to ========== Make the audio channel communicate network state changes to the call. This change enables voice-only calls to keep track of the network state. This is only a partial fix - the last modality to change state controls the state for the entire call, so a call with a failed video transport will also stop sending audio packets. Handling this condition correctly would require the call to keep track of network state for each media type separately, and take care of conditions such as a failed video channel getting removed, while a functioning audio channel remains. BUG=webrtc:5307 ==========
tina.legrand@webrtc.org changed reviewers: + solenberg@webrtc.org - tina.legrand@webrtc.org
On 2016/03/02 00:30:08, pthatcher1 wrote: > lgtm > > I added Tina as a reviewer in case she wants to double-check the voice engine > parts. > > But I think this is correct, and we mostly just need to verify that turning back > the changes to the SendPacer is OK. Thanks Peter! I actually want Fredrik to double check. Adding him, and removing myself.
https://codereview.webrtc.org/1757683002/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/1/webrtc/call/call.cc#newcode556 webrtc/call/call.cc:556: // for each media type separately. Do we think this is a common scenario? In that case maybe it's actually better to still allow audio to be sent even though the network might be down, just to avoid no audio in the special case when only the video connection is down?
lgtm
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
https://codereview.webrtc.org/1757683002/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/1/webrtc/call/call.cc#newcode556 webrtc/call/call.cc:556: // for each media type separately. Does this risk audio/video network states being out of sync? That sounds scary to me.
https://codereview.webrtc.org/1757683002/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/1/webrtc/call/call.cc#newcode556 webrtc/call/call.cc:556: // for each media type separately. On 2016/03/02 12:54:34, pbos-webrtc wrote: > Does this risk audio/video network states being out of sync? That sounds scary > to me. Currently the audio/video MediaChannels each determine their own state based on the information they receive from their respective transport. These can technically be out of sync, and this CL does not change that. What changes, though, is that previously only the state of the VideoChannel would affect the state of the audio and video streams, and of the pacer. After this change, the last state signaled by either the video or the audio stream - whichever happens last - controls these states. This makes the state be correctly set in the case of a call answered with audio only, where the video stream is never started. In other words, the out-of-sync condition could already be happening and this change should not be affecting its frequency. What it adds is the ability to handle audio-only calls in a slightly more correct way. Eliminating any possibility of an out-of-sync state seems to involve a fairly extensive refactoring - this TODO is there to indicate that. https://codereview.webrtc.org/1757683002/diff/1/webrtc/call/call.cc#newcode556 webrtc/call/call.cc:556: // for each media type separately. On 2016/03/02 08:25:59, stefan-webrtc (holmer) wrote: > Do we think this is a common scenario? In that case maybe it's actually better > to still allow audio to be sent even though the network might be down, just to > avoid no audio in the special case when only the video connection is down? This sounds like a good idea - let's keep the special case to allow sending audio until correct state tracking is implemented.
Keeping the workaround to send audio packets even when the pacer is stopped, per Stefan's suggestion.
still lgtm :)
On 2016/03/02 20:27:50, pthatcher1 wrote: > still lgtm :) ditto!
lgtm I would however recommend that you file a bug about tracking the correct state of audio/video connections, and mention the bug id in the code so that we have a reference.
This inconsistent state scares me, and I don't think keeping track of up/down for audio and video separately inside webrtc::Call is too hard. Can you take a look at doing this TODO? https://codereview.webrtc.org/1757683002/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:553: // TODO(skvlad): In the unbundled case the network state might be different Can you add this now? I think it's scary that audio down can bring down video (scarier than sending audio even though audio is down). You can add a MediaType parameter to this method and only make the state disabled if both media types are down. That should cover the bundle case right now without potentially breaking unbundle.
On 2016/03/03 10:18:57, pbos-webrtc wrote: > This inconsistent state scares me, and I don't think keeping track of up/down > for audio and video separately inside webrtc::Call is too hard. Can you take a > look at doing this TODO? > > https://codereview.webrtc.org/1757683002/diff/20001/webrtc/call/call.cc > File webrtc/call/call.cc (right): > > https://codereview.webrtc.org/1757683002/diff/20001/webrtc/call/call.cc#newco... > webrtc/call/call.cc:553: // TODO(skvlad): In the unbundled case the network > state might be different > Can you add this now? I think it's scary that audio down can bring down video > (scarier than sending audio even though audio is down). You can add a MediaType > parameter to this method and only make the state disabled if both media types > are down. That should cover the bundle case right now without potentially > breaking unbundle. This also requires that a bundled transport signals OnReadySend to both audio and video (which I think makes sense).
https://codereview.webrtc.org/1757683002/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:553: // TODO(skvlad): In the unbundled case the network state might be different On 2016/03/03 10:18:57, pbos-webrtc wrote: > Can you add this now? I think it's scary that audio down can bring down video > (scarier than sending audio even though audio is down). You can add a MediaType > parameter to this method and only make the state disabled if both media types > are down. That should cover the bundle case right now without potentially > breaking unbundle. +1, I agree we need to fix this now if we are going to submit this CL.
Made the Call class keep track of the network state for audio and video streams separately. This is the full fix for the issue where only the video network stream state was used to control the paced sender - resulting in no packets going out in a call which was answered with audio only (which was worked around by sending through high priority packets even if the pacer is stopped). The fix makes the call aware of the state and presence of the audio and video media streams. In tests that do not create the media streams, the network is assumed to be always up. Similarly, if a stream has been just created and has not reported its state yet, its connection is assumed to be up. Only if all active streams have reported the network to be down do we stop the pacer; however, we let the send/receive streams know of the state of their respective video streams - so we'll stop the video encoder in the event of video connectivity loss, but keep the audio running.
https://codereview.webrtc.org/1757683002/diff/40001/webrtc/call.h File webrtc/call.h (right): https://codereview.webrtc.org/1757683002/diff/40001/webrtc/call.h#newcode37 webrtc/call.h:37: enum class ChannelNetworkState I don't think you need this enum. The CHANNEL_NOT_PRESENT state is implicit when we don't have streams of a certain kind. You can figure out in Call::SignalChannelNetworkState() what the aggregate state is, by just keeping track of the audio/video network states separately and looking at the stream counts. Note that when adding/removing streams, you will potentially have to resignal the aggregate state to the congestion controller. https://codereview.webrtc.org/1757683002/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:500: receive_stream->SignalNetworkState( For the send streams, the network state is signaled *before* the streams are added to the internal data structures, for the video receive stream, the state is signaled *after*. Unless there is some subtle reason for that I suggest we make it consistent. Also, should the network state should be signaled for AudioReceiveStream? https://codereview.webrtc.org/1757683002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1757683002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:630: webrtc::ChannelNetworkState::CHANNEL_NETWORK_UP); I'd think Call can already figure this out since it knows whether any video/audio streams are available. Why do we need to signal it? https://codereview.webrtc.org/1757683002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:638: call_->SignalChannelNetworkState( Likewise.
Instead of using a separate CHANNEL_NOT_PRESENT state, now relying on the presence of audio/video streams in the call to infer which media types are active.
lgtm I like this new approach. https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call.h File webrtc/call.h (right): https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call.h#newcode137 webrtc/call.h:137: virtual void SignalChannelNetworkState(MediaType media, We're going to need a TODO here to handle the complete unbundled case where the *same* media type has different streams that flow over different networks with different states. But it's a while down the road and we might choose to not support it that well anyway.
Thanks for changing! Looks very nice! Just a few nits. https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:414: send_stream->SignalNetworkState(video_network_state_); Move SignalNetworkState(), logging and UpdateAggregateNetworkState() outside the scope holding the lock (we want to hold the lock for as short duration as possible). Also, make it the order: signal, update, log since that is closer to what you do for the audio streams. https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:478: receive_stream->SignalNetworkState(video_network_state_); Like above: signal, update, log There shouldn't be any reason to have different ordering. https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:562: if (media == MediaType::AUDIO) is MediaType::DATA ok? I would prefer a switch (media) { case MediaType::AUDIO: ... case MediaType::VIDEO: .... default: RTC_NOTREACHED(); break; } https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:605: if ((!haveVideo && !haveAudio) || Hmm, was this a special case for the tests? If so, can the test be modified? Or is there some other reason we should consider the network alway up when we have no streams? https://codereview.webrtc.org/1757683002/diff/60001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/1757683002/diff/60001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtccall.cc:439: ADD_FAILURE() << "SignalChannelNetworkState called with unknown parameter."; nit: indent https://codereview.webrtc.org/1757683002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1757683002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1364: webrtc::MediaType::VIDEO, nit: indent, plus make a single line of the ternary below https://codereview.webrtc.org/1757683002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1757683002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2406: webrtc::MediaType::AUDIO, nit: indent + make the ternary a single line
Updated with code review feedback. https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call.h File webrtc/call.h (right): https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call.h#newcode137 webrtc/call.h:137: virtual void SignalChannelNetworkState(MediaType media, On 2016/03/05 02:21:09, pthatcher1 wrote: > We're going to need a TODO here to handle the complete unbundled case where the > *same* media type has different streams that flow over different networks with > different states. But it's a while down the road and we might choose to not > support it that well anyway. Done. https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:414: send_stream->SignalNetworkState(video_network_state_); On 2016/03/07 14:16:43, the sun wrote: > Move SignalNetworkState(), logging and UpdateAggregateNetworkState() outside the > scope holding the lock (we want to hold the lock for as short duration as > possible). > > Also, make it the order: signal, update, log since that is closer to what you do > for the audio streams. Done. https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:478: receive_stream->SignalNetworkState(video_network_state_); On 2016/03/07 14:16:43, the sun wrote: > Like above: signal, update, log > There shouldn't be any reason to have different ordering. Done. https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:562: if (media == MediaType::AUDIO) On 2016/03/07 14:16:43, the sun wrote: > is MediaType::DATA ok? > I would prefer a > switch (media) { > case MediaType::AUDIO: > ... > case MediaType::VIDEO: > .... > default: > RTC_NOTREACHED(); > break; > } > Done. https://codereview.webrtc.org/1757683002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:605: if ((!haveVideo && !haveAudio) || On 2016/03/07 14:16:43, the sun wrote: > Hmm, was this a special case for the tests? If so, can the test be modified? Or > is there some other reason we should consider the network alway up when we have > no streams? This used to be a special case for the tests in the previous design where the MediaChannels had to report their status - but it's not needed any more. Even in tests that don't create MediaChannels, there are send/receive streams - so state tracking works well even there. I have removed this special case. https://codereview.webrtc.org/1757683002/diff/60001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/1757683002/diff/60001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtccall.cc:439: ADD_FAILURE() << "SignalChannelNetworkState called with unknown parameter."; On 2016/03/07 14:16:43, the sun wrote: > nit: indent Done. https://codereview.webrtc.org/1757683002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1757683002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1364: webrtc::MediaType::VIDEO, On 2016/03/07 14:16:43, the sun wrote: > nit: indent, plus make a single line of the ternary below Done. https://codereview.webrtc.org/1757683002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1757683002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2406: webrtc::MediaType::AUDIO, On 2016/03/07 14:16:43, the sun wrote: > nit: indent + make the ternary a single line Done.
LGTM, just a few more nits (and one non-nit) https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:415: send_stream->SignalNetworkState(video_network_state_); nit: make order the same; signal, updateaggregate.., log (like for CreateVideoReceiveStream below) https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:618: congestion_controller_ -> SignalNetworkState(aggregateState); nit: remove spaces around "->" https://codereview.webrtc.org/1757683002/diff/80001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/1757683002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtccall.cc:281: webrtc::MediaType media) const { nit: indent should be 4 chars https://codereview.webrtc.org/1757683002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtccall.cc:430: webrtc::NetworkState state) { nit: indent should be 4 chars https://codereview.webrtc.org/1757683002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1757683002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2403: { nit: move { to line above (consistent with rest of file) not nit: add thread checker DCHECK: RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:567: default: Remove the default case here and list the ones that shouldn't be used https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:596: bool haveVideo = false; camel case not style compliant https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:600: if (audio_send_ssrcs_.size() > 0) haveAudio = true; if (!audio_send_ssrcs_.empty()) have_audio = true; Same for video, here and below. https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:618: congestion_controller_ -> SignalNetworkState(aggregateState); On 2016/03/07 20:00:34, the sun wrote: > nit: remove spaces around "->" run "git cl format"
Fixed more code review issues. https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:415: send_stream->SignalNetworkState(video_network_state_); On 2016/03/07 20:00:34, the sun wrote: > nit: make order the same; signal, updateaggregate.., log (like for > CreateVideoReceiveStream below) Done. https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:567: default: On 2016/03/08 23:35:40, pbos-webrtc wrote: > Remove the default case here and list the ones that shouldn't be used Done. https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:596: bool haveVideo = false; On 2016/03/08 23:35:40, pbos-webrtc wrote: > camel case not style compliant Done. https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:600: if (audio_send_ssrcs_.size() > 0) haveAudio = true; On 2016/03/08 23:35:40, pbos-webrtc wrote: > if (!audio_send_ssrcs_.empty()) > have_audio = true; > Same for video, here and below. Done. https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:618: congestion_controller_ -> SignalNetworkState(aggregateState); On 2016/03/07 20:00:34, the sun wrote: > nit: remove spaces around "->" Done. https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:618: congestion_controller_ -> SignalNetworkState(aggregateState); On 2016/03/08 23:35:40, pbos-webrtc wrote: > On 2016/03/07 20:00:34, the sun wrote: > > nit: remove spaces around "->" > > run "git cl format" Thanks, didn't know of this! very useful. https://codereview.webrtc.org/1757683002/diff/80001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/1757683002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtccall.cc:281: webrtc::MediaType media) const { On 2016/03/07 20:00:34, the sun wrote: > nit: indent should be 4 chars Done. https://codereview.webrtc.org/1757683002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtccall.cc:430: webrtc::NetworkState state) { On 2016/03/07 20:00:34, the sun wrote: > nit: indent should be 4 chars Done. https://codereview.webrtc.org/1757683002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1757683002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2403: { On 2016/03/07 20:00:34, the sun wrote: > nit: move { to line above (consistent with rest of file) > > not nit: add thread checker DCHECK: > RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); Done.
https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:567: default: On 2016/03/08 23:35:40, pbos-webrtc wrote: > Remove the default case here and list the ones that shouldn't be used The default state is a good idea anyway, to make sure future additions to the enum are properly handled. Without the default: NOT_REACHED(), the code will silently do things we may not want.
On 2016/03/09 09:54:39, the sun wrote: > https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc > File webrtc/call/call.cc (right): > > https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... > webrtc/call/call.cc:567: default: > On 2016/03/08 23:35:40, pbos-webrtc wrote: > > Remove the default case here and list the ones that shouldn't be used > > The default state is a good idea anyway, to make sure future additions to the > enum are properly handled. Without the default: NOT_REACHED(), the code will > silently do things we may not want. If we list all the the possible enum values and there is no default state - the build will break the next time someone adds a value to the enum. I believe this was pbos's intention here - to make sure we don't forget to revisit the switch statement when making further changes. I think I've taken care of all the feedback so far - can someone please take one final look? Thanks!
I'm missing unittests for the new, more complicated, logic in Call https://codereview.webrtc.org/1757683002/diff/100001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:579: kv.second->SignalNetworkState(video_network_state_); Will SignalChannelNetworkState be called for both video and audio when doing BUNDLE? If not, shouldn't we use the aggregate state here? Otherwise video won't pause the encoder when the network goes down, if that is only signaled for audio. https://codereview.webrtc.org/1757683002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:613: NetworkState aggregate_state = kNetworkDown; Maybe NetworkState aggregate_state = (have_video && video_network_state_ == kNetworkUp) || (have_audio && audio_network_state_ == kNetworkUp); https://codereview.webrtc.org/1757683002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:619: LOG(LS_INFO) << "UpdateAggregateNetworkState: aggregateState=" aggregate_state https://codereview.webrtc.org/1757683002/diff/100001/webrtc/video/end_to_end_... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/1757683002/diff/100001/webrtc/video/end_to_end_... webrtc/video/end_to_end_tests.cc:3223: sender_call_->SignalChannelNetworkState(MediaType::VIDEO, kNetworkDown); Maybe we should add a test which verifies the expected behavior for audio too?
On 2016/03/11 04:02:42, skvlad wrote: > On 2016/03/09 09:54:39, the sun wrote: > > https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc > > File webrtc/call/call.cc (right): > > > > > https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... > > webrtc/call/call.cc:567: default: > > On 2016/03/08 23:35:40, pbos-webrtc wrote: > > > Remove the default case here and list the ones that shouldn't be used > > > > The default state is a good idea anyway, to make sure future additions to the > > enum are properly handled. Without the default: NOT_REACHED(), the code will > > silently do things we may not want. > > If we list all the the possible enum values and there is no default state - the > build will break the next time someone adds a value to the enum. I believe this > was pbos's intention here - to make sure we don't forget to revisit the switch > statement when making further changes. Ok, I didn't know we have a build warning enabled for that. Catching at build time is obviously better than catching at runtime so if that is the case, I'm fine with the current code. > > I think I've taken care of all the feedback so far - can someone please take one > final look? Thanks!
On 2016/03/11 08:59:29, the sun wrote: > On 2016/03/11 04:02:42, skvlad wrote: > > On 2016/03/09 09:54:39, the sun wrote: > > > https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc > > > File webrtc/call/call.cc (right): > > > > > > > > > https://codereview.webrtc.org/1757683002/diff/80001/webrtc/call/call.cc#newco... > > > webrtc/call/call.cc:567: default: > > > On 2016/03/08 23:35:40, pbos-webrtc wrote: > > > > Remove the default case here and list the ones that shouldn't be used > > > > > > The default state is a good idea anyway, to make sure future additions to > the > > > enum are properly handled. Without the default: NOT_REACHED(), the code will > > > silently do things we may not want. > > > > If we list all the the possible enum values and there is no default state - > the > > build will break the next time someone adds a value to the enum. I believe > this > > was pbos's intention here - to make sure we don't forget to revisit the switch > > statement when making further changes. > > Ok, I didn't know we have a build warning enabled for that. Catching at build > time is obviously better than catching at runtime so if that is the case, I'm > fine with the current code. > > > > > I think I've taken care of all the feedback so far - can someone please take > one > > final look? Thanks! I have added end-to-end tests that exercise the state changes for both audio and video. Adding targeted unit tests for the code in call.cc needs extensive refactoring. As it stands, the entire Call class is pretty much untestable; its unit tests currently just verify that CreateStream/DestroyStream can run. The minimal change to the class to enable unit-testing would be something like this: 1. define a StreamFactory interface, and make Call use that to construct instances of AudioSendStream etc. instead of new-ing them up; 2. expose some definitions from webrtc::internal::VideoSendStream in webrtc::VideoSendStream; update the Call class to use the public interface instead of the internal one. The classes can't be mocked without that, as the internal classes are declared final. 3. create mocks for the Audio/Video Send/Receive stream classes that observe events, and inject these mocks through a test implementation of the StreamFactory. This is a fairly extensive change that affects the API for the purposes of testing one method; a complete set of end-to-end tests was much easier to implement. Are you ok with having only e2e test coverage for now, until the Call class is refactored to be easier to test?
Added end-to-end tests to verify that video streams are not affected by network changes for audio. https://codereview.webrtc.org/1757683002/diff/100001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1757683002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:579: kv.second->SignalNetworkState(video_network_state_); On 2016/03/11 08:29:41, stefan-webrtc (holmer) wrote: > Will SignalChannelNetworkState be called for both video and audio when doing > BUNDLE? If not, shouldn't we use the aggregate state here? Otherwise video won't > pause the encoder when the network goes down, if that is only signaled for > audio. I believe it will be called for both media types: both BaseChannels (audio and video) will be using the same TransportChannel and will be subscribed to its SignalReadyToSend signal. When the TransportChannel state changes, both of them will notify the call of a state change for the respective media types. https://codereview.webrtc.org/1757683002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:613: NetworkState aggregate_state = kNetworkDown; On 2016/03/11 08:29:41, stefan-webrtc (holmer) wrote: > Maybe > > NetworkState aggregate_state = (have_video && video_network_state_ == > kNetworkUp) || (have_audio && audio_network_state_ == kNetworkUp); aggregate_state is not a bool but an enum; the equivalent code would look like this: NetworkState aggregate_state = ((have_video && video_network_state == kNetworkUp) || (have_audio && audio_network_state == kNetworkUp)) ? kNetworkUp : kNetworkDown; which is not more readable than the if statement in my opinion. https://codereview.webrtc.org/1757683002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:619: LOG(LS_INFO) << "UpdateAggregateNetworkState: aggregateState=" On 2016/03/11 08:29:41, stefan-webrtc (holmer) wrote: > aggregate_state Done. https://codereview.webrtc.org/1757683002/diff/100001/webrtc/video/end_to_end_... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/1757683002/diff/100001/webrtc/video/end_to_end_... webrtc/video/end_to_end_tests.cc:3223: sender_call_->SignalChannelNetworkState(MediaType::VIDEO, kNetworkDown); On 2016/03/11 08:29:41, stefan-webrtc (holmer) wrote: > Maybe we should add a test which verifies the expected behavior for audio too? I've added tests that verify that video streams are not affected by audio network state changes. Until the workaround in paced_sender is removed, audio packets won't be stopped by network state changes so there are no tests for that.
Ping. Please take a look at the latest version of this change. Thanks!
LGTM
lgtm https://codereview.webrtc.org/1757683002/diff/120001/webrtc/media/engine/fake... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/1757683002/diff/120001/webrtc/media/engine/fake... webrtc/media/engine/fakewebrtccall.cc:286: default: I think you should be able to enumerate all enums here so we get compile-time failures if we add new enums that we don't handle yet (remove default:) https://codereview.webrtc.org/1757683002/diff/120001/webrtc/media/engine/fake... webrtc/media/engine/fakewebrtccall.cc:437: default: same here
https://codereview.webrtc.org/1757683002/diff/120001/webrtc/media/engine/fake... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/1757683002/diff/120001/webrtc/media/engine/fake... webrtc/media/engine/fakewebrtccall.cc:286: default: On 2016/03/22 10:17:41, pbos-webrtc wrote: > I think you should be able to enumerate all enums here so we get compile-time > failures if we add new enums that we don't handle yet (remove default:) Done. https://codereview.webrtc.org/1757683002/diff/120001/webrtc/media/engine/fake... webrtc/media/engine/fakewebrtccall.cc:437: default: On 2016/03/22 10:17:41, pbos-webrtc wrote: > same here Done.
lgtm
The CQ bit was checked by skvlad@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, solenberg@webrtc.org, pbos@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/1757683002/#ps160001 (title: "Added handling for the case where the enum class value is outside of the valid range")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1757683002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757683002/160001
Message was sent while issue was closed.
Description was changed from ========== Make the audio channel communicate network state changes to the call. This change enables voice-only calls to keep track of the network state. This is only a partial fix - the last modality to change state controls the state for the entire call, so a call with a failed video transport will also stop sending audio packets. Handling this condition correctly would require the call to keep track of network state for each media type separately, and take care of conditions such as a failed video channel getting removed, while a functioning audio channel remains. BUG=webrtc:5307 ========== to ========== Make the audio channel communicate network state changes to the call. This change enables voice-only calls to keep track of the network state. This is only a partial fix - the last modality to change state controls the state for the entire call, so a call with a failed video transport will also stop sending audio packets. Handling this condition correctly would require the call to keep track of network state for each media type separately, and take care of conditions such as a failed video channel getting removed, while a functioning audio channel remains. BUG=webrtc:5307 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Make the audio channel communicate network state changes to the call. This change enables voice-only calls to keep track of the network state. This is only a partial fix - the last modality to change state controls the state for the entire call, so a call with a failed video transport will also stop sending audio packets. Handling this condition correctly would require the call to keep track of network state for each media type separately, and take care of conditions such as a failed video channel getting removed, while a functioning audio channel remains. BUG=webrtc:5307 ========== to ========== Make the audio channel communicate network state changes to the call. This change enables voice-only calls to keep track of the network state. This is only a partial fix - the last modality to change state controls the state for the entire call, so a call with a failed video transport will also stop sending audio packets. Handling this condition correctly would require the call to keep track of network state for each media type separately, and take care of conditions such as a failed video channel getting removed, while a functioning audio channel remains. BUG=webrtc:5307 Committed: https://crrev.com/7a43d253f99f3dce7123cdabb7c99a7985dbb021 Cr-Commit-Position: refs/heads/master@{#12093} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7a43d253f99f3dce7123cdabb7c99a7985dbb021 Cr-Commit-Position: refs/heads/master@{#12093} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
