Android implementation of WebAudio audio file decoder

webkit/media/android/audio_decoder_android.cc

Index: webkit/media/android/audio_decoder_android.cc
diff --git a/webkit/media/android/audio_decoder_android.cc b/webkit/media/android/audio_decoder_android.cc
index a04d36eadb6a23a04ade01d8110e15892182b6a7..67e674a779fe250b8e7e82cd6205fc0e22cc9ac4 100644
--- a/webkit/media/android/audio_decoder_android.cc
+++ b/webkit/media/android/audio_decoder_android.cc
@@ -4,14 +4,183 @@
 
 #include "webkit/media/audio_decoder.h"
 
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <unistd.h>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/file_descriptor_posix.h"
 #include "base/logging.h"
+#include "base/shared_memory.h"
+#include "media/base/audio_bus.h"
+#include "media/base/limits.h"
+#include "third_party/WebKit/Source/Platform/chromium/public/WebAudioBus.h"
 
 namespace webkit_media {
 
+class AudioDecoderIO {
+public:

[palmer, 2013/04/10 00:57:53]

STYLE NIT: public and private get indented by 1 space.

+  AudioDecoderIO(const char* data, size_t data_size);
+  ~AudioDecoderIO();
+  bool ShareEncodedToProcess(base::SharedMemoryHandle* handle);
+  bool IsValid() const { return read_fd_ > 0 && write_fd_ > 0; }
+  int read_fd() const { return read_fd_; }
+  int write_fd() const { return write_fd_; }
+
+private:
+  // Shared memory that will hold the encoded audio data.  This is
+  // used by MediaCodec for decoding.
+  base::SharedMemory encoded_shared_memory_;
+
+  // A pipe used to communicate with MediaCodec.  MediaCodec owns
+  // write_fd_ and writes to it.
+  int read_fd_;
+  int write_fd_;
+};
+
+AudioDecoderIO::AudioDecoderIO(const char* data, size_t data_size)
+    : read_fd_(-1),
+      write_fd_(-1) {
+

[palmer, 2013/04/10 00:57:53]

Might make sense to sanity-check |data| and |data_size| here.

[Raymond Toy (Google), 2013/04/10 19:23:36]

Yes, checking |data| is a good idea.  I don't have a good feel for what
|data_size| should be other than it should probably be strictly positive.

[palmer, 2013/04/10 21:32:26]

Yeah, me neither. :) There is an underlying limit for these mappings at 2 GiB,
which is probably unnecessarily large for our purposes. So, |data| not NULL,
|data_size| strictly positive and < 2 GiB.

+  // Create the shared memory and copy our data to it so that
+  // MediaCodec can access it.
+  encoded_shared_memory_.CreateAndMapAnonymous(data_size);
+
+  if (encoded_shared_memory_.memory()) {
+    memcpy(encoded_shared_memory_.memory(), data, data_size);
+
+    // Create a pipe for reading/writing the decoded pcm data
+    int pipefd[2];
+
+    if (!pipe(pipefd)) {
+      // Pipe was created successfully
+      read_fd_ = pipefd[0];
+      write_fd_ = pipefd[1];
+    }
+  }
+}
+
+AudioDecoderIO::~AudioDecoderIO() {
+  // Close the read end of the pipe.  The write end should have been
+  // closed by MediaCodec.
+  if (read_fd_ >= 0) {

[palmer, 2013/04/10 00:57:53]

Note a discrepancy here: |IsValid| checks that read_fd_ is > 0, but we will
close it even if it is == 0. Seems odd.

[Raymond Toy (Google), 2013/04/10 19:23:36]

A bug.  Thanks.  read_fd_ could be 0.

+    DVLOG(0) << "Closing read end of pipe: " << read_fd_;
+    if (close(read_fd_))
+      DVLOG(0) << "Failed to close read end!";

[palmer, 2013/04/10 00:57:53]

As before, maybe it'd be useful to use strerror.

+  }
+}
+
+bool AudioDecoderIO::ShareEncodedToProcess(base::SharedMemoryHandle* handle) {
+  return encoded_shared_memory_.ShareToProcess(
+      base::Process::Current().handle(),
+      handle);
+}
+
+//

[palmer, 2013/04/10 00:57:53]

NIT: No leading //.

+// To decode audio data, we want to use the Android MediaCodec class.
+// But this can't run in a sandboxed process so we need to do the
+// decoding in the browser.  To do this, we create a shared memory

[palmer, 2013/04/10 00:57:53]

It's not so much that we *do* the decoding in the browser, it's that we initiate
the request to MediaCodec in the browser. MediaCodec does the decoding.

(Right?)

