Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2405183002:
Moving WebRtcVoiceMediaChannel::SendSetCodec to AudioSendStream. (Closed)
|
Created:
4 years, 2 months ago by minyue-webrtc Modified:
4 years, 2 months ago Reviewers:
the sun CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, tlegrand-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMoving WebRtcVoiceMediaChannel::SendSetCodec to AudioSendStream.
BUG=webrtc:5806, webrtc:4690
Committed: https://crrev.com/7a973447eb147e60f2184bc42f7a878a1e087f0e
Cr-Commit-Position: refs/heads/master@{#14700}
Patch Set 1 #Patch Set 2 : working version #
Total comments: 4
Patch Set 3 : avoid duplication #
Total comments: 2
Patch Set 4 : removing ApplyMaxSendBitrate #
Total comments: 24
Patch Set 5 : fixing unittests #
Total comments: 4
Patch Set 6 : adding logs and addressing some earlier comments. #
Total comments: 9
Patch Set 7 : on fredrik's comments #
Total comments: 11
Patch Set 8 : on Fredrik's comments #Patch Set 9 : adding audio send stream unittests #Patch Set 10 : rebasing #
Total comments: 4
Patch Set 11 : on fredrik's comments #
Total comments: 8
Patch Set 12 : try to solve win_bot #
Total comments: 2
Patch Set 13 : final change #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 44 (22 generated)
Description was changed from ========== Moving WebRtcVoiceMediaChannel::SendSetCodec to AudioSendStream. BUG= ========== to ========== Moving WebRtcVoiceMediaChannel::SendSetCodec to AudioSendStream. BUG= ==========
minyue@webrtc.org changed reviewers: + solenberg@webrtc.org
Hi Fredrik, Please take a look at this. I made it build by copying missing functionality from cricket to audio send stream. We may decide a better way to do it. It is not done yet, since setsendcodec still needs to be moved to create audio send stream. Since I can build now, I will continue with this.
Hi, I have got a working version.
https://codereview.webrtc.org/2405183002/diff/20001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2405183002/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:553: return config_.send_codec_spec.codec_inst.pltype != -1; Think we can do without this function. https://codereview.webrtc.org/2405183002/diff/20001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2405183002/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.h:18: #include "webrtc/api/rtpparameters.h" why?
Hi Fredrik, Thanks for the inspiring talk! Here is an easy way of avoiding copying many things. There can be more final cleaning ups (logging / removing side comments). But I hope the CL is moving on the right track. https://codereview.webrtc.org/2405183002/diff/20001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2405183002/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:553: return config_.send_codec_spec.codec_inst.pltype != -1; On 2016/10/12 15:15:17, the sun wrote: > Think we can do without this function. sure. https://codereview.webrtc.org/2405183002/diff/20001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2405183002/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.h:18: #include "webrtc/api/rtpparameters.h" On 2016/10/12 15:15:17, the sun wrote: > why? No need now. In patch set 1, some moved function kept their signatures and need RtpParameters
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/19126) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/18972)
https://codereview.webrtc.org/2405183002/diff/40001/webrtc/api/call/audio_sen... File webrtc/api/call/audio_send_stream.h (right): https://codereview.webrtc.org/2405183002/diff/40001/webrtc/api/call/audio_sen... webrtc/api/call/audio_send_stream.h:153: struct CodecPref { I think you can do even better by adding to SendCodecSpec: bool is_multi_rate; int max_bitrate_bps; and iterating through the WVoE CodecPrefs list and figure this information out before pushing the new config. It doesn't look like it will affect the logic severly.
before I start fixing unittests, let me be brave and do one more clean up. This is very good now 1. it cleans an old TODO, which, if we do later, will introduce structural change again. 2. it makes this CL significantly cleaner. 3. it allows send rate in config_ to match the codec rate in operation. Much easier to understand and test. https://codereview.webrtc.org/2405183002/diff/40001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2405183002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:374: ApplyMaxSendBitrate(); The only reason to place this here is, as far as I can see, the SetSendCodec inside ApplyMaxSendBitrate can fail. But, SetSendCodec on line 313 can also fail. I think it is totally fine to do clamping on |config_.send_codec_spec.codec_inst.rate| before ctor.
https://codereview.webrtc.org/2405183002/diff/60001/webrtc/api/call/audio_sen... File webrtc/api/call/audio_send_stream.h (right): https://codereview.webrtc.org/2405183002/diff/60001/webrtc/api/call/audio_sen... webrtc/api/call/audio_send_stream.h:98: int max_send_bitrate_bps = 0; is it possible to use the field above - max_bitrate_kbps? what is the difference between these? https://codereview.webrtc.org/2405183002/diff/60001/webrtc/api/call/audio_sen... webrtc/api/call/audio_send_stream.h:123: if (red_payload_type != rhs.red_payload_type) { I don't think this is used anymore https://codereview.webrtc.org/2405183002/diff/60001/webrtc/api/call/audio_sen... webrtc/api/call/audio_send_stream.h:149: int cng_plfreq = -1; expand to cng_payload_freq https://codereview.webrtc.org/2405183002/diff/60001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2405183002/diff/60001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:117: SetSendCodecs(); if (!SetupSendCodec()) { LOG(LS_ERROR) << "Failed to set up send codec state."; } https://codereview.webrtc.org/2405183002/diff/60001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:303: bool AudioSendStream::SetSendCodecs() { Rename to SetupSendCodec() instead. https://codereview.webrtc.org/2405183002/diff/60001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:311: // Set the codec immediately, since SetVADStatus() depends on whether Do we need to check whether the codec_inst is initialized and not go further if not? If codec_inst.pt == -1 codecs->SetSendCodec() will probably fail. Is this a condition to worry about in practise? https://codereview.webrtc.org/2405183002/diff/60001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:422: if (codec->GetSendCodec(channel, current_codec) == 0 && Add a TODO to look into whether this optimization is really needed. Also, remove this function and add the code inline in SetSendCodecs() https://codereview.webrtc.org/2405183002/diff/60001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2405183002/diff/60001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.h:66: bool ApplyMaxSendBitrate(); Leftover? https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1122: max_send_bitrate_bps_(0) { remove line, init at declaration https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1179: void MaybeRecreateAudioSendStream(int bps) { I think you can drop the "Maybe" - the fact that we're not being wasteful should be opaque to the caller. https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1183: if (max_send_bitrate_bps_ == new_max_send_bitrate_bps) {} even for one liners in this file, here and below https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1298: // parameters.encodings[0].max_bitrate_bps could have changed. Thank you for adding that comment! I was just wondering. https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1371: int max_send_bitrate_bps_; = 0 https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1631: // TODO(minyue): The following legacy actions go into Remove comment?
Hi Fredrik, I now managed to fix unittests. All tests are kept, which is good. Your other comments and some final cleaning up will follow. Those are generally easier to solve than what we've gone so far. Please start reviewing these changes. https://codereview.webrtc.org/2405183002/diff/60001/webrtc/api/call/audio_sen... File webrtc/api/call/audio_send_stream.h (right): https://codereview.webrtc.org/2405183002/diff/60001/webrtc/api/call/audio_sen... webrtc/api/call/audio_send_stream.h:98: int max_send_bitrate_bps = 0; On 2016/10/13 13:15:05, the sun wrote: > is it possible to use the field above - max_bitrate_kbps? what is the difference > between these? left over, to remove https://codereview.webrtc.org/2405183002/diff/60001/webrtc/api/call/audio_sen... webrtc/api/call/audio_send_stream.h:123: if (red_payload_type != rhs.red_payload_type) { On 2016/10/13 13:15:05, the sun wrote: > I don't think this is used anymore ok. but, it may be out of the scope of this CL. https://codereview.webrtc.org/2405183002/diff/60001/webrtc/api/call/audio_sen... webrtc/api/call/audio_send_stream.h:149: int cng_plfreq = -1; On 2016/10/13 13:15:05, the sun wrote: > expand to cng_payload_freq ok. but, it may be out of the scope of this CL. https://codereview.webrtc.org/2405183002/diff/60001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2405183002/diff/60001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.h:66: bool ApplyMaxSendBitrate(); On 2016/10/13 13:15:05, the sun wrote: > Leftover? yes, to remove https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1371: int max_send_bitrate_bps_; On 2016/10/13 13:15:05, the sun wrote: > = 0 we now force it be initialized as a ctor argument https://codereview.webrtc.org/2405183002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2405183002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1357: int DecideSendBitrate() const { Needed to split this into DecideSendBitrate and IsMaxSendBitrateValid because we need to decide if a new max_send_bitrate_ or a new rtp_parameter.max_bitrate_bps_ is valid, at various place. https://codereview.webrtc.org/2405183002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1670: return it->second->SetRtpParameters(reduced_params); possibility of returning false for this function is captured by it->second->SetRtpParameters() https://codereview.webrtc.org/2405183002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2419: bool success = true; this is now possible to return false as the old code. But different from the old one, we do not break when on the first failure, which is poor behavior IMO. https://codereview.webrtc.org/2405183002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/2405183002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:241: int max_send_bitrate_bps_ = 0; add back because SetSendBitrate (on all streams) can happen before AddSendStream, which is captured by one of the unittests.
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9198)
Hi Fredrik, In PS6, logs which were removed temporarily are added back. I tried to keep the same wording in the logs. Your earlier comments are also dealt with. It might be easier if you take a look at PS5 first, then compare PS6 with PS5, since I put some more comments on how old codes are modified into new. https://codereview.webrtc.org/2405183002/diff/60001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2405183002/diff/60001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:311: // Set the codec immediately, since SetVADStatus() depends on whether On 2016/10/13 13:15:05, the sun wrote: > Do we need to check whether the codec_inst is initialized and not go further if > not? If codec_inst.pt == -1 codecs->SetSendCodec() will probably fail. Is this a > condition to worry about in practise? SetSendCodec checks pltype and return false if it is <0 or >127, that is what we can rely on. https://codereview.webrtc.org/2405183002/diff/60001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:422: if (codec->GetSendCodec(channel, current_codec) == 0 && On 2016/10/13 13:15:05, the sun wrote: > Add a TODO to look into whether this optimization is really needed. > > Also, remove this function and add the code inline in SetSendCodecs() yes, added TODO and moved the function inline. https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1179: void MaybeRecreateAudioSendStream(int bps) { On 2016/10/13 13:15:05, the sun wrote: > I think you can drop the "Maybe" - the fact that we're not being wasteful should > be opaque to the caller. I have changed this function quite a bit. Please take a look at the new solution. https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1183: if (max_send_bitrate_bps_ == new_max_send_bitrate_bps) On 2016/10/13 13:15:05, the sun wrote: > {} even for one liners in this file, here and below Done. https://codereview.webrtc.org/2405183002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1631: // TODO(minyue): The following legacy actions go into On 2016/10/13 13:15:05, the sun wrote: > Remove comment? Done.
A few comments - I'll take a closer look later! https://codereview.webrtc.org/2405183002/diff/100001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2405183002/diff/100001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:13: #include <algorithm> Why? https://codereview.webrtc.org/2405183002/diff/100001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:39: #define LOG_RTCERR2(func, a1, a2, err) \ Add TODOs that these macros should be removed. https://codereview.webrtc.org/2405183002/diff/100001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2405183002/diff/100001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:142: EXPECT_CALL(voice_engine_, GetSendCodec(_, _)).WillOnce(Return(-1)); Doesn't this mean we'll get an error log line? https://codereview.webrtc.org/2405183002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2405183002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:280: auto codec = call_.GetAudioSendStream(ssrc)->GetConfig().send_codec_spec; Generally, I don't think it's a good idea to hide test expectations in utility functions. They should live right in the test case. Utilities for extracting values though, is a good thing. We already have: GetSendStreamConfig(uint32_t ssrc), which gives you a webrtc::AudioSendStream::Config&. How about you create two more utilities: const webrtc::AudioSendStream::Config::SendCodecSpec& GetSendStreamCodecSpec(uint32_t ssrc) and const webrtc::Codecinst& GetSendStreamCodecInst(uint32_t ssrc) and add the EXPECT_EQ in the test cases instead, e.g.: CheckCodecBitrate(kSsrc1, kDesiredBitrate); becomes EXPECT_EQ(kDesiredBitrate, GetSendStreamCodecInst(kSsrc1).pacsize);
Hi Fredrik, A quick reply on one of your comments. https://codereview.webrtc.org/2405183002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2405183002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:280: auto codec = call_.GetAudioSendStream(ssrc)->GetConfig().send_codec_spec; On 2016/10/17 14:58:22, the sun wrote: > Generally, I don't think it's a good idea to hide test expectations in utility > functions. They should live right in the test case. Utilities for extracting > values though, is a good thing. We already have: > > GetSendStreamConfig(uint32_t ssrc), which gives you a > webrtc::AudioSendStream::Config&. How about you create two more utilities: > > const webrtc::AudioSendStream::Config::SendCodecSpec& > GetSendStreamCodecSpec(uint32_t ssrc) > and > const webrtc::Codecinst& GetSendStreamCodecInst(uint32_t ssrc) > > and add the EXPECT_EQ in the test cases instead, e.g.: > > CheckCodecBitrate(kSsrc1, kDesiredBitrate); > becomes > EXPECT_EQ(kDesiredBitrate, GetSendStreamCodecInst(kSsrc1).pacsize); The reason for these CheckXXX helpers two two folded: 1) with void return, we can ASSERT. If we let them return a value, ASSERT has to be placed outside, then theywill not simplify the code at all. 2) I want to check several things together in one of them: CheckSendCodec
Hi Fredrik, I uploaded a new patch set. It addressed your comments + 1 more finding. https://codereview.webrtc.org/2405183002/diff/100001/webrtc/api/call/audio_se... File webrtc/api/call/audio_send_stream.h (right): https://codereview.webrtc.org/2405183002/diff/100001/webrtc/api/call/audio_se... webrtc/api/call/audio_send_stream.h:90: int cng_payload_type = -1; // pt, or -1 to disable Comfort Noise Generator. I found this to be duplicate of SendCodecSpec::cng_payload_type. So I removed this. https://codereview.webrtc.org/2405183002/diff/100001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2405183002/diff/100001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:13: #include <algorithm> On 2016/10/17 14:58:22, the sun wrote: > Why? Oh, should be removed, I used std::min in one of the earlier PS, it is gone now. https://codereview.webrtc.org/2405183002/diff/100001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:39: #define LOG_RTCERR2(func, a1, a2, err) \ On 2016/10/17 14:58:22, the sun wrote: > Add TODOs that these macros should be removed. Done. https://codereview.webrtc.org/2405183002/diff/100001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2405183002/diff/100001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:142: EXPECT_CALL(voice_engine_, GetSendCodec(_, _)).WillOnce(Return(-1)); On 2016/10/17 14:58:22, the sun wrote: > Doesn't this mean we'll get an error log line? This is only for the first time when GetSendCodec gets called. It means no codec has been set. I add a comment here. This first call is invoked by AudioSendStream::SetupSendCodec, and it does not issue error log for -1. Another thing to add, this behavior has to match the channel id, see new patch set.
Generally looks good, please add the missing test for AudioSendStream. https://codereview.webrtc.org/2405183002/diff/120001/webrtc/api/call/audio_se... File webrtc/api/call/audio_send_stream.h (right): https://codereview.webrtc.org/2405183002/diff/120001/webrtc/api/call/audio_se... webrtc/api/call/audio_send_stream.h:139: int red_payload_type = -1; unused? remove https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1195: call_->DestroyAudioSendStream(stream_); It seems we need a private: utility to recreate the stream, so these public: methods can concentrate on populating the config. https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1300: if (!IsMaxSendBitrateValid(MinPositive( Semantically, I find it really odd that SetRtpParameters() may fail, because the previously configured max_send_bitrate is now considered invalid. Is there some other way we can deconvolute this logic? https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1308: SetMaxSendBitrate(max_send_bitrate_bps_); Take care of return value and DCHECK it https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1311: UpdateSendState(); Already done in SetMaxSendBitrate() https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:273: ASSERT_TRUE(call_.GetAudioSendStream(ssrc) != nullptr); Even if you decide to keep these utility functions, can you please have them call the other utility for fetching the Config instead.
Hi Fredrik, I made a further update. PTAL. Regarding the unittests in audio send stream, I have thought about it. An easy solution is to introduce fakewebrtcvoiceengine in there and delegate some functions in MockVoiceEngine to it. I am not sure it is a good way to go. Anyways, we can discuss it. Additionally, I would like to take it in a follow up CL since this CL has taken longer than expected. https://codereview.webrtc.org/2405183002/diff/120001/webrtc/api/call/audio_se... File webrtc/api/call/audio_send_stream.h (right): https://codereview.webrtc.org/2405183002/diff/120001/webrtc/api/call/audio_se... webrtc/api/call/audio_send_stream.h:139: int red_payload_type = -1; On 2016/10/18 14:29:49, the sun wrote: > unused? remove Done. https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1195: call_->DestroyAudioSendStream(stream_); On 2016/10/18 14:29:50, the sun wrote: > It seems we need a private: utility to recreate the stream, so these public: > methods can concentrate on populating the config. Done. https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1300: if (!IsMaxSendBitrateValid(MinPositive( On 2016/10/18 14:29:49, the sun wrote: > Semantically, I find it really odd that SetRtpParameters() may fail, because the > previously configured max_send_bitrate is now considered invalid. Is there some > other way we can deconvolute this logic? I think it is not too bad to let SetRtpParameters return boolean since the parameter has to be valid to take effect. But it is indeed bad to let it call SetMaxSendBitrate. By adding a common helper function to recreate streams on all possible reasons, this problem now goes away. https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1311: UpdateSendState(); On 2016/10/18 14:29:50, the sun wrote: > Already done in SetMaxSendBitrate() I changed it so that SetRtpParameters js not directed to SetMaxSendBitrate any longer https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2405183002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:273: ASSERT_TRUE(call_.GetAudioSendStream(ssrc) != nullptr); On 2016/10/18 14:29:50, the sun wrote: > Even if you decide to keep these utility functions, can you please have them > call the other utility for fetching the Config instead. I see what "the other function" mean now. I did not notice it since ASSERT_TRUE() + GetAudioSendStream(sssrc)->GetConfig() thingy is used in the old code. Although GetSendStreamConfig(ssrc) may not be fully safe, since it uses EXPECT_TRUE rather than ASSERT_TRUE, it is still good to direct all config require to it, as I did in the new patch set. I also take your suggestion of changing CheckXXX to GetXXX.
now are audio send stream unittests also added. PTAL.
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
The CQ bit was unchecked by minyue@webrtc.org
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/12199)
a few more https://codereview.webrtc.org/2405183002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2405183002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1301: bool IsMaxSendBitrateValid(int bps) const { Is it possible to combine these two functions into a single one which returns e.g. an rtc::Optional<int> for the bitrate? https://codereview.webrtc.org/2405183002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1330: int DecideSendBitrate() const { This should be a static function taking 3 parameters: max_send_bitrate, max_bitrate_bps (from rtp_parameters), and a const CodecInst&. Then there is no ambiguity regarding the call order - the state of these methods doesn't need to be latched in before calling the method. Also, nit: ComputeSendBitrate() https://codereview.webrtc.org/2405183002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1332: rtp_parameters_.encodings[0].max_bitrate_bps); Should there be an RTC_DCHECK(IsMaxSendBitrateValid(bps)) here? https://codereview.webrtc.org/2405183002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1333: const int current_rate = config_.send_codec_spec.codec_inst.rate; codec_rate would be more descriptive
Patchset #11 (id:200001) has been deleted
Hi Fredrik, I addressed your comments. https://codereview.webrtc.org/2405183002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2405183002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:331: const auto& send_codec_spec = config_.send_codec_spec; there is no need to copy https://codereview.webrtc.org/2405183002/diff/220001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2405183002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1381: webrtc::AudioSendStream::Config::SendCodecSpec send_codec_spec_; On renaming current_rate to codec_rate. I figured that the legacy code did not want to maintain "current_rate", it rather try to use the inherit rate in send_codec_spec, so, I added this member variable.
lgtm % comments https://codereview.webrtc.org/2405183002/diff/220001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2405183002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:475: // TODO(bemasc): Fix this so that if SetMaxSendBitrate(50) is followed by Isn't this already fixed with your change? https://codereview.webrtc.org/2405183002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:483: ; remove https://codereview.webrtc.org/2405183002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1183: RTC_CHECK(send_rate); DCHECK - this is not a runtime error, it's a logic error https://codereview.webrtc.org/2405183002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1381: webrtc::AudioSendStream::Config::SendCodecSpec send_codec_spec_; On 2016/10/19 14:39:19, minyue-webrtc wrote: > On renaming current_rate to codec_rate. I figured that the legacy code did not > want to maintain "current_rate", it rather try to use the inherit rate in > send_codec_spec, so, I added this member variable. Acknowledged. https://codereview.webrtc.org/2405183002/diff/240001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2405183002/diff/240001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:349: EXPECT_CALL(*helper.voice_engine(), SetVADStatus(kChannelId, true, _, _)) The voice engine mock is strict - don't you get a bunch of errors for unexpected calls on it?
Thanks for the review! Now some final polishes. I will start CQ to see how bots thinks about it. https://codereview.webrtc.org/2405183002/diff/220001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2405183002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:475: // TODO(bemasc): Fix this so that if SetMaxSendBitrate(50) is followed by On 2016/10/19 19:37:52, the sun wrote: > Isn't this already fixed with your change? I think so, but it seems that this has been fixed even before this change. https://codereview.webrtc.org/2405183002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:483: ; On 2016/10/19 19:37:52, the sun wrote: > remove oh, yes. https://codereview.webrtc.org/2405183002/diff/240001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2405183002/diff/240001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:349: EXPECT_CALL(*helper.voice_engine(), SetVADStatus(kChannelId, true, _, _)) On 2016/10/19 19:37:52, the sun wrote: > The voice engine mock is strict - don't you get a bunch of errors for unexpected > calls on it? no. Since I used G722 and many opus specific calls are not invoked.
Description was changed from ========== Moving WebRtcVoiceMediaChannel::SendSetCodec to AudioSendStream. BUG= ========== to ========== Moving WebRtcVoiceMediaChannel::SendSetCodec to AudioSendStream. BUG=webrtc:5806, webrtc:4690 ==========
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2405183002/#ps250001 (title: "final change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Moving WebRtcVoiceMediaChannel::SendSetCodec to AudioSendStream. BUG=webrtc:5806, webrtc:4690 ========== to ========== Moving WebRtcVoiceMediaChannel::SendSetCodec to AudioSendStream. BUG=webrtc:5806, webrtc:4690 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:250001)
Message was sent while issue was closed.
Description was changed from ========== Moving WebRtcVoiceMediaChannel::SendSetCodec to AudioSendStream. BUG=webrtc:5806, webrtc:4690 ========== to ========== Moving WebRtcVoiceMediaChannel::SendSetCodec to AudioSendStream. BUG=webrtc:5806, webrtc:4690 Committed: https://crrev.com/7a973447eb147e60f2184bc42f7a878a1e087f0e Cr-Commit-Position: refs/heads/master@{#14700} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/7a973447eb147e60f2184bc42f7a878a1e087f0e Cr-Commit-Position: refs/heads/master@{#14700} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
