Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1604563002:
Add send-side BWE to WebRtcVoiceEngine under a finch experiment. (Closed)
|
Created:
4 years, 11 months ago by stefan-webrtc Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd send-side BWE to WebRtcVoiceEngine under a finch experiment.
This adds negotiation of both transport sequence number and transport
feedback. Only offers transport seq num if the
WebRTC-Audio-SendSideBwe finch experiment is enabled.
TBR=mflodman@webrtc.org
BUG=webrtc:5263
Committed: https://crrev.com/ba4c0e45ffcb9c1871389b07b9be5b515d7841d5
Cr-Commit-Position: refs/heads/master@{#11487}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove combined_audio_video_bwe. #Patch Set 3 : Remove extension filter test which is no longer needed. #
Total comments: 8
Patch Set 4 : Comments addressed. #
Total comments: 2
Patch Set 5 : Remove RecreateAudioReceiveStream(). #
Total comments: 14
Patch Set 6 : Comments addressed. #
Total comments: 4
Patch Set 7 : Rebase. #
Total comments: 13
Patch Set 8 : Comments addressed. #Patch Set 9 : format #Patch Set 10 : Addressed one more comment. #
Messages
Total messages: 44 (10 generated)
stefan@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/1604563002/diff/1/talk/media/webrtc/webrtcvoice... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1604563002/diff/1/talk/media/webrtc/webrtcvoice... talk/media/webrtc/webrtcvoiceengine.cc:1749: } I think this should work, as it is what we are doing for video. WDYT? Note that we have to recreate the _receive_ streams here, and not the send streams, since it's the send parameters of the receive streams we are interested in...
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1604563002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1604563002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Remove combined_audio_video_bwe.
stefan@webrtc.org changed reviewers: + mflodman@webrtc.org
+mflodman (mainly for webrtc/audio_receive_stream.h, but feel free to review everything) I decided to remove the combined_audio_video_bwe option since audio-bwe on the send-side will be enabled when the two extensions are negotiated, which is controlled by the finch experiment and can be force-enabled by modifying the SDP. Because of this there is no need to have an option for controlling this. I will clean up both abs-send-time for audio and the option in a follow-up CL if you agree that this makes sense.
Remove extension filter test which is no longer needed.
ah, I was going to ask about the cleanup you already mention! https://codereview.webrtc.org/1604563002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1604563002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1246: void RecreateAudioReceiveStream() { Why do we need to recreate the stream if no config has changed? https://codereview.webrtc.org/1604563002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1416: it.second->RecreateAudioReceiveStream(); Not needed anymore. At least not in the current form. https://codereview.webrtc.org/1604563002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1669: const AudioCodec* WebRtcVoiceMediaChannel::GetPreferredCodec( IIUC this function and GetRedSendCodec() do not use any of the WVoMC state? Provided that is correct, please make them static and move to the WebRtcVoiceCodecs utility class, above.
Comments addressed.
I accidentally included a rebase in patchset 3. The only real changes were in webrtcvoiceengine.* https://codereview.webrtc.org/1604563002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1604563002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1246: void RecreateAudioReceiveStream() { On 2016/01/21 13:26:24, the sun wrote: > Why do we need to recreate the stream if no config has changed? I don't really know, but apparently tests are breaking if we don't. I.e., if I remove line 1416, a bunch of peerconnection_unittests start failing consistently, for instance: out/Debug/peerconnection_unittests: P2PTestConductor.LocalP2PTestDtlsTransferCallee The reason for the failure is: ../../talk/app/webrtc/test/peerconnectiontestwrapper.cc:228: Failure Value of: CheckForAudio() Actual: false Expected: true which seems to suggest that no audio packets are actually being sent. https://codereview.webrtc.org/1604563002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1416: it.second->RecreateAudioReceiveStream(); On 2016/01/21 13:26:24, the sun wrote: > Not needed anymore. At least not in the current form. See above https://codereview.webrtc.org/1604563002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1669: const AudioCodec* WebRtcVoiceMediaChannel::GetPreferredCodec( On 2016/01/21 13:26:24, the sun wrote: > IIUC this function and GetRedSendCodec() do not use any of the WVoMC state? > Provided that is correct, please make them static and move to the > WebRtcVoiceCodecs utility class, above. Done.
https://codereview.webrtc.org/1604563002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1604563002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1246: void RecreateAudioReceiveStream() { On 2016/01/21 15:09:09, stefan-webrtc (holmer) wrote: > On 2016/01/21 13:26:24, the sun wrote: > > Why do we need to recreate the stream if no config has changed? > > I don't really know, but apparently tests are breaking if we don't. I.e., if I > remove line 1416, a bunch of peerconnection_unittests start failing > consistently, for instance: > out/Debug/peerconnection_unittests: > P2PTestConductor.LocalP2PTestDtlsTransferCallee > > The reason for the failure is: > ../../talk/app/webrtc/test/peerconnectiontestwrapper.cc:228: Failure > Value of: CheckForAudio() > Actual: false > Expected: true > > which seems to suggest that no audio packets are actually being sent. Then there is something with how the stream is being set up which has changed behind the scenes (i.e. not part of the config), and the only thing I can find in ARS ctor meeting that requirement is what is provided by the CongestionController: the CC objects and the RBE. Reconstructing with no config change shouldn't be necessary. Please remove this RecreateAudioReceiveStream method and debug. https://codereview.webrtc.org/1604563002/diff/60001/webrtc/audio/audio_receiv... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/1604563002/diff/60001/webrtc/audio/audio_receiv... webrtc/audio/audio_receive_stream.cc:122: congestion_controller->GetRemoteBitrateEstimator(true); The logic here has changed - is there no need to get the RBE in case !UseSendSideBwe()? Sorry for being lazy; I'll look into more detail later, just wanted to bring it up in case it is connected to the issue with the failing peerconnection_unittests...
https://codereview.webrtc.org/1604563002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1604563002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1246: void RecreateAudioReceiveStream() { On 2016/01/22 10:45:16, the sun wrote: > On 2016/01/21 15:09:09, stefan-webrtc (holmer) wrote: > > On 2016/01/21 13:26:24, the sun wrote: > > > Why do we need to recreate the stream if no config has changed? > > > > I don't really know, but apparently tests are breaking if we don't. I.e., if I > > remove line 1416, a bunch of peerconnection_unittests start failing > > consistently, for instance: > > out/Debug/peerconnection_unittests: > > P2PTestConductor.LocalP2PTestDtlsTransferCallee > > > > The reason for the failure is: > > ../../talk/app/webrtc/test/peerconnectiontestwrapper.cc:228: Failure > > Value of: CheckForAudio() > > Actual: false > > Expected: true > > > > which seems to suggest that no audio packets are actually being sent. > > Then there is something with how the stream is being set up which has changed > behind the scenes (i.e. not part of the config), and the only thing I can find > in ARS ctor meeting that requirement is what is provided by the > CongestionController: the CC objects and the RBE. > > Reconstructing with no config change shouldn't be necessary. Please remove this > RecreateAudioReceiveStream method and debug. You are right, and the problem is that the tests are using the same SSRC for send and receive streams, which isn't handled by the PacketRouter. I will make a patch to handle this in the PacketRouter as it doesn't hurt to be flexible, and fixing the tests doesn't look trivial to me.
Remove RecreateAudioReceiveStream().
I have now removed RecreateAudioReceiveStreams() as requested. This will have to wait on https://codereview.webrtc.org/1628683002/ before being committed, but I think this CL should be able to make progress in parallel. Any more comments on this CL? https://codereview.webrtc.org/1604563002/diff/60001/webrtc/audio/audio_receiv... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/1604563002/diff/60001/webrtc/audio/audio_receiv... webrtc/audio/audio_receive_stream.cc:122: congestion_controller->GetRemoteBitrateEstimator(true); On 2016/01/22 10:45:16, the sun wrote: > The logic here has changed - is there no need to get the RBE in case > !UseSendSideBwe()? Sorry for being lazy; I'll look into more detail later, just > wanted to bring it up in case it is connected to the issue with the failing > peerconnection_unittests... No there is no need for that, it should only be used if UseSendSideBwe(config) returns true.
https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:418: int* red_payload_type) { RTC_DCHECK(voe_codec); RTC_DCHECK(red_payload_type); https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:419: // Set send codec (the first non-telephone-event/CN codec) Bad comment - we don't set the send codec here, we select it https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:443: if (!WebRtcVoiceEngine::ToCodecInst(*found_codec, voe_codec)) { Remove "WebRtcVoiceEngine::" https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1764: // and in that case reconfigure all receive streams. Is this then assuming that because we changed send codec here, the other party did as well? https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:83: explicit WebRtcVoiceEngineTestFake() : WebRtcVoiceEngineTestFake("") {} no explicit https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:1317: recv_parameters.codecs.push_back(kOpusCodec); You could set any codec here right? https://codereview.webrtc.org/1604563002/diff/80001/webrtc/call/bitrate_estim... File webrtc/call/bitrate_estimator_tests.cc (left): https://codereview.webrtc.org/1604563002/diff/80001/webrtc/call/bitrate_estim... webrtc/call/bitrate_estimator_tests.cc:276: TEST_F(BitrateEstimatorTest, ImmediatelySwitchToASTForAudio) { Do you have more cleanup to do if AST is now deprecated?
Comments addressed.
https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:418: int* red_payload_type) { On 2016/01/26 10:26:55, the sun wrote: > RTC_DCHECK(voe_codec); > RTC_DCHECK(red_payload_type); Done. https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:419: // Set send codec (the first non-telephone-event/CN codec) On 2016/01/26 10:26:55, the sun wrote: > Bad comment - we don't set the send codec here, we select it Done. https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:443: if (!WebRtcVoiceEngine::ToCodecInst(*found_codec, voe_codec)) { On 2016/01/26 10:26:55, the sun wrote: > Remove "WebRtcVoiceEngine::" Done. https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1764: // and in that case reconfigure all receive streams. On 2016/01/26 10:26:55, the sun wrote: > Is this then assuming that because we changed send codec here, the other party > did as well? Yes, I think it does. Note that this is on the receive stream, so it only affects feedback parameters configured on that send codec. https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:83: explicit WebRtcVoiceEngineTestFake() : WebRtcVoiceEngineTestFake("") {} On 2016/01/26 10:26:55, the sun wrote: > no explicit Done. https://codereview.webrtc.org/1604563002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:1317: recv_parameters.codecs.push_back(kOpusCodec); On 2016/01/26 10:26:55, the sun wrote: > You could set any codec here right? Yes, changed to kIsacCodec to make that clear. https://codereview.webrtc.org/1604563002/diff/80001/webrtc/call/bitrate_estim... File webrtc/call/bitrate_estimator_tests.cc (left): https://codereview.webrtc.org/1604563002/diff/80001/webrtc/call/bitrate_estim... webrtc/call/bitrate_estimator_tests.cc:276: TEST_F(BitrateEstimatorTest, ImmediatelySwitchToASTForAudio) { On 2016/01/26 10:26:55, the sun wrote: > Do you have more cleanup to do if AST is now deprecated? There is a lot of clean up that can be done, but only for audio at the moment. I'm leaving most of it for a follow-up.
Any other comments here?
I think it looks good, except for the recv stream configuration to rely on send codecs. https://codereview.webrtc.org/1604563002/diff/100001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1604563002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:1779: kv.second->RecreateAudioReceiveStream(transport_cc_enabled_); It seems to me that this should be done in response to changing receive header extensions, it shouldn't really have anything to do with what send codec we're using? But we say we support tseq# extensions, so if we are told to accept receiving those, we can configure the recv streams accodingly. Similarly for sending. https://codereview.webrtc.org/1604563002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2091: !send_codecs_.empty() ? HasTransportCc(send_codecs_[0]) : false; Likewise - we should be able to just look at extension list?
https://codereview.webrtc.org/1604563002/diff/100001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1604563002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:1779: kv.second->RecreateAudioReceiveStream(transport_cc_enabled_); On 2016/01/29 12:35:34, the sun wrote: > It seems to me that this should be done in response to changing receive header > extensions, it shouldn't really have anything to do with what send codec we're > using? > > But we say we support tseq# extensions, so if we are told to accept receiving > those, we can configure the recv streams accodingly. Similarly for sending. Video does it this way, and I was given the following explanation: SetSendParameters() is called both on senders and receivers. For receivers, it corresponds to what feedback they are supposed to _sending_, therefore transport cc feedback should be enabled/disabled from there. This is done in WebRtcVideoChannel2::SetSendParameters(). I could move it to SetSendParameters for audio too, but I think it still needs to be on the SetSendParameters path. https://codereview.webrtc.org/1604563002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2091: !send_codecs_.empty() ? HasTransportCc(send_codecs_[0]) : false; On 2016/01/29 12:35:34, the sun wrote: > Likewise - we should be able to just look at extension list? Same thing here, it's the receiver's send codec that specifies feedback parameters, unfortunately...
Rebase.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1604563002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1604563002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
Ping
Ok, I get it. Just a few more small things. https://codereview.webrtc.org/1604563002/diff/120001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1604563002/diff/120001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine_unittest.cc:1986: if (extension.uri == cricket::kRtpTransportSequenceNumberHeaderExtension) { transport_cc may still be enabled in this case - is that ok? https://codereview.webrtc.org/1604563002/diff/120001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/1604563002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:57: ss << ", "; transport_cc is missing from the printout https://codereview.webrtc.org/1604563002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:171: (header.extension.hasAbsoluteSendTime || you can remove this part of the conditional - remote_bitrate_estimator is always null so we're never routing ast to rbe. https://codereview.webrtc.org/1604563002/diff/120001/webrtc/call/bitrate_esti... File webrtc/call/bitrate_estimator_tests.cc (left): https://codereview.webrtc.org/1604563002/diff/120001/webrtc/call/bitrate_esti... webrtc/call/bitrate_estimator_tests.cc:276: TEST_F(BitrateEstimatorTest, ImmediatelySwitchToASTForAudio) { Since you're not removing AST I suggest you leave this in until you do. https://codereview.webrtc.org/1604563002/diff/120001/webrtc/call/bitrate_esti... webrtc/call/bitrate_estimator_tests.cc:298: TEST_F(BitrateEstimatorTest, SwitchesToASTForAudio) { ditto
Comments addressed.
format
Addressed one more comment.
https://codereview.webrtc.org/1604563002/diff/120001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1604563002/diff/120001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine_unittest.cc:1986: if (extension.uri == cricket::kRtpTransportSequenceNumberHeaderExtension) { On 2016/02/03 15:49:01, the sun wrote: > transport_cc may still be enabled in this case - is that ok? I assume you mean that transport_cc still may be enabled even if transport sequence numbers are not negotiated? This test only verifies that capabilities include the transport sequence number extension. The question is definitely valid if I interpreted it correctly, and I think both would be reasonable implementations, that is, to send transport_cc even though we don't have transport seq num should likely be OK since we may still have transport seq num on other streams. On the other hand maybe it makes more sense to only send transport_cc on those other streams which actually themselves have transport seq nums. The current behavior in audio_receive_stream.cc is to only send it if both are negotiated, so I don't think we need logic to handle that case in webrtcvoiceengine.cc. We'll just notify the send stream about what has been negotiated, and the stream will do what's right. https://codereview.webrtc.org/1604563002/diff/120001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/1604563002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:57: ss << ", "; On 2016/02/03 15:49:01, the sun wrote: > transport_cc is missing from the printout Done. https://codereview.webrtc.org/1604563002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:171: (header.extension.hasAbsoluteSendTime || On 2016/02/03 15:49:01, the sun wrote: > you can remove this part of the conditional - remote_bitrate_estimator is always > null so we're never routing ast to rbe. Done. https://codereview.webrtc.org/1604563002/diff/120001/webrtc/call/bitrate_esti... File webrtc/call/bitrate_estimator_tests.cc (left): https://codereview.webrtc.org/1604563002/diff/120001/webrtc/call/bitrate_esti... webrtc/call/bitrate_estimator_tests.cc:276: TEST_F(BitrateEstimatorTest, ImmediatelySwitchToASTForAudio) { On 2016/02/03 15:49:01, the sun wrote: > Since you're not removing AST I suggest you leave this in until you do. I won't be able to keep this test if I remove support for AST in audio_receive_stream.cc, which I think makes sense if we remove combined_audio_video_bwe from the receive config. I could clean up even more AST in this CL if you really prefer that, or we'll just make sure to do it in a follow-up?
https://codereview.webrtc.org/1604563002/diff/120001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1604563002/diff/120001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine_unittest.cc:1986: if (extension.uri == cricket::kRtpTransportSequenceNumberHeaderExtension) { On 2016/02/04 09:47:38, stefan-webrtc (holmer) wrote: > On 2016/02/03 15:49:01, the sun wrote: > > transport_cc may still be enabled in this case - is that ok? > > I assume you mean that transport_cc still may be enabled even if transport > sequence numbers are not negotiated? This test only verifies that capabilities > include the transport sequence number extension. > > The question is definitely valid if I interpreted it correctly, and I think both > would be reasonable implementations, that is, to send transport_cc even though > we don't have transport seq num should likely be OK since we may still have > transport seq num on other streams. On the other hand maybe it makes more sense > to only send transport_cc on those other streams which actually themselves have > transport seq nums. The current behavior in audio_receive_stream.cc is to only > send it if both are negotiated, so I don't think we need logic to handle that > case in webrtcvoiceengine.cc. We'll just notify the send stream about what has > been negotiated, and the stream will do what's right. I meant to say: "We'll just notify the _receive_ stream about what has been negotiated, and the stream will do what's right."
lgtm https://codereview.webrtc.org/1604563002/diff/120001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1604563002/diff/120001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine_unittest.cc:1986: if (extension.uri == cricket::kRtpTransportSequenceNumberHeaderExtension) { On 2016/02/04 09:48:56, stefan-webrtc (holmer) wrote: > On 2016/02/04 09:47:38, stefan-webrtc (holmer) wrote: > > On 2016/02/03 15:49:01, the sun wrote: > > > transport_cc may still be enabled in this case - is that ok? > > > > I assume you mean that transport_cc still may be enabled even if transport > > sequence numbers are not negotiated? This test only verifies that capabilities > > include the transport sequence number extension. > > > > The question is definitely valid if I interpreted it correctly, and I think > both > > would be reasonable implementations, that is, to send transport_cc even though > > we don't have transport seq num should likely be OK since we may still have > > transport seq num on other streams. On the other hand maybe it makes more > sense > > to only send transport_cc on those other streams which actually themselves > have > > transport seq nums. The current behavior in audio_receive_stream.cc is to only > > send it if both are negotiated, so I don't think we need logic to handle that > > case in webrtcvoiceengine.cc. We'll just notify the send stream about what has > > been negotiated, and the stream will do what's right. > > I meant to say: "We'll just notify the _receive_ stream about what has > been negotiated, and the stream will do what's right." Acknowledged. https://codereview.webrtc.org/1604563002/diff/120001/webrtc/call/bitrate_esti... File webrtc/call/bitrate_estimator_tests.cc (left): https://codereview.webrtc.org/1604563002/diff/120001/webrtc/call/bitrate_esti... webrtc/call/bitrate_estimator_tests.cc:276: TEST_F(BitrateEstimatorTest, ImmediatelySwitchToASTForAudio) { On 2016/02/04 09:47:38, stefan-webrtc (holmer) wrote: > On 2016/02/03 15:49:01, the sun wrote: > > Since you're not removing AST I suggest you leave this in until you do. > > I won't be able to keep this test if I remove support for AST in > audio_receive_stream.cc, which I think makes sense if we remove > combined_audio_video_bwe from the receive config. I could clean up even more AST > in this CL if you really prefer that, or we'll just make sure to do it in a > follow-up? Ah, sorry, didn't notice the "true" argument to the Stream ctor. No more cleanup here - do it in a follow-up. https://codereview.webrtc.org/1604563002/diff/120001/webrtc/call/bitrate_esti... webrtc/call/bitrate_estimator_tests.cc:298: TEST_F(BitrateEstimatorTest, SwitchesToASTForAudio) { On 2016/02/03 15:49:02, the sun wrote: > ditto Acknowledged.
Description was changed from ========== Add send-side BWE to WebRtcVoiceEngine under a finch experiment. This adds negotiation of both transport sequence number and transport feedback. Only offers transport seq num if the WebRTC-Audio-SendSideBwe finch experiment is enabled. BUG=webrtc:5263 ========== to ========== Add send-side BWE to WebRtcVoiceEngine under a finch experiment. This adds negotiation of both transport sequence number and transport feedback. Only offers transport seq num if the WebRTC-Audio-SendSideBwe finch experiment is enabled. TBR=mflodman@webrtc.org BUG=webrtc:5263 ==========
Magnus, please take a look after the fact. I only need your review on webrtc/audio_receiver_stream.h.
The CQ bit was checked by stefan@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1604563002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1604563002/180001
Message was sent while issue was closed.
Description was changed from ========== Add send-side BWE to WebRtcVoiceEngine under a finch experiment. This adds negotiation of both transport sequence number and transport feedback. Only offers transport seq num if the WebRTC-Audio-SendSideBwe finch experiment is enabled. TBR=mflodman@webrtc.org BUG=webrtc:5263 ========== to ========== Add send-side BWE to WebRtcVoiceEngine under a finch experiment. This adds negotiation of both transport sequence number and transport feedback. Only offers transport seq num if the WebRTC-Audio-SendSideBwe finch experiment is enabled. TBR=mflodman@webrtc.org BUG=webrtc:5263 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add send-side BWE to WebRtcVoiceEngine under a finch experiment. This adds negotiation of both transport sequence number and transport feedback. Only offers transport seq num if the WebRTC-Audio-SendSideBwe finch experiment is enabled. TBR=mflodman@webrtc.org BUG=webrtc:5263 ========== to ========== Add send-side BWE to WebRtcVoiceEngine under a finch experiment. This adds negotiation of both transport sequence number and transport feedback. Only offers transport seq num if the WebRTC-Audio-SendSideBwe finch experiment is enabled. TBR=mflodman@webrtc.org BUG=webrtc:5263 Committed: https://crrev.com/ba4c0e45ffcb9c1871389b07b9be5b515d7841d5 Cr-Commit-Position: refs/heads/master@{#11487} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/ba4c0e45ffcb9c1871389b07b9be5b515d7841d5 Cr-Commit-Position: refs/heads/master@{#11487} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
