Support (E)AC3 passthrough

media/base/audio_buffer_queue.cc

Index: media/base/audio_buffer_queue.cc
diff --git a/media/base/audio_buffer_queue.cc b/media/base/audio_buffer_queue.cc
index dfa655a3be33498c11f125d12ff32d68626cacc0..090ad23c725ac7a02adf3ecf808a854d1b216acb 100644
--- a/media/base/audio_buffer_queue.cc
+++ b/media/base/audio_buffer_queue.cc
@@ -35,7 +35,8 @@ void AudioBufferQueue::Append(const scoped_refptr<AudioBuffer>& buffer_in) {
 int AudioBufferQueue::ReadFrames(int frames,
                                  int dest_frame_offset,
                                  AudioBus* dest) {
-  DCHECK_GE(dest->frames(), frames + dest_frame_offset);
+  if (!dest->is_bitstream_format())

[DaleCurtis, 2017/06/15 21:46:32]

Add corresponding dcheck for bitstream formats.

[AndyWu, 2017/08/02 01:43:40]

Done.

+    DCHECK_GE(dest->frames(), frames + dest_frame_offset);
   return InternalRead(frames, true, 0, dest_frame_offset, dest);
 }
 
@@ -60,6 +61,35 @@ int AudioBufferQueue::InternalRead(int frames,
                                    int source_frame_offset,

[DaleCurtis, 2017/06/15 21:46:32]

Why do you need any changes here? If calling parameters are correct up and down
the chain the existing code should work fine, no?

[AndyWu, 2017/08/02 01:43:40]

For compressed bitstream formats, a partial compressed audio frame is less
useful, since it can't generate any PCM frame out of it. Also, we want to keep
the granularity as fine as possible so that discarding a small chunk of PCM
frames is still possible. Thus, we only transfer a complete AudioBuffer at a
time.

[DaleCurtis, 2017/08/03 01:38:10]

I guess I meant why not just DCHECK() that you have the right parameters when a
bitstream format is used.

[AndyWu, 2017/08/03 17:11:17]

Unfortunately, we don't know the right parameter during initialization stage
[1], so I use a reasonable large number, kMaxFramesPerCompressedAudioBuffer, to
only ensure the capacity of |dest|. Thus, the frame count of |dest| won't be the
same as the frame count of a AudioBuffer.

[1]
https://chromium-review.googlesource.com/c/596720/2/media/renderers/audio_ren...

                                    int dest_frame_offset,
                                    AudioBus* dest) {
+  if (!buffers_.size())

[DaleCurtis, 2017/06/15 21:46:32]

if buffers_.empty()

[AndyWu, 2017/08/02 01:43:40]

Done.

+    return 0;
+
+  if ((*buffers_.begin())->IsBitstreamFormat()) {

[chcunningham, 2017/06/14 20:03:08]

This block needs a comment to explain why |frames| is totally ignored in this
case (you're just reading the whole of the front buffer)

[DaleCurtis, 2017/06/15 21:46:32]

Just buffers_.front()->IsBitstreamFormat()

[AndyWu, 2017/08/02 01:43:40]

Done.

[AndyWu, 2017/08/02 01:43:40]

Done.

+    DCHECK(!dest_frame_offset);
+    DCHECK(!source_frame_offset);
+
+    scoped_refptr<AudioBuffer> buffer = buffers_.front();

[chcunningham, 2017/06/14 20:03:08]

Should you use current_buffer_ here to be consistent with original code below?

[AndyWu, 2017/08/02 01:43:40]

Done.

+    int taken = buffer->frame_count();
+
+    // if |dest| is NULL, there's no need to copy.
+    if (dest)
+      buffer->ReadFrames(buffer->frame_count(), 0, dest->data_size(), dest);

[chcunningham, 2017/06/14 20:03:08]

Do you mean to pass dest->data_size() here? Shouldn't this be
|dest_frame_offset|? 

[AndyWu, 2017/08/02 01:43:40]

You are right!
I was trying to append the data to the end of existing data, so I passed
data_size() instead of |dest_frame_offset|. It's tricky and bad.

+
+    if (advance_position) {
+      // Update the appropriate values since |taken| frames have been copied
+      // out.
+      frames_ -= taken;
+      DCHECK_GE(frames_, 0);
+
+      // Remove any buffers before the current buffer as there is no going
+      // backwards.
+      buffers_.pop_front();
+      current_buffer_ = buffers_.begin();
+    }
+
+    return taken;
+  }
+
   // Counts how many frames are actually read from the buffer queue.
   int taken = 0;
   BufferQueue::iterator current_buffer = current_buffer_;