Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(22)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1722253002: - Clean up unused voice engine DTMF code. (Closed)

Created:
4 years, 10 months ago by the sun
Modified:
4 years, 9 months ago
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, henrika_webrtc, hlundin-webrtc, tterriberry_mozilla.com, audio-team_agora.io, qiang.lu, niklas.enbom, peah-webrtc, minyue-webrtc
Base URL:
https://chromium.googlesource.com/external/webrtc.git@voe_dtmf_1
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

- Clean up unused voice engine DTMF code following removal of VoEDtmf APIs. - Use better types in AudioSendStream::SendTelephoneEvent() and related methods. BUG=webrtc:4690 Committed: https://crrev.com/8886c816582a7c6190c5429222cb8096fca302a6 Cr-Commit-Position: refs/heads/master@{#11927}

Patch Set 1 #

Patch Set 2 : more remove #

Total comments: 6

Patch Set 3 : Address comments #

Patch Set 4 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -329 lines) Patch
M webrtc/audio/audio_send_stream.h View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/audio/audio_send_stream.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/audio/audio_send_stream_unittest.cc View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/audio_send_stream.h View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/media/engine/fakewebrtccall.h View 1 2 3 2 chunks +4 lines, -4 lines 0 comments Download
M webrtc/media/engine/fakewebrtccall.cc View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/media/engine/webrtcvoiceengine.cc View 1 2 3 1 chunk +1 line, -2 lines 0 comments Download
M webrtc/modules/audio_coding/test/TwoWayCommunication.cc View 1 1 chunk +0 lines, -8 lines 0 comments Download
M webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M webrtc/test/mock_voe_channel_proxy.h View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M webrtc/voice_engine/BUILD.gn View 1 2 1 chunk +0 lines, -2 lines 0 comments Download
M webrtc/voice_engine/channel.h View 1 2 5 chunks +3 lines, -20 lines 0 comments Download
M webrtc/voice_engine/channel.cc View 1 2 10 chunks +16 lines, -112 lines 0 comments Download
M webrtc/voice_engine/channel_proxy.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M webrtc/voice_engine/channel_proxy.cc View 1 2 1 chunk +2 lines, -4 lines 0 comments Download
M webrtc/voice_engine/dtmf_inband.h View 1 chunk +3 lines, -0 lines 0 comments Download
D webrtc/voice_engine/dtmf_inband_queue.h View 1 chunk +0 lines, -50 lines 0 comments Download
D webrtc/voice_engine/dtmf_inband_queue.cc View 1 chunk +0 lines, -86 lines 0 comments Download
M webrtc/voice_engine/transmit_mixer.h View 2 chunks +0 lines, -4 lines 0 comments Download
M webrtc/voice_engine/transmit_mixer.cc View 3 chunks +0 lines, -21 lines 0 comments Download
M webrtc/voice_engine/voice_engine.gyp View 1 2 1 chunk +0 lines, -2 lines 0 comments Download

Messages

Total messages: 24 (10 generated)
the sun
4 years, 10 months ago (2016-02-24 10:17:16 UTC) #3
hlundin-webrtc
Nice, but I think our policy is to use general ints instead of the stdint ...
4 years, 10 months ago (2016-02-24 10:33:20 UTC) #4
tlegrand-webrtc
LGTM, after considering Henrik's comments.
4 years, 10 months ago (2016-02-25 10:37:47 UTC) #5
the sun
https://codereview.webrtc.org/1722253002/diff/20001/webrtc/audio/audio_send_stream.h File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/1722253002/diff/20001/webrtc/audio/audio_send_stream.h#newcode43 webrtc/audio/audio_send_stream.h:43: uint16_t duration_ms) override; On 2016/02/24 10:33:20, hlundin-webrtc wrote: > ...
4 years, 9 months ago (2016-03-08 15:20:48 UTC) #6
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1722253002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1722253002/40001
4 years, 9 months ago (2016-03-08 15:30:10 UTC) #8
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) ...
4 years, 9 months ago (2016-03-08 17:30:37 UTC) #10
the sun
On 2016/03/08 17:30:37, commit-bot: I haz the power wrote: > Dry run: Try jobs failed ...
4 years, 9 months ago (2016-03-09 09:57:54 UTC) #11
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1722253002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1722253002/60001
4 years, 9 months ago (2016-03-09 09:58:35 UTC) #13
hlundin-webrtc
Messy rebase made diff hard to read. Here's my rubberstamp LGTM.
4 years, 9 months ago (2016-03-09 10:21:57 UTC) #14
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 9 months ago (2016-03-09 10:58:54 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1722253002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1722253002/60001
4 years, 9 months ago (2016-03-09 11:18:07 UTC) #19
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 9 months ago (2016-03-09 11:32:49 UTC) #21
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/8886c816582a7c6190c5429222cb8096fca302a6 Cr-Commit-Position: refs/heads/master@{#11927}
4 years, 9 months ago (2016-03-09 11:32:59 UTC) #23
the sun
4 years, 9 months ago (2016-03-09 15:31:41 UTC) #24
Message was sent while issue was closed. 
A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.webrtc.org/1776243003/ by solenberg@webrtc.org.

The reason for reverting is: Breaks Chromium FYI bots for Android. E.g.
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Tests%20%...
.

Powered by Google App Engine
This is Rietveld 408576698