Removed LEGACY_BITEXACT from neteq_impl.cc and updated the ACM unit tests.

webrtc/modules/audio_coding/neteq/neteq_impl.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 29 matching lines @@
 #include "webrtc/modules/audio_coding/neteq/packet_buffer.h"
 #include "webrtc/modules/audio_coding/neteq/packet.h"
 #include "webrtc/modules/audio_coding/neteq/payload_splitter.h"
 #include "webrtc/modules/audio_coding/neteq/post_decode_vad.h"
 #include "webrtc/modules/audio_coding/neteq/preemptive_expand.h"
 #include "webrtc/modules/audio_coding/neteq/sync_buffer.h"
 #include "webrtc/modules/audio_coding/neteq/tick_timer.h"
 #include "webrtc/modules/audio_coding/neteq/timestamp_scaler.h"
 #include "webrtc/modules/include/module_common_types.h"
 
-// Modify the code to obtain backwards bit-exactness. Once bit-exactness is no
-// longer required, this #define should be removed (and the code that it
-// enables).
-#define LEGACY_BITEXACT
-
 namespace webrtc {
 
 NetEqImpl::Dependencies::Dependencies(
     const NetEq::Config& config,
     const rtc::scoped_refptr<AudioDecoderFactory>& decoder_factory)
     : tick_timer(new TickTimer),
       buffer_level_filter(new BufferLevelFilter),
       decoder_database(new DecoderDatabase(decoder_factory)),
       delay_peak_detector(new DelayPeakDetector(tick_timer.get())),
       delay_manager(new DelayManager(config.max_packets_in_buffer,
@@ skipping 1073 matching lines @@
     // The only valid reason to get kUndefined is that new_codec_ is set.
     assert(new_codec_);
     if (*play_dtmf && !header) {
       timestamp_ = dtmf_event->timestamp;
     } else {
       if (!header) {
         LOG(LS_ERROR) << "Packet missing where it shouldn't.";
         return -1;
       }
       timestamp_ = header->timestamp;
-      if (*operation == kRfc3389CngNoPacket
-#ifndef LEGACY_BITEXACT
-          // Without this check, it can happen that a non-CNG packet is sent to
-          // the CNG decoder as if it was a SID frame. This is clearly a bug,
-          // but is kept for now to maintain bit-exactness with the test
-          // vectors.
-          && decoder_database_->IsComfortNoise(header->payloadType)
-#endif
-      ) {
+      if (*operation == kRfc3389CngNoPacket &&
+          decoder_database_->IsComfortNoise(header->payloadType)) {
         // Change decision to CNG packet, since we do have a CNG packet, but it
         // was considered too early to use. Now, use it anyway.
         *operation = kRfc3389Cng;
       } else if (*operation != kRfc3389Cng) {

[minyue-webrtc, 2016/07/07 15:22:05]

is "else if (*operation != kRfc3389Cng)" still valid,

Can *operation become undefined?

[ossu, 2016/07/08 09:08:26]

Hmm, this looks weird. If operation is CngNoPacket and we don't have a packet,
we'll automatically change it to kNormal? That seems off. I guess the
IsComfortNoise check should be in its own if-clause. That way, we'll either have
CngNoPacket, Cng or Normal at the end of these if/else-if blocks.

I don't see how operation could become Undefined after passing through here,
though.

[minyue-webrtc, 2016/07/08 12:36:25]

Ah, I see. The original code did not capture all cases either. Therefore
*operation can remain undefined at the end of the function. and then the caller
has an assert.

         *operation = kNormal;
       }
     }
     // Adjust |sync_buffer_| timestamp before setting |end_timestamp| to the
     // new value.
     sync_buffer_->IncreaseEndTimestamp(timestamp_ - end_timestamp);
     end_timestamp = timestamp_;
     new_codec_ = false;
     decision_logic_->SoftReset();
     buffer_level_filter_->Reset();
@@ skipping 194 matching lines @@
       decoder->Reset();
 
     // Reset comfort noise decoder.
     ComfortNoiseDecoder* cng_decoder = decoder_database_->GetActiveCngDecoder();
     if (cng_decoder)
       cng_decoder->Reset();
 
     reset_decoder_ = false;
   }
 
-#ifdef LEGACY_BITEXACT
-  // Due to a bug in old SignalMCU, it could happen that CNG operation was
-  // decided, but a speech packet was provided. The speech packet will be used
-  // to update the comfort noise decoder, as if it was a SID frame, which is
-  // clearly wrong.
-  if (*operation == kRfc3389Cng) {

[minyue-webrtc, 2016/07/07 15:22:05]

is it still possible to run in here?

though it is wrapped in LEGACY_BITEXACT, it seems to mean more than BITEXACT

Seems we never tested "return 0", what would that mean?

[ossu, 2016/07/08 09:08:26]

Hmm, maybe. If we did, we'd call DecodeLoop() below with a NULL decoder (since
CngDecoder != AudioDecoder). But DecodeLoop seems fine with that - it does
checks to basically do nothing if the packet is comfort noise. return_value will
be zero and decoded_length will be untouched. Basically, it will just fall
through everything and return 0 anyways.

-    return 0;
-  }
-#endif
-
   *decoded_length = 0;
   // Update codec-internal PLC state.
   if ((*operation == kMerge) && decoder && decoder->HasDecodePlc()) {
     decoder->DecodePlc(1, &decoded_buffer_[*decoded_length]);
   }
 
   int return_value;
   if (*operation == kCodecInternalCng) {
     RTC_DCHECK(packet_list->empty());
     return_value = DecodeCng(decoder, decoded_length, speech_type);
@@ skipping 374 matching lines @@
   return 0;
 }
 
 int NetEqImpl::DoRfc3389Cng(PacketList* packet_list, bool play_dtmf) {
   if (!packet_list->empty()) {
     // Must have exactly one SID frame at this point.
     assert(packet_list->size() == 1);
     Packet* packet = packet_list->front();
     packet_list->pop_front();
     if (!decoder_database_->IsComfortNoise(packet->header.payloadType)) {
-#ifdef LEGACY_BITEXACT

[minyue-webrtc, 2016/07/07 15:22:05]

is it still possible to run in here?

though it is wrapped in LEGACY_BITEXACT, it seems to mean more than BITEXACT

Seems we never tested "return kOtherError", what would that mean?

[ossu, 2016/07/08 09:08:26]

Well, we do return other types of errors, e.g. kUnknownRtpPayloadType. Maybe the
LOG should be replaced with an RTC_UNREACHED (or RTC_CHECK(false)) and the
return removed. With this change, we really should never reach the point where
we try to generate CNG from a non-CNG packet.

-      // This can happen due to a bug in GetDecision. Change the payload type
-      // to a CNG type, and move on. Note that this means that we are in fact
-      // sending a non-CNG payload to the comfort noise decoder for decoding.
-      // Clearly wrong, but will maintain bit-exactness with legacy.
-      if (fs_hz_ == 8000) {
-        packet->header.payloadType =
-            decoder_database_->GetRtpPayloadType(NetEqDecoder::kDecoderCNGnb);
-      } else if (fs_hz_ == 16000) {
-        packet->header.payloadType =
-            decoder_database_->GetRtpPayloadType(NetEqDecoder::kDecoderCNGwb);
-      } else if (fs_hz_ == 32000) {
-        packet->header.payloadType = decoder_database_->GetRtpPayloadType(
-            NetEqDecoder::kDecoderCNGswb32kHz);
-      } else if (fs_hz_ == 48000) {
-        packet->header.payloadType = decoder_database_->GetRtpPayloadType(
-            NetEqDecoder::kDecoderCNGswb48kHz);
-      }
-      assert(decoder_database_->IsComfortNoise(packet->header.payloadType));
-#else
       LOG(LS_ERROR) << "Trying to decode non-CNG payload as CNG.";
       return kOtherError;
-#endif
     }
     // UpdateParameters() deletes |packet|.
     if (comfort_noise_->UpdateParameters(packet) ==
         ComfortNoise::kInternalError) {
       algorithm_buffer_->Zeros(output_size_samples_);
       return -comfort_noise_->internal_error_code();
     }
   }
   int cn_return = comfort_noise_->Generate(output_size_samples_,
                                            algorithm_buffer_.get());
@@ skipping 354 matching lines @@
   }
 }
 
 void NetEqImpl::CreateDecisionLogic() {
   decision_logic_.reset(DecisionLogic::Create(
       fs_hz_, output_size_samples_, playout_mode_, decoder_database_.get(),
       *packet_buffer_.get(), delay_manager_.get(), buffer_level_filter_.get(),
       tick_timer_.get()));
 }
 }  // namespace webrtc