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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1353803002: Simple cleanups of AudioDecoder and AudioEncoder classes (Closed)

Created:
5 years, 3 months ago by kwiberg-webrtc
Modified:
5 years, 3 months ago
Reviewers:
the sun, hlundin-webrtc
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, tlegrand-webrtc, the sun
Base URL:
https://chromium.googlesource.com/external/webrtc.git@dmove-isac
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Simple cleanups of AudioDecoder and AudioEncoder classes * Make sure they're all final and don't allow copying or assignment. * Get rid of the single-channel PCM decoder classes. * Move some includes from .h to .cc files where possible. BUG=webrtc:4557 Committed: https://crrev.com/8967183bf79322c5cb3cbd99e9b7aa298228d0a2 Cr-Commit-Position: refs/heads/master@{#10021}

Patch Set 1 #

Total comments: 6

Patch Set 2 : review nits #

Patch Set 3 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+74 lines, -97 lines) Patch
M webrtc/modules/audio_coding/codecs/cng/include/audio_encoder_cng.h View 1 chunk +3 lines, -0 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/g711/audio_decoder_pcm.cc View 3 chunks +2 lines, -10 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/g711/include/audio_decoder_pcm.h View 3 chunks +11 lines, -30 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/g711/include/audio_encoder_pcm.h View 2 chunks +3 lines, -0 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/g722/include/audio_decoder_g722.h View 3 chunks +3 lines, -3 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/g722/include/audio_encoder_g722.h View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/ilbc/interface/audio_decoder_ilbc.h View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/audio_coding/codecs/ilbc/interface/audio_encoder_ilbc.h View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/isac/audio_decoder_isac_t.h View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/opus/interface/audio_decoder_opus.h View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/audio_coding/codecs/opus/interface/audio_encoder_opus.h View 2 chunks +3 lines, -2 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc View 2 chunks +5 lines, -11 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/pcm16b/include/audio_decoder_pcm16b.h View 2 chunks +4 lines, -12 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/pcm16b/include/audio_encoder_pcm16b.h View 1 chunk +4 lines, -0 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h View 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/modules/audio_coding/main/acm2/audio_coding_module_unittest_oldapi.cc View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc View 2 chunks +7 lines, -7 lines 0 comments Download
M webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc View 1 8 chunks +16 lines, -16 lines 0 comments Download

Messages

Total messages: 17 (6 generated)
kwiberg-webrtc
🐯
5 years, 3 months ago (2015-09-17 11:05:26 UTC) #2
the sun
https://codereview.webrtc.org/1353803002/diff/1/webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc File webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc (right): https://codereview.webrtc.org/1353803002/diff/1/webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc#newcode30 webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:30: virtual ~MockAudioDecoder() { Die(); } override?
5 years, 3 months ago (2015-09-17 13:43:51 UTC) #4
kwiberg-webrtc
https://codereview.webrtc.org/1353803002/diff/1/webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc File webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc (right): https://codereview.webrtc.org/1353803002/diff/1/webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc#newcode30 webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:30: virtual ~MockAudioDecoder() { Die(); } On 2015/09/17 13:43:51, the ...
5 years, 3 months ago (2015-09-17 13:54:56 UTC) #5
hlundin-webrtc
Two nits, then lgtm. https://codereview.webrtc.org/1353803002/diff/1/webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc File webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc (right): https://codereview.webrtc.org/1353803002/diff/1/webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc#newcode23 webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:23: class MockAudioDecoder : public AudioDecoder ...
5 years, 3 months ago (2015-09-17 14:49:49 UTC) #6
the sun
lgtm
5 years, 3 months ago (2015-09-17 15:19:21 UTC) #7
kwiberg-webrtc
All three nits fixed in patch set #2. https://codereview.webrtc.org/1353803002/diff/1/webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc File webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc (right): https://codereview.webrtc.org/1353803002/diff/1/webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc#newcode23 webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:23: class ...
5 years, 3 months ago (2015-09-21 08:01:27 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1353803002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1353803002/40001
5 years, 3 months ago (2015-09-22 17:34:38 UTC) #11
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on ...
5 years, 3 months ago (2015-09-22 19:34:56 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1353803002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1353803002/40001
5 years, 3 months ago (2015-09-22 19:54:01 UTC) #15
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 3 months ago (2015-09-22 21:06:33 UTC) #16
commit-bot: I haz the power
5 years, 3 months ago (2015-09-22 21:06:39 UTC) #17
Message was sent while issue was closed. 
Patchset 3 (id:??) landed as
https://crrev.com/8967183bf79322c5cb3cbd99e9b7aa298228d0a2
Cr-Commit-Position: refs/heads/master@{#10021}

Powered by Google App Engine
This is Rietveld 408576698