+// buffer that holds the audio data.  We send a message to the browser
+// to start the decoder using this buffer and one end of a pipe.  The
+// MediaCodec class will decode the data from the shared memory and
+// write the pcm samples back to us over a pipe.
 bool DecodeAudioFileData(WebKit::WebAudioBus* destination_bus, const char* data,
-                         size_t data_size, double sample_rate) {
-  NOTIMPLEMENTED();
-  return false;
+                         size_t data_size, double sample_rate,
+                         const WebAudioMediaCodecRunner& runner) {
+  AudioDecoderIO audio_decoder(data, data_size);
+
+  if (!audio_decoder.IsValid())
+    return false;
+
+  base::SharedMemoryHandle encoded_data_handle;
+  audio_decoder.ShareEncodedToProcess(&encoded_data_handle);
+  base::FileDescriptor fd(audio_decoder.write_fd(), true);
+
+  DVLOG(0) << "DecodeAudioFileData: Starting MediaCodec";
+
+  // Start MediaCodec processing in the browser which will read from
+  // encoded_data_handle for our shared memory and write the decoded
+  // pcm samples (16-bit integer) to our pipe.
+
+  runner.Run(encoded_data_handle, fd);
+
+  // First, read the number of channels, the sample rate, and the
+  // number of frames and a flag indicating if the file is an
+  // ogg/vorbis file.  This must be coordinated with
+  // WebAudioMediaCodecBridge!
+  //
+  // TODO(rtoy): If we know the number of samples, we can create the
+  // destination bus directly and do the conversion directly to the
+  // bus instead of buffering up everything before saving the data to
+  // the bus.
+
+  int input_fd = audio_decoder.read_fd();
+  long info[4];
+
+  DVLOG(0) << "Reading audio file info from fd " << input_fd;
+  ssize_t nread = read(input_fd, info, sizeof(info));
+  DVLOG(0) << "read:  " << nread << " bytes:\n"
+           << " 0: number of channels = " << info[0] << "\n"
+           << " 1: sample rate        = " << info[1] << "\n"
+           << " 2: number of frames   = " << info[2] << "\n"
+           << " 3: is vorbis          = " << info[3];
+
+  if (nread != sizeof(info))
+    return false;
+
+  int number_of_channels = info[0];

[palmer, 2013/04/10 00:57:53]

Sanity check this --- What would 100 mean, or -1?

+  unsigned long expected_number_of_samples = info[2] * number_of_channels;

[palmer, 2013/04/10 00:57:53]

I would use a single term, either "samples" or "frames", but not both.

+  double file_sample_rate = static_cast<double>(info[1]);
+  bool is_vorbis = static_cast<bool>(info[3]);
+
+  // Sanity checks
+  if (!number_of_channels ||
+      number_of_channels > media::limits::kMaxChannels ||

[palmer, 2013/04/10 00:57:53]

-1 would (incorrectly) pass this test.

+      file_sample_rate < media::limits::kMinSampleRate ||
+      file_sample_rate > media::limits::kMaxSampleRate) {
+    return false;
+  }
+
+  short pipe_data[PIPE_BUF / sizeof(short)];

[palmer, 2013/04/10 00:57:53]

int16_t

+  std::vector<short> decoded_samples;

[palmer, 2013/04/10 00:57:53]

Elsewhere, you use the more exact type, int16_t. That's a good idea; We expect
16-bit samples, so declare that. short is ambiguous.

+
+  // Keep reading from the pipe until it's closed.
+  while ((nread = read(input_fd, pipe_data, sizeof(pipe_data))) > 0) {
+    int nsamples = nread / sizeof(short);

[palmer, 2013/04/10 00:57:53]

My instinct is to use size_t for this and for |k|.

+    decoded_samples.reserve(decoded_samples.size() + nsamples);
+    for (int k = 0; k < nsamples; ++k) {
+      decoded_samples.push_back(pipe_data[k]);
+    }
+  }
+
+  DVLOG(0) << "Total samples read = " << decoded_samples.size();
+
+  if (!is_vorbis &&
+      expected_number_of_samples &&
+      decoded_samples.size() != expected_number_of_samples) {
+    VLOG(0) << "Expected " << expected_number_of_samples
+            << " but received " << decoded_samples.size();
+  }
+
+  // Convert the samples and save them in the audio bus.
+  int number_of_samples = decoded_samples.size();

[palmer, 2013/04/10 00:57:53]

size_t here and on next line

+  int number_of_frames = number_of_samples / number_of_channels;
+
+  destination_bus->initialize(number_of_channels,

[palmer, 2013/04/10 00:57:53]

Since these are now size_ts, you may need to change the interface of
|initialize|. If that is not possible, you should use
base::checked_numeric_cast.

+                              number_of_frames,
+                              file_sample_rate);
+
+  int decoded_frames = 0;
+  for (int m = 0; m < number_of_samples; m += number_of_channels) {

[palmer, 2013/04/10 00:57:53]

size_t throughout

+    for (int k = 0; k < number_of_channels; ++k) {
+      destination_bus->channelData(k)[decoded_frames] =
+          decoded_samples[m + k] / 32768.0;
+    }
+    ++decoded_frames;
+  }
+
+  return true;
 }
 
 }  // namespace webkit_media