Cleanup OPUS decoder. Remove extraneous transforms and copies.

media/filters/opus_audio_decoder.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/opus_audio_decoder.h"
 
 #include <cmath>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/location.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/sys_byteorder.h"
 #include "media/base/audio_buffer.h"
 #include "media/base/audio_decoder_config.h"
 #include "media/base/audio_timestamp_helper.h"
 #include "media/base/bind_to_loop.h"
 #include "media/base/buffers.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/demuxer.h"
 #include "media/base/pipeline.h"
 #include "third_party/opus/src/include/opus.h"
 #include "third_party/opus/src/include/opus_multistream.h"
 
 namespace media {
 
 static uint16 ReadLE16(const uint8* data, size_t data_size, int read_offset) {
-  DCHECK(data);
   uint16 value = 0;
   DCHECK_LE(read_offset + sizeof(value), data_size);
   memcpy(&value, data + read_offset, sizeof(value));
   return base::ByteSwapToLE16(value);
 }
 
 static int TimeDeltaToAudioFrames(base::TimeDelta time_delta,
                                   int frame_rate) {
   return std::ceil(time_delta.InSecondsF() * frame_rate);
 }
 
 // The Opus specification is part of IETF RFC 6716:
 // http://tools.ietf.org/html/rfc6716
 
 // Opus uses Vorbis channel mapping, and Vorbis channel mapping specifies
 // mappings for up to 8 channels. This information is part of the Vorbis I
 // Specification:
 // http://www.xiph.org/vorbis/doc/Vorbis_I_spec.html
 static const int kMaxVorbisChannels = 8;
 
-// Opus allows for decode of S16 or float samples. OpusAudioDecoder always uses
-// S16 samples.
-static const int kBitsPerChannel = 16;
-static const int kBytesPerChannel = kBitsPerChannel / 8;
-
 // Maximum packet size used in Xiph's opusdec and FFmpeg's libopusdec.
-static const int kMaxOpusOutputPacketSizeSamples = 960 * 6 * kMaxVorbisChannels;
-static const int kMaxOpusOutputPacketSizeBytes =
-    kMaxOpusOutputPacketSizeSamples * kBytesPerChannel;
+static const int kMaxOpusOutputPacketSizeSamples = 960 * 6;
 
 static void RemapOpusChannelLayout(const uint8* opus_mapping,
                                    int num_channels,
                                    uint8* channel_layout) {
   DCHECK_LE(num_channels, kMaxVorbisChannels);
 
   // Opus uses Vorbis channel layout.
   const int32 num_layouts = kMaxVorbisChannels;
   const int32 num_layout_values = kMaxVorbisChannels;
 
@@ skipping 131 matching lines @@
   int num_coupled;
   uint8 stream_map[kMaxVorbisChannels];
 };
 
 // Returns true when able to successfully parse and store Opus extra data in
 // |extra_data|. Based on opus header parsing code in libopusdec from FFmpeg,
 // and opus_header from Xiph's opus-tools project.
 static bool ParseOpusExtraData(const uint8* data, int data_size,
                                const AudioDecoderConfig& config,
                                OpusExtraData* extra_data) {
-  if (data_size < kOpusExtraDataSize)
+  if (data_size < kOpusExtraDataSize) {
+    DLOG(ERROR) << "Extra data size is too small:" << data_size;
     return false;
+  }
 
   extra_data->channels = *(data + kOpusExtraDataChannelsOffset);
 
   if (extra_data->channels <= 0 || extra_data->channels > kMaxVorbisChannels) {
-    DVLOG(0) << "invalid channel count in extra data: " << extra_data->channels;
+    DLOG(ERROR) << "invalid channel count in extra data: "
+                << extra_data->channels;
     return false;
   }
 
   extra_data->skip_samples =
       ReadLE16(data, data_size, kOpusExtraDataSkipSamplesOffset);
 
   extra_data->channel_mapping = *(data + kOpusExtraDataChannelMappingOffset);
 
   if (!extra_data->channel_mapping) {
     if (extra_data->channels > kMaxChannelsWithDefaultLayout) {
-      DVLOG(0) << "Invalid extra data, missing stream map.";
+      DLOG(ERROR) << "Invalid extra data, missing stream map.";
       return false;
     }
 
     extra_data->num_streams = 1;
     extra_data->num_coupled =
         (ChannelLayoutToChannelCount(config.channel_layout()) > 1) ? 1 : 0;
     return true;
   }
 
   if (data_size < kOpusExtraDataStreamMapOffset + extra_data->channels) {
-    DVLOG(0) << "Invalid stream map; insufficient data for current channel "
-             << "count: " << extra_data->channels;
+    DLOG(ERROR) << "Invalid stream map; insufficient data for current channel "
+                << "count: " << extra_data->channels;
     return false;
   }
 
   extra_data->num_streams = *(data + kOpusExtraDataNumStreamsOffset);
   extra_data->num_coupled = *(data + kOpusExtraDataNumCoupledOffset);
 
   if (extra_data->num_streams + extra_data->num_coupled != extra_data->channels)
     DVLOG(1) << "Inconsistent channel mapping.";
 
   for (int i = 0; i < extra_data->channels; ++i)
     extra_data->stream_map[i] = *(data + kOpusExtraDataStreamMapOffset + i);
   return true;
 }
 
 OpusAudioDecoder::OpusAudioDecoder(
     const scoped_refptr<base::MessageLoopProxy>& message_loop)
     : message_loop_(message_loop),
       weak_factory_(this),
       demuxer_stream_(NULL),
       opus_decoder_(NULL),
       bits_per_channel_(0),
       channel_layout_(CHANNEL_LAYOUT_NONE),
       samples_per_second_(0),
+      sample_format_(kUnknownSampleFormat),
       last_input_timestamp_(kNoTimestamp()),
       frames_to_discard_(0),
       frame_delay_at_start_(0) {
 }
 
 void OpusAudioDecoder::Initialize(
     DemuxerStream* stream,
     const PipelineStatusCB& status_cb,
     const StatisticsCB& statistics_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   PipelineStatusCB initialize_cb = BindToCurrentLoop(status_cb);
 
   if (demuxer_stream_) {
     // TODO(scherkus): initialization currently happens more than once in
     // PipelineIntegrationTest.BasicPlayback.
-    DVLOG(0) << "Initialize has already been called.";
+    DLOG(ERROR) << "Initialize has already been called.";

[acolwell GONE FROM CHROMIUM, 2013/12/11 23:30:02]

This looks like stale code. Please remove.

[DaleCurtis, 2013/12/12 00:03:24]

The whole block? Or just the DLOG?

[acolwell GONE FROM CHROMIUM, 2013/12/12 00:53:14]

The whole block.

     CHECK(false);
   }
 
   weak_this_ = weak_factory_.GetWeakPtr();
   demuxer_stream_ = stream;
 
   if (!ConfigureDecoder()) {
     initialize_cb.Run(DECODER_ERROR_NOT_SUPPORTED);
     return;
   }
@@ skipping 80 matching lines @@
   // input buffer is received.
   if (input->end_of_stream()) {
     base::ResetAndReturn(&read_cb_).Run(kOk, AudioBuffer::CreateEOSBuffer());
     return;
   }
 
   // Make sure we are notified if http://crbug.com/49709 returns.  Issue also
   // occurs with some damaged files.
   if (input->timestamp() == kNoTimestamp() &&
       output_timestamp_helper_->base_timestamp() == kNoTimestamp()) {
-    DVLOG(1) << "Received a buffer without timestamps!";
+    DLOG(ERROR) << "Received a buffer without timestamps!";
     base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL);
     return;
   }
 
   if (last_input_timestamp_ != kNoTimestamp() &&
       input->timestamp() != kNoTimestamp() &&
       input->timestamp() < last_input_timestamp_) {
     base::TimeDelta diff = input->timestamp() - last_input_timestamp_;
-    DVLOG(1) << "Input timestamps are not monotonically increasing! "
-             << " ts " << input->timestamp().InMicroseconds() << " us"
-             << " diff " << diff.InMicroseconds() << " us";
+    DLOG(ERROR) << "Input timestamps are not monotonically increasing! "
+                << " ts " << input->timestamp().InMicroseconds() << " us"
+                << " diff " << diff.InMicroseconds() << " us";
     base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL);
     return;
   }
 
   last_input_timestamp_ = input->timestamp();
 
   scoped_refptr<AudioBuffer> output_buffer;
 
   if (!Decode(input, &output_buffer)) {
     base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL);
     return;
   }
 
   if (output_buffer.get()) {
     // Execute callback to return the decoded audio.
     base::ResetAndReturn(&read_cb_).Run(kOk, output_buffer);
   } else {
     // We exhausted the input data, but it wasn't enough for a frame.  Ask for
     // more data in order to fulfill this read.
     ReadFromDemuxerStream();
   }
 }
 
 bool OpusAudioDecoder::ConfigureDecoder() {
   const AudioDecoderConfig& config = demuxer_stream_->audio_decoder_config();
 
   if (config.codec() != kCodecOpus) {
-    DVLOG(0) << "codec must be kCodecOpus.";
+    DLOG(ERROR) << "codec must be kCodecOpus.";
     return false;
   }
 
   const int channel_count =
       ChannelLayoutToChannelCount(config.channel_layout());
   if (!config.IsValidConfig() || channel_count > kMaxVorbisChannels) {
-    DVLOG(0) << "Invalid or unsupported audio stream -"
-             << " codec: " << config.codec()
-             << " channel count: " << channel_count
-             << " channel layout: " << config.channel_layout()
-             << " bits per channel: " << config.bits_per_channel()
-             << " samples per second: " << config.samples_per_second();
-    return false;
-  }
-
-  if (config.bits_per_channel() != kBitsPerChannel) {
-    DVLOG(0) << "16 bit samples required.";
+    DLOG(ERROR) << "Invalid or unsupported audio stream -"
+                << " codec: " << config.codec()
+                << " channel count: " << channel_count
+                << " channel layout: " << config.channel_layout()
+                << " bits per channel: " << config.bits_per_channel()
+                << " samples per second: " << config.samples_per_second();
     return false;
   }
 
   if (config.is_encrypted()) {
-    DVLOG(0) << "Encrypted audio stream not supported.";
+    DLOG(ERROR) << "Encrypted audio stream not supported.";
     return false;
   }
 
   if (opus_decoder_ &&
       (bits_per_channel_ != config.bits_per_channel() ||
        channel_layout_ != config.channel_layout() ||
        samples_per_second_ != config.samples_per_second())) {
-    DVLOG(1) << "Unsupported config change :";
-    DVLOG(1) << "\tbits_per_channel : " << bits_per_channel_
-             << " -> " << config.bits_per_channel();
-    DVLOG(1) << "\tchannel_layout : " << channel_layout_
-             << " -> " << config.channel_layout();
-    DVLOG(1) << "\tsample_rate : " << samples_per_second_
-             << " -> " << config.samples_per_second();
+    DLOG(ERROR) << "Unsupported config change -"
+                << " bits_per_channel: " << bits_per_channel_
+                << " -> " << config.bits_per_channel()
+                << ", channel_layout: " << channel_layout_
+                << " -> " << config.channel_layout()
+                << ", sample_rate: " << samples_per_second_
+                << " -> " << config.samples_per_second();
     return false;
   }
 
   // Clean up existing decoder if necessary.
   CloseDecoder();
 
-  // Allocate the output buffer if necessary.
-  if (!output_buffer_)
-    output_buffer_.reset(new int16[kMaxOpusOutputPacketSizeSamples]);
-
   // Parse the Opus Extra Data.
   OpusExtraData opus_extra_data;
   if (!ParseOpusExtraData(config.extra_data(), config.extra_data_size(),
                           config,
                           &opus_extra_data))
     return false;
 
-  if (!config.codec_delay().InMicroseconds())
-    return false;
-
   // Convert from seconds to samples.
   timestamp_offset_ = config.codec_delay();
   frame_delay_at_start_ = TimeDeltaToAudioFrames(config.codec_delay(),
                                                  config.samples_per_second());
-  if (frame_delay_at_start_ < 0) {
-    DVLOG(1) << "Invalid file. Incorrect value for codec delay.";
-    return false;
-  }
-  if (frame_delay_at_start_ != opus_extra_data.skip_samples) {
-    DVLOG(1) << "Invalid file. Codec Delay in container does not match the "
-             << "value in Opus Extra Data.";
+  if (timestamp_offset_ <= base::TimeDelta() || frame_delay_at_start_ < 0) {
+    DLOG(ERROR) << "Invalid file. Incorrect value for codec delay: "
+                << config.codec_delay().InMicroseconds();
     return false;
   }
 
-  uint8 channel_mapping[kMaxVorbisChannels];
+  if (frame_delay_at_start_ != opus_extra_data.skip_samples) {
+    DLOG(ERROR) << "Invalid file. Codec Delay in container does not match the "
+                << "value in Opus Extra Data.";
+    return false;
+  }
+
+  uint8 channel_mapping[kMaxVorbisChannels] = {0};

[acolwell GONE FROM CHROMIUM, 2013/12/11 23:30:02]

nit: Why is this change needed? Doesn't it immediately get overwritten by the
next few lines?

[DaleCurtis, 2013/12/12 00:03:24]

Not necessary, but good for sanity. The memcpy below only fills 2 out of the 8
possible channels, it's not immediately obvious that RemapOpusChanneLayout() may
initialize all spots.

   memcpy(&channel_mapping,
          kDefaultOpusChannelLayout,
          kMaxChannelsWithDefaultLayout);
 
   if (channel_count > kMaxChannelsWithDefaultLayout) {
     RemapOpusChannelLayout(opus_extra_data.stream_map,
                            channel_count,
                            channel_mapping);
   }
 
   // Init Opus.
   int status = OPUS_INVALID_STATE;
   opus_decoder_ = opus_multistream_decoder_create(config.samples_per_second(),
                                                   channel_count,
                                                   opus_extra_data.num_streams,
                                                   opus_extra_data.num_coupled,
                                                   channel_mapping,
                                                   &status);
   if (!opus_decoder_ || status != OPUS_OK) {
-    DVLOG(0) << "opus_multistream_decoder_create failed status="
+    DLOG(ERROR) << "opus_multistream_decoder_create failed status="
              << opus_strerror(status);
     return false;
   }
 
-  bits_per_channel_ = config.bits_per_channel();
+  // Android uses a fixed point build of the opus decoder.
+#if defined(OS_ANDROID)

[acolwell GONE FROM CHROMIUM, 2013/12/11 23:30:02]

nit: Any reason not to move these changes to the constructor since they don't
change during the life of the object?

[DaleCurtis, 2013/12/12 00:03:24]

Done.

+  sample_format_ = kSampleFormatS16;
+#else
+  sample_format_ = kSampleFormatF32;
+#endif
+
+  bits_per_channel_ = SampleFormatToBytesPerChannel(sample_format_) * 8;
   channel_layout_ = config.channel_layout();
   samples_per_second_ = config.samples_per_second();
   output_timestamp_helper_.reset(
       new AudioTimestampHelper(config.samples_per_second()));
   return true;
 }
 
 void OpusAudioDecoder::CloseDecoder() {
   if (opus_decoder_) {
     opus_multistream_decoder_destroy(opus_decoder_);
     opus_decoder_ = NULL;
   }
 }
 
 void OpusAudioDecoder::ResetTimestampState() {
   output_timestamp_helper_->SetBaseTimestamp(kNoTimestamp());
   last_input_timestamp_ = kNoTimestamp();
   frames_to_discard_ = TimeDeltaToAudioFrames(
       demuxer_stream_->audio_decoder_config().seek_preroll(),
       samples_per_second_);
 }
 
 bool OpusAudioDecoder::Decode(const scoped_refptr<DecoderBuffer>& input,
                               scoped_refptr<AudioBuffer>* output_buffer) {
-  int frames_decoded = opus_multistream_decode(opus_decoder_,
-                                               input->data(),
-                                               input->data_size(),
-                                               &output_buffer_[0],
-                                               kMaxOpusOutputPacketSizeSamples,
-                                               0);
+  // Allocate a buffer for the output samples.
+  *output_buffer = AudioBuffer::CreateBuffer(
+      sample_format_,
+      ChannelLayoutToChannelCount(channel_layout_),
+      kMaxOpusOutputPacketSizeSamples);
+  const int buffer_size =
+      output_buffer->get()->channel_count() *
+      output_buffer->get()->frame_count() *
+      SampleFormatToBytesPerChannel(sample_format_);
+
+  // Android uses a fixed point build of the opus decoder.
+#if defined(OS_ANDROID)

[acolwell GONE FROM CHROMIUM, 2013/12/11 23:30:02]

I don't believe we actually even use this code on Android. I'd prefer to defer
the #ifdefs until the code is actually used on that platform and we've verified
the perf difference is worth the #ifdef.

[DaleCurtis, 2013/12/12 00:03:24]

Hmm, vignesh said it works on Android? Vignesh?

+  int16* int16_output_buffer = reinterpret_cast<int16*>(
+      output_buffer->get()->channel_data()[0]);
+  int frames_decoded =
+      opus_multistream_decode(opus_decoder_,
+                              input->data(),
+                              input->data_size(),
+                              int16_output_buffer,
+                              buffer_size,
+                              0);
+#else
+  float* float_output_buffer = reinterpret_cast<float*>(
+      output_buffer->get()->channel_data()[0]);
+  int frames_decoded =
+      opus_multistream_decode_float(opus_decoder_,
+                                    input->data(),
+                                    input->data_size(),
+                                    float_output_buffer,
+                                    buffer_size,
+                                    0);
+#endif
+
   if (frames_decoded < 0) {
-    DVLOG(0) << "opus_multistream_decode failed for"
-             << " timestamp: " << input->timestamp().InMicroseconds()
-             << " us, duration: " << input->duration().InMicroseconds()
-             << " us, packet size: " << input->data_size() << " bytes with"
-             << " status: " << opus_strerror(frames_decoded);
+    DLOG(ERROR) << "opus_multistream_decode failed for"
+                << " timestamp: " << input->timestamp().InMicroseconds()
+                << " us, duration: " << input->duration().InMicroseconds()
+                << " us, packet size: " << input->data_size() << " bytes with"
+                << " status: " << opus_strerror(frames_decoded);
     return false;
   }
 
-  uint8* decoded_audio_data = reinterpret_cast<uint8*>(&output_buffer_[0]);
-  int bytes_decoded = frames_decoded *
-      demuxer_stream_->audio_decoder_config().bytes_per_frame();
-  DCHECK_LE(bytes_decoded, kMaxOpusOutputPacketSizeBytes);
-
-  if (output_timestamp_helper_->base_timestamp() == kNoTimestamp() &&
-      !input->end_of_stream()) {
-    DCHECK(input->timestamp() != kNoTimestamp());
-    output_timestamp_helper_->SetBaseTimestamp(input->timestamp());
-  }
+  // Trim off any extraneous allocation.
+  DCHECK_LE(frames_decoded, output_buffer->get()->frame_count());
+  const int trim_frames = output_buffer->get()->frame_count() - frames_decoded;
+  if (trim_frames > 0)
+    output_buffer->get()->TrimEnd(trim_frames);
 
   // Skip samples should be equal to codec delay when the file starts and when
   // there is a seek to zero.
   // TODO(vigneshv): This should be checked for start of stream rather than
   // input timestamp of zero to accomodate streams that don't start at zero.
   if (input->timestamp() == base::TimeDelta())
     frames_to_discard_ = frame_delay_at_start_;
 
-  if (bytes_decoded > 0 && frames_decoded > frames_to_discard_) {
-    // Copy the audio samples into an output buffer.
-    uint8* data[] = { decoded_audio_data };
-    *output_buffer = AudioBuffer::CopyFrom(
-        kSampleFormatS16,
-        ChannelLayoutToChannelCount(channel_layout_),
-        frames_decoded,
-        data,
-        output_timestamp_helper_->GetTimestamp() - timestamp_offset_,
-        output_timestamp_helper_->GetFrameDuration(frames_decoded));
-    output_timestamp_helper_->AddFrames(frames_decoded);
+  // Handle frame discard and trimming.
+  if (frames_decoded > 0) {
     if (frames_to_discard_ > 0) {

[acolwell GONE FROM CHROMIUM, 2013/12/11 23:30:02]

This doesn't look right to me. It assumes that frames_decoded is >
frame_to_discard_. frames_to_discard_ can span multiple input buffers.

[DaleCurtis, 2013/12/12 00:03:24]

Good eye, during testing I realized it was broken. Fixed.

       output_buffer->get()->TrimStart(frames_to_discard_);
       frames_decoded -= frames_to_discard_;
       frames_to_discard_ = 0;
     }
     if (input->discard_padding().InMicroseconds() > 0) {
       int discard_padding = TimeDeltaToAudioFrames(input->discard_padding(),
                                                    samples_per_second_);
       if (discard_padding < 0 || discard_padding > frames_decoded) {
         DVLOG(1) << "Invalid file. Incorrect discard padding value.";
         return false;
       }
       output_buffer->get()->TrimEnd(discard_padding);
       frames_decoded -= discard_padding;
     }
-  } else if (bytes_decoded > 0) {
-    frames_to_discard_ -= frames_decoded;
-    frames_decoded = 0;
   }
 
   // Decoding finished successfully, update statistics.
   PipelineStatistics statistics;
   statistics.audio_bytes_decoded = input->data_size();
   statistics_cb_.Run(statistics);
 
   // Discard the buffer to indicate we need more data.
-  if (!frames_decoded)
+  if (!frames_decoded) {
     *output_buffer = NULL;
+    return true;
+  }
+
+  if (output_timestamp_helper_->base_timestamp() == kNoTimestamp() &&

[acolwell GONE FROM CHROMIUM, 2013/12/11 23:30:02]

This also does not look right to me. This assumes that the first samples to be
rendered are actually at the beginning of the compressed frame. I don't believe
Opus requires that. I think the timestamp helper should be set to the first
input buffer like the old code did. I also think that AddFrames() should be
called for all frames output from the decoder, even the dropped ones. That way
the "output_timestamp_helper_->GetTimestamp() - timestamp_offset_" computation
will be correct even if the codec_delay doesn't happen to land on an input
buffer boundary.

For example say I have the following situation: (Assuming 1Hz so frames and
timestamps are the same units)
coded_delay = 100 frames -> timestamp_offset = 100, frames_to_discard_ = 100
input buffer 1 : timestamp 0, decoded_frames = 50 -> 
  * 0 frames output
  * output_timestamp_helper_->GetTimestamp() = 50
  * frames_to_discard_ = 50
input buffer 2 : timestamp 50, decoded_frames = 75 ->
  * 25 frames output
  * output_timestamp_helper_->GetTimestamp() = 100 (since it is only counting
dropped frames at this point)
  * frames_to_discard_ = 0 
  * output_timestamp_helper_->GetTimestamp() - timestamp_offset_ = 0 (which is
correct for the beginning of the output samples).

[DaleCurtis, 2013/12/12 00:03:24]

Done.  Note: I still have some hackish detection for setting the startup
preskip. :-/

+      !input->end_of_stream()) {
+    DCHECK(input->timestamp() != kNoTimestamp());
+    output_timestamp_helper_->SetBaseTimestamp(input->timestamp());
+  }
+
+  output_buffer->get()->set_timestamp(
+      output_timestamp_helper_->GetTimestamp() - timestamp_offset_);
+  output_buffer->get()->set_duration(
+      output_timestamp_helper_->GetFrameDuration(frames_decoded));
+  output_timestamp_helper_->AddFrames(frames_decoded);
 
   return true;
 }
 
 }  // namespace media