Fix MP3StreamParser so it adds buffer timestamps and marks the end of segments.

BUG=280550
TEST=MP3StreamParserTest.*
R=scherkus@chromium.org

Review URL: https://codereview.chromium.org/24287002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@224484 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 238 insertions, 21 deletions

diff --git a/media/media.gyp b/media/media.gyp
index a25571d..c0f2327 100644
--- a/media/media.gyp
+++ b/media/media.gyp
@@ -1001,6 +1001,7 @@
             'filters/ffmpeg_video_decoder_unittest.cc',
             'filters/pipeline_integration_test.cc',
             'filters/pipeline_integration_test_base.cc',
+            'mp3/mp3_stream_parser_unittest.cc',
             'mp4/mp4_stream_parser_unittest.cc',
             'webm/webm_cluster_parser_unittest.cc',
           ],
@@ -1037,8 +1038,9 @@
             'base/simd/convert_rgb_to_yuv_unittest.cc',
           ],
         }],
-        ['proprietary_codecs==1 or branding=="Chrome"', {
+        ['proprietary_codecs==1', {
           'sources': [
+            'mp3/mp3_stream_parser_unittest.cc',
             'mp4/aac_unittest.cc',
             'mp4/avc_unittest.cc',
             'mp4/box_reader_unittest.cc',
diff --git a/media/mp3/mp3_stream_parser.cc b/media/mp3/mp3_stream_parser.cc
index 319c868..0688d99 100644
--- a/media/mp3/mp3_stream_parser.cc
+++ b/media/mp3/mp3_stream_parser.cc
@@ -157,18 +157,25 @@ bool MP3StreamParser::Parse(const uint8* buf, int size) {
 
   queue_.Push(buf, size);
 
+  bool end_of_segment = true;
+  BufferQueue buffers;
   for (;;) {
     const uint8* data;
     int data_size;
     queue_.Peek(&data, &data_size);
 
     if (size < 4)
-      return true;
+      break;
 
     uint32 start_code = data[0] << 24 | data[1] << 16 | data[2] << 8 | data[3];
     int bytes_read = 0;
+    bool parsed_metadata = true;
     if ((start_code & kMP3StartCodeMask) == kMP3StartCodeMask) {
-      bytes_read = ParseMP3Frame(data, data_size);
+      bytes_read = ParseMP3Frame(data, data_size, &buffers);
+
+      // Only allow the current segment to end if a full frame has been parsed.
+      end_of_segment = bytes_read > 0;
+      parsed_metadata = false;
     } else if (start_code == kICYStartCode) {
       bytes_read = ParseIcecastHeader(data, data_size);
     } else if ((start_code & kID3StartCodeMask) == kID3v1StartCode) {
@@ -191,13 +198,22 @@ bool MP3StreamParser::Parse(const uint8* buf, int size) {
       return false;
     } else if (bytes_read == 0) {
       // Need more data.
-      return true;
+      break;
     }
 
+    // Send pending buffers if we have encountered metadata.
+    if (parsed_metadata && !buffers.empty() && !SendBuffers(&buffers, true))
+      return false;
+
     queue_.Pop(bytes_read);
+    end_of_segment = true;
   }
 
-  return true;
+  if (buffers.empty())
+    return true;
+
+  // Send buffers collected in this append that haven't been sent yet.
+  return SendBuffers(&buffers, end_of_segment);
 }
 
 void MP3StreamParser::ChangeState(State state) {
@@ -348,7 +364,9 @@ int MP3StreamParser::ParseFrameHeader(const uint8* data, int size,
   return 4;
 }
 
-int MP3StreamParser::ParseMP3Frame(const uint8* data, int size) {
+int MP3StreamParser::ParseMP3Frame(const uint8* data,
+                                   int size,
+                                   BufferQueue* buffers) {
   DVLOG(2) << __FUNCTION__ << "(" << size << ")";
 
   int sample_rate;
@@ -374,6 +392,10 @@ int MP3StreamParser::ParseMP3Frame(const uint8* data, int size) {
        config_.channel_layout() != channel_layout)) {
     // Clear config data so that a config change is initiated.
     config_ = AudioDecoderConfig();
+
+    // Send all buffers associated with the previous config.
+    if (!buffers->empty() && !SendBuffers(buffers, true))
+      return -1;
   }
 
   if (!config_.IsValidConfig()) {
@@ -398,22 +420,11 @@ int MP3StreamParser::ParseMP3Frame(const uint8* data, int size) {
       return -1;
   }
 
-  if (!in_media_segment_) {
-    in_media_segment_ = true;
-    new_segment_cb_.Run();
-  }
-
-  BufferQueue audio_buffers;
-  BufferQueue video_buffers;
-
-  // TODO(acolwell): Change this code to parse as many frames as
-  // possible before calling |new_buffers_cb_|.
   scoped_refptr<StreamParserBuffer> buffer =
       StreamParserBuffer::CopyFrom(data, frame_size, true);
-  audio_buffers.push_back(buffer);
-
-  if (!new_buffers_cb_.Run(audio_buffers, video_buffers))
-    return -1;
+  buffer->set_timestamp(timestamp_helper_->GetTimestamp());
+  buffer->set_duration(timestamp_helper_->GetFrameDuration(sample_count));
+  buffers->push_back(buffer);
 
   timestamp_helper_->AddFrames(sample_count);
 
@@ -563,4 +574,25 @@ int MP3StreamParser::FindNextValidStartCode(const uint8* data, int size) const {
   return 0;
 }
 
+bool MP3StreamParser::SendBuffers(BufferQueue* buffers, bool end_of_segment) {
+  DCHECK(!buffers->empty());
+
+  if (!in_media_segment_) {
+    in_media_segment_ = true;
+    new_segment_cb_.Run();
+  }
+
+  BufferQueue empty_video_buffers;
+  if (!new_buffers_cb_.Run(*buffers, empty_video_buffers))
+    return false;
+  buffers->clear();
+
+  if (end_of_segment) {
+    in_media_segment_ = false;
+    end_of_segment_cb_.Run();
+  }
+
+  return true;
+}
+
 }  // namespace media
diff --git a/media/mp3/mp3_stream_parser.h b/media/mp3/mp3_stream_parser.h
index a4b4057..97730ae 100644
--- a/media/mp3/mp3_stream_parser.h
+++ b/media/mp3/mp3_stream_parser.h
@@ -88,7 +88,7 @@ class MEDIA_EXPORT MP3StreamParser : public StreamParser {
                        int* sample_rate,
                        ChannelLayout* channel_layout,
                        int* sample_count) const;
-  int ParseMP3Frame(const uint8* data, int size);
+  int ParseMP3Frame(const uint8* data, int size, BufferQueue* buffers);
   int ParseIcecastHeader(const uint8* data, int size);
   int ParseID3v1(const uint8* data, int size);
   int ParseID3v2(const uint8* data, int size);
@@ -111,6 +111,14 @@ class MEDIA_EXPORT MP3StreamParser : public StreamParser {
   // < 0 : An error was encountered during parsing.
   int FindNextValidStartCode(const uint8* data, int size) const;
 
+  // Sends the buffers in |buffers| to |new_buffers_cb_| and then clears
+  // |buffers|.
+  // If |end_of_segment| is set to true, then |end_of_segment_cb_| is called
+  // after |new_buffers_cb_| to signal that these buffers represent the end of a
+  // media segment.
+  // Returns true if the buffers are sent successfully.
+  bool SendBuffers(BufferQueue* buffers, bool end_of_segment);
+
   DISALLOW_COPY_AND_ASSIGN(MP3StreamParser);
 };
 
diff --git a/media/mp3/mp3_stream_parser_unittest.cc b/media/mp3/mp3_stream_parser_unittest.cc
new file mode 100644
index 0000000..9d30954
--- /dev/null
+++ b/media/mp3/mp3_stream_parser_unittest.cc
@@ -0,0 +1,175 @@
+// Copyright 2013 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 "base/bind.h"
+#include "media/base/audio_decoder_config.h"
+#include "media/base/decoder_buffer.h"
+#include "media/base/stream_parser_buffer.h"
+#include "media/base/test_data_util.h"
+#include "media/base/video_decoder_config.h"
+#include "media/mp3/mp3_stream_parser.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace media {
+
+class MP3StreamParserTest : public testing::Test {
+ public:
+  MP3StreamParserTest() {}
+
+ protected:
+  MP3StreamParser parser_;
+  std::stringstream results_stream_;
+
+  bool AppendData(const uint8* data, size_t length) {
+    return parser_.Parse(data, length);
+  }
+
+  bool AppendDataInPieces(const uint8* data, size_t length, size_t piece_size) {
+    const uint8* start = data;
+    const uint8* end = data + length;
+    while (start < end) {
+      size_t append_size =
+          std::min(piece_size, static_cast<size_t>(end - start));
+      if (!AppendData(start, append_size))
+        return false;
+      start += append_size;
+    }
+    return true;
+  }
+
+  void OnInitDone(bool success, base::TimeDelta duration) {
+    DVLOG(1) << __FUNCTION__ << "(" << success << ", "
+             << duration.InMilliseconds() << ")";
+  }
+
+  bool OnNewConfig(const AudioDecoderConfig& audio_config,
+                   const VideoDecoderConfig& video_config) {
+    DVLOG(1) << __FUNCTION__ << "(" << audio_config.IsValidConfig() << ", "
+             << video_config.IsValidConfig() << ")";
+    EXPECT_TRUE(audio_config.IsValidConfig());
+    EXPECT_FALSE(video_config.IsValidConfig());
+    return true;
+  }
+
+  std::string BufferQueueToString(const StreamParser::BufferQueue& buffers) {
+    std::stringstream ss;
+
+    ss << "{";
+    for (StreamParser::BufferQueue::const_iterator itr = buffers.begin();
+         itr != buffers.end();
+         ++itr) {
+      ss << " " << (*itr)->timestamp().InMilliseconds();
+      if ((*itr)->IsKeyframe())
+        ss << "K";
+    }
+    ss << " }";
+
+    return ss.str();
+  }
+
+  bool OnNewBuffers(const StreamParser::BufferQueue& audio_buffers,
+                    const StreamParser::BufferQueue& video_buffers) {
+    EXPECT_FALSE(audio_buffers.empty());
+    EXPECT_TRUE(video_buffers.empty());
+
+    std::string buffers_str = BufferQueueToString(audio_buffers);
+    DVLOG(1) << __FUNCTION__ << " : " << buffers_str;
+    results_stream_ << buffers_str;
+    return true;
+  }
+
+  bool OnNewTextBuffers(TextTrack* text_track,
+                        const StreamParser::BufferQueue& buffers) {
+    return true;
+  }
+
+  void OnKeyNeeded(const std::string& type,
+                   const std::vector<uint8>& init_data) {
+    DVLOG(1) << __FUNCTION__ << "(" << type << ", " << init_data.size() << ")";
+  }
+
+  scoped_ptr<TextTrack> OnAddTextTrack(TextKind kind,
+                                       const std::string& label,
+                                       const std::string& language) {
+    return scoped_ptr<TextTrack>();
+  }
+
+  void OnNewSegment() {
+    DVLOG(1) << __FUNCTION__;
+    results_stream_ << "NewSegment";
+  }
+
+  void OnEndOfSegment() {
+    DVLOG(1) << __FUNCTION__;
+    results_stream_ << "EndOfSegment";
+  }
+
+  void InitializeParser() {
+    parser_.Init(
+        base::Bind(&MP3StreamParserTest::OnInitDone, base::Unretained(this)),
+        base::Bind(&MP3StreamParserTest::OnNewConfig, base::Unretained(this)),
+        base::Bind(&MP3StreamParserTest::OnNewBuffers, base::Unretained(this)),
+        base::Bind(&MP3StreamParserTest::OnNewTextBuffers,
+                   base::Unretained(this)),
+        base::Bind(&MP3StreamParserTest::OnKeyNeeded, base::Unretained(this)),
+        base::Bind(&MP3StreamParserTest::OnAddTextTrack,
+                   base::Unretained(this)),
+        base::Bind(&MP3StreamParserTest::OnNewSegment, base::Unretained(this)),
+        base::Bind(&MP3StreamParserTest::OnEndOfSegment,
+                   base::Unretained(this)),
+        LogCB());
+  }
+
+  std::string ParseFile(const std::string& filename, int append_bytes) {
+    results_stream_.clear();
+    InitializeParser();
+
+    scoped_refptr<DecoderBuffer> buffer = ReadTestDataFile(filename);
+    EXPECT_TRUE(
+        AppendDataInPieces(buffer->data(), buffer->data_size(), append_bytes));
+    return results_stream_.str();
+  }
+};
+
+// Test parsing with small prime sized chunks to smoke out "power of
+// 2" field size assumptions.
+TEST_F(MP3StreamParserTest, UnalignedAppend) {
+  std::string expected =
+      "NewSegment"
+      "{ 0K }"
+      "{ 26K }"
+      "{ 52K }"
+      "{ 78K }"
+      "{ 104K }"
+      "{ 130K }"
+      "{ 156K }"
+      "{ 182K }"
+      "EndOfSegment"
+      "NewSegment"
+      "{ 208K }"
+      "{ 235K }"
+      "{ 261K }"
+      "EndOfSegment"
+      "NewSegment"
+      "{ 287K }"
+      "{ 313K }"
+      "EndOfSegment";
+  EXPECT_EQ(expected, ParseFile("sfx.mp3", 17));
+}
+
+// Test parsing with a larger piece size to verify that multiple buffers
+// are passed to |new_buffer_cb_|.
+TEST_F(MP3StreamParserTest, UnalignedAppend512) {
+  std::string expected =
+      "NewSegment"
+      "{ 0K }"
+      "{ 26K 52K 78K 104K }"
+      "{ 130K 156K 182K }"
+      "{ 208K 235K 261K 287K }"
+      "{ 313K }"
+      "EndOfSegment";
+  EXPECT_EQ(expected, ParseFile("sfx.mp3", 512));
+}
+
+}  // namespace media