Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2206223002:
Removed calls to VoE::SetPlayout() from WebRTCVoiceEngine. (Closed)
|
Created:
4 years, 4 months ago by aleloi Modified:
4 years, 4 months ago Reviewers:
the sun CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, tlegrand-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemoved calls to VoE::SetPlayout() from WebRTCVoiceEngine.
This is part of rewriting the ConferenceMixer and OutputMixer.
Calls are instead routed through AudioReceiveStream::Start/Stop.
NOTRY=True
Committed: https://crrev.com/84ef615a5da13ffd3d8ac150485afa94ca3343c9
Cr-Commit-Position: refs/heads/master@{#13636}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Removed playout error unittest. #
Total comments: 12
Patch Set 3 : Removed start/stop playout status code and related tests. #
Total comments: 3
Patch Set 4 : Added most tests back. Changed fake VoE. #
Total comments: 13
Patch Set 5 : Removed unneeded vars from fake channel. Minor changes. #
Total comments: 4
Patch Set 6 : Updated comments in test. #
Messages
Total messages: 32 (15 generated)
The CQ bit was checked by aleloi@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/...
aleloi@webrtc.org changed reviewers: + solenberg@webrtc.org
PTAL :) https://codereview.webrtc.org/2206223002/diff/1/webrtc/audio/audio_receive_st... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2206223002/diff/1/webrtc/audio/audio_receive_st... webrtc/audio/audio_receive_stream.cc:161: } Similar to AudioSendStream::Start/Stop https://codereview.webrtc.org/2206223002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2206223002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1377: The wrapper audio stream tells the real stream to start or stop playout. https://codereview.webrtc.org/2206223002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.h (left): https://codereview.webrtc.org/2206223002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.h:248: bool SetPlayout(int channel, bool playout); Removed private member function that did the communication with VoE.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/16912)
Oops, that a real bug. Investigating!
https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2755: This test sets up a channel for which VoE::StartPlayout(channel_id) always fails. Since calls to VoE go though void AudioReceiveStream::Start/Stop, there is no way to get the return value from VoE to here. I removed the test.
The CQ bit was checked by aleloi@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/...
Suppose the return value from VoE::StartPlayout/StopPlayout doesn't end up here. Then I think we should change the ***Playout methods from bool to void as well, since the return value doesn't contain failure information. Alternatively, we could change AudioReceiveStream::Start/Stop to return the error.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/3813)
https://codereview.webrtc.org/2206223002/diff/20001/webrtc/audio/audio_receiv... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2206223002/diff/20001/webrtc/audio/audio_receiv... webrtc/audio/audio_receive_stream.cc:169: LOG(LS_ERROR) << "AudioReceiveStream::Start failed with error: " << error; ::Stop https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (left): https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2627: engine()->voe()->base()->StopPlayout(channel); Note that the return value for stop is ignored. I suggest you remove the LOG(LS_ERROR) from AudioReceiveStream::Stop(). https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1940: bool WebRtcVoiceMediaChannel::SetPlayout(bool playout) { To me it looks like the return value of this method is never used, so please change it to void all the way up the chain. https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1945: bool WebRtcVoiceMediaChannel::PausePlayout() { Ditto. In addition, PausePlayout() Looks internal to this class, so you can remove it and call ChangePlayout() directly at the call site. https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1949: bool WebRtcVoiceMediaChannel::ResumePlayout() { Same here. https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1953: bool WebRtcVoiceMediaChannel::ChangePlayout(bool playout) { And the return value can be removed here as well, if I'm not mistaken.
https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (left): https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2627: engine()->voe()->base()->StopPlayout(channel); On 2016/08/03 14:50:56, the sun wrote: > Note that the return value for stop is ignored. I suggest you remove the > LOG(LS_ERROR) from AudioReceiveStream::Stop(). Done. https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1940: bool WebRtcVoiceMediaChannel::SetPlayout(bool playout) { On 2016/08/03 14:50:55, the sun wrote: > To me it looks like the return value of this method is never used, so please > change it to void all the way up the chain. Done. https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1945: bool WebRtcVoiceMediaChannel::PausePlayout() { On 2016/08/03 14:50:56, the sun wrote: > Ditto. In addition, PausePlayout() Looks internal to this class, so you can > remove it and call ChangePlayout() directly at the call site. Done. https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1949: bool WebRtcVoiceMediaChannel::ResumePlayout() { On 2016/08/03 14:50:56, the sun wrote: > Same here. Done. https://codereview.webrtc.org/2206223002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1953: bool WebRtcVoiceMediaChannel::ChangePlayout(bool playout) { On 2016/08/03 14:50:56, the sun wrote: > And the return value can be removed here as well, if I'm not mistaken. Done. https://codereview.webrtc.org/2206223002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2206223002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:778: EXPECT_TRUE(voe_.GetPlayout(channel_num)); voe_ is a fake WebRTCVoiceEngine, which is a Voice Engine wrapper. channel_ is a VoiceMediaChannel, which is the class in which we changed Set/Change/Resume/...-Playout. Earlier, SetPlayout was implemented as engine()->voe()->base()->???Playout(channel). Now, the same is supposed to happen, but AudioReceiveStream makes the call to VoE. In the tests, engine() is a fake voice engine. The call goes to AudioReceiveStream, but it has another voice engine, so the test's VoE doesn't know it's supposed to mark the channel as being played. I couldn't figure out a way to fix the tests, so I removed the parts related to playout status of the channels.
https://codereview.webrtc.org/2206223002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2206223002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:778: EXPECT_TRUE(voe_.GetPlayout(channel_num)); On 2016/08/04 09:08:31, aleloi wrote: > voe_ is a fake WebRTCVoiceEngine, which is a Voice Engine wrapper. channel_ is a > VoiceMediaChannel, which is the class in which we changed > Set/Change/Resume/...-Playout. Earlier, SetPlayout was implemented as > engine()->voe()->base()->???Playout(channel). Now, the same is supposed to > happen, but AudioReceiveStream makes the call to VoE. In the tests, engine() is > a fake voice engine. The call goes to AudioReceiveStream, but it has another > voice engine, so the test's VoE doesn't know it's supposed to mark the channel > as being played. > > I couldn't figure out a way to fix the tests, so I removed the parts related to > playout status of the channels. I suggest you instead implement the Start()/Stop() methods on FakeAudioReceiveStream (see fakewebrtccall.h/.cc) to toggle a bool 'started', together with a public 'started()' method to retrieve the bool. Then you can update the tests similar to how some of the test cases here have been updated: instead of testing voe_.GetPlayout(channel_num), you test GetRecvStream(ssrc).started(). Replace 'ssrc' with kSsrc1, etc, as appropriate per test. Usually the test code can be cleaned up a bit; channel_num and calls to GetLastChannel() can be removed. Also, if possible, take the opportunity to remove GetPlayout() from the VoE fake.
I tested this patch in Chrome; it worked. Sound and no crash.
Patchset #4 (id:60001) has been deleted
https://codereview.webrtc.org/2206223002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2206223002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:778: EXPECT_TRUE(voe_.GetPlayout(channel_num)); On 2016/08/04 09:44:44, the sun wrote: > On 2016/08/04 09:08:31, aleloi wrote: > > voe_ is a fake WebRTCVoiceEngine, which is a Voice Engine wrapper. channel_ is > a > > VoiceMediaChannel, which is the class in which we changed > > Set/Change/Resume/...-Playout. Earlier, SetPlayout was implemented as > > engine()->voe()->base()->???Playout(channel). Now, the same is supposed to > > happen, but AudioReceiveStream makes the call to VoE. In the tests, engine() > is > > a fake voice engine. The call goes to AudioReceiveStream, but it has another > > voice engine, so the test's VoE doesn't know it's supposed to mark the channel > > as being played. > > > > I couldn't figure out a way to fix the tests, so I removed the parts related > to > > playout status of the channels. > > I suggest you instead implement the Start()/Stop() methods on > FakeAudioReceiveStream (see fakewebrtccall.h/.cc) to toggle a bool 'started', > together with a public 'started()' method to retrieve the bool. Then you can > update the tests similar to how some of the test cases here have been updated: > instead of testing voe_.GetPlayout(channel_num), you test > GetRecvStream(ssrc).started(). Replace 'ssrc' with kSsrc1, etc, as appropriate > per test. > > Usually the test code can be cleaned up a bit; channel_num and calls to > GetLastChannel() can be removed. Also, if possible, take the opportunity to > remove GetPlayout() from the VoE fake. I did exactly as you suggested. The tests are back. https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2349: EXPECT_FALSE(voe_.GetPlayout(channel_num1)); The first channel has to recv stream. Therefore, GetRecvStream(...).started() would crash.
https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2349: EXPECT_FALSE(voe_.GetPlayout(channel_num1)); On 2016/08/04 11:38:59, aleloi wrote: > The first channel has to recv stream. Therefore, GetRecvStream(...).started() > would crash. *no* recv stream
Nice, just a few more. https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtcvoiceengine.h (right): https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvoiceengine.h:131: bool playout = false; remove me! https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvoiceengine.h:245: WEBRTC_FUNC(StartPlayout, (int channel)) { Remove impl. Make it a WEBRTC_STUB(). https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvoiceengine.h:258: WEBRTC_FUNC(StopPlayout, (int channel)) { Same, make it a WEBRTC_STUB(). https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvoiceengine.h:583: int playout_fail_channel_ = -1; Remove me too! https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2349: EXPECT_FALSE(voe_.GetPlayout(channel_num1)); On 2016/08/04 11:39:30, aleloi wrote: > On 2016/08/04 11:38:59, aleloi wrote: > > The first channel has to recv stream. Therefore, GetRecvStream(...).started() > > would crash. > > *no* recv stream Yup. https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2361: EXPECT_FALSE(GetSendStream(kSsrc1).IsSending()); No reason to remove this line.
https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtcvoiceengine.h (right): https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvoiceengine.h:131: bool playout = false; On 2016/08/04 11:56:29, the sun wrote: > remove me! Done. https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvoiceengine.h:245: WEBRTC_FUNC(StartPlayout, (int channel)) { On 2016/08/04 11:56:29, the sun wrote: > Remove impl. Make it a WEBRTC_STUB(). Done. https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvoiceengine.h:258: WEBRTC_FUNC(StopPlayout, (int channel)) { On 2016/08/04 11:56:29, the sun wrote: > Same, make it a WEBRTC_STUB(). Done. https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvoiceengine.h:583: int playout_fail_channel_ = -1; On 2016/08/04 11:56:29, the sun wrote: > Remove me too! Done. https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2206223002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2361: EXPECT_FALSE(GetSendStream(kSsrc1).IsSending()); On 2016/08/04 11:56:29, the sun wrote: > No reason to remove this line. Acknowledged.
lgtm % nits https://codereview.webrtc.org/2206223002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2206223002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2359: // Restart playout and make sure only recv streams are played out. remove "only" https://codereview.webrtc.org/2206223002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2364: // Now remove the recv streams and verify that the send stream doesn't play. nit: comment is wrong
Thanks! Uploading now. https://codereview.webrtc.org/2206223002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2206223002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2359: // Restart playout and make sure only recv streams are played out. On 2016/08/04 12:22:18, the sun wrote: > remove "only" Done. https://codereview.webrtc.org/2206223002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2364: // Now remove the recv streams and verify that the send stream doesn't play. On 2016/08/04 12:22:18, the sun wrote: > nit: comment is wrong Done.
Description was changed from ========== Removed calls to VoE::SetPlayout() from WebRTCVoiceEngine. This is part of rewriting the ConferenceMixer and OutputMixer. Calls are instead routed through AudioReceiveStream::Start/Stop. ========== to ========== Removed calls to VoE::SetPlayout() from WebRTCVoiceEngine. This is part of rewriting the ConferenceMixer and OutputMixer. Calls are instead routed through AudioReceiveStream::Start/Stop. NOTRY=True ==========
The CQ bit was checked by aleloi@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/2206223002/#ps120001 (title: "Updated comments in test.")
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 ========== Removed calls to VoE::SetPlayout() from WebRTCVoiceEngine. This is part of rewriting the ConferenceMixer and OutputMixer. Calls are instead routed through AudioReceiveStream::Start/Stop. NOTRY=True ========== to ========== Removed calls to VoE::SetPlayout() from WebRTCVoiceEngine. This is part of rewriting the ConferenceMixer and OutputMixer. Calls are instead routed through AudioReceiveStream::Start/Stop. NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Removed calls to VoE::SetPlayout() from WebRTCVoiceEngine. This is part of rewriting the ConferenceMixer and OutputMixer. Calls are instead routed through AudioReceiveStream::Start/Stop. NOTRY=True ========== to ========== Removed calls to VoE::SetPlayout() from WebRTCVoiceEngine. This is part of rewriting the ConferenceMixer and OutputMixer. Calls are instead routed through AudioReceiveStream::Start/Stop. NOTRY=True Committed: https://crrev.com/84ef615a5da13ffd3d8ac150485afa94ca3343c9 Cr-Commit-Position: refs/heads/master@{#13636} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/84ef615a5da13ffd3d8ac150485afa94ca3343c9 Cr-Commit-Position: refs/heads/master@{#13636} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
