Refactor ChunkDemuxer code so all source state is in a SourceState class.

media/filters/chunk_demuxer.cc

Index: media/filters/chunk_demuxer.cc
diff --git a/media/filters/chunk_demuxer.cc b/media/filters/chunk_demuxer.cc
index fdd47e7fcf8cb4f2e3871fb82b03658d519bec51..cc086af4c5f4b04eb49306b212665c1d5f116ecb 100644
--- a/media/filters/chunk_demuxer.cc
+++ b/media/filters/chunk_demuxer.cc
@@ -21,6 +21,147 @@ using base::TimeDelta;
 
 namespace media {
 
+// Contains state belonging to a source id.
+class SourceState {

[scherkus (not reviewing), 2013/04/05 18:58:26]

does it make any sense to split this class out + unit-test-ify it?

[acolwell GONE FROM CHROMIUM, 2013/04/05 19:14:37]

I'm not sure it quite makes sense yet. I think ChunkDemuxer and SourceState are
still a little too coupled right now for us to have separate unit tests for
each. I expect further refactoring will make these 2 classes a little more
independent and I think I'll be able to refactor the tests then. 

+ public:
+  explicit SourceState(scoped_ptr<StreamParser> stream_parser);
+
+  void Init(const StreamParser::InitCB& init_cb,
+            const StreamParser::NewConfigCB& config_cb,
+            const StreamParser::NewBuffersCB& audio_cb,
+            const StreamParser::NewBuffersCB& video_cb,
+            const StreamParser::NeedKeyCB& need_key_cb,
+            const StreamParser::NewMediaSegmentCB& new_segment_cb,
+            const LogCB& log_cb);
+
+  // Appends new data to the StreamParser.
+  // Returns true if the data was successfully appended. Returns false if an
+  // error occurred.
+  bool Append(const uint8* data, size_t length);
+
+  // Aborts the current append sequence and resets the parser.
+  void Abort();
+
+  // Sets |timestamp_offset_| if possible.
+  // Returns if the offset was set. Returns false if the offset could not be
+  // updated at this time.
+  bool SetTimestampOffset(TimeDelta timestamp_offset);
+
+  TimeDelta timestamp_offset() const { return timestamp_offset_; }
+
+ private:
+  // Called by the |stream_parser_| at the beginning of a new media segment.
+  // |timestamp| is the timestamp on the first buffer in the segment.
+  // It modifies the state of this object and then calls |new_segment_cb| with
+  // modified version of |timestamp|.
+  void OnNewMediaSegment(const StreamParser::NewMediaSegmentCB& new_segment_cb,
+                         TimeDelta timestamp);
+
+  // Called by the |stream_parser_| at the end of a media segment.
+  void OnEndOfMediaSegment();
+
+  // Called by the |stream_parser_| when new buffers have been parsed. It
+  // applies |timestamp_offset_| to all buffers in |buffers| and then calls
+  // |new_buffers_cb| with the modified buffers.
+  // Returns true on a successful call. Returns false if an error occured while
+  // processing the buffers.
+  bool OnBuffers(const StreamParser::NewBuffersCB& new_buffers_cb,
+                 const StreamParser::BufferQueue& buffers);
+
+  // Helper function that adds |timestamp_offset_| to each buffer in |buffers|.
+  void AdjustBufferTimestamps(const StreamParser::BufferQueue& buffers);
+
+  // The offset to apply to media segment timestamps.
+  TimeDelta timestamp_offset_;
+
+  // Keeps track of whether |timestamp_offset_| can be modified.
+  bool can_update_offset_;
+
+  // The object used to parse appended data.
+  scoped_ptr<StreamParser> stream_parser_;
+
+  DISALLOW_COPY_AND_ASSIGN(SourceState);
+};
+
+SourceState::SourceState(scoped_ptr<StreamParser> stream_parser)
+    : can_update_offset_(true),
+      stream_parser_(stream_parser.release()) {
+}
+
+void SourceState::Init(const StreamParser::InitCB& init_cb,
+                       const StreamParser::NewConfigCB& config_cb,
+                       const StreamParser::NewBuffersCB& audio_cb,
+                       const StreamParser::NewBuffersCB& video_cb,
+                       const StreamParser::NeedKeyCB& need_key_cb,
+                       const StreamParser::NewMediaSegmentCB& new_segment_cb,
+                       const LogCB& log_cb) {
+  stream_parser_->Init(init_cb, config_cb,
+                       base::Bind(&SourceState::OnBuffers,
+                                  base::Unretained(this), audio_cb),
+                       base::Bind(&SourceState::OnBuffers,
+                                  base::Unretained(this), video_cb),
+                       need_key_cb,
+                       base::Bind(&SourceState::OnNewMediaSegment,
+                                  base::Unretained(this), new_segment_cb),
+                       base::Bind(&SourceState::OnEndOfMediaSegment,
+                                  base::Unretained(this)),
+                       log_cb);
+}
+
+bool SourceState::SetTimestampOffset(TimeDelta timestamp_offset) {
+  if (!can_update_offset_)
+    return false;
+
+  timestamp_offset_ = timestamp_offset;
+  return true;
+}
+
+bool SourceState::Append(const uint8* data, size_t length) {
+  return stream_parser_->Parse(data, length);
+}
+
+void SourceState::Abort() {
+  stream_parser_->Flush();
+  can_update_offset_ = true;
+}
+
+void SourceState::AdjustBufferTimestamps(
+    const StreamParser::BufferQueue& buffers) {
+  if (timestamp_offset_ == TimeDelta())
+    return;
+
+  for (StreamParser::BufferQueue::const_iterator itr = buffers.begin();
+       itr != buffers.end(); ++itr) {
+    (*itr)->SetDecodeTimestamp(
+        (*itr)->GetDecodeTimestamp() + timestamp_offset_);
+    (*itr)->SetTimestamp((*itr)->GetTimestamp() + timestamp_offset_);
+  }
+}
+
+void SourceState::OnNewMediaSegment(
+    const StreamParser::NewMediaSegmentCB& new_segment_cb,
+    TimeDelta timestamp) {
+  DCHECK(timestamp != kNoTimestamp());
+  DVLOG(2) << "OnNewMediaSegment(" << timestamp.InSecondsF() << ")";
+
+  can_update_offset_ = false;
+  new_segment_cb.Run(timestamp + timestamp_offset_);
+}
+
+void SourceState::OnEndOfMediaSegment() {
+  DVLOG(2) << "OnEndOfMediaSegment()";
+  can_update_offset_ = true;
+}
+
+bool SourceState::OnBuffers(const StreamParser::NewBuffersCB& new_buffers_cb,
+                            const StreamParser::BufferQueue& buffers) {
+  if (new_buffers_cb.is_null())
+    return false;
+
+  AdjustBufferTimestamps(buffers);
+
+  return new_buffers_cb.Run(buffers);
+}
 
 class ChunkDemuxerStream : public DemuxerStream {
  public:
@@ -533,8 +674,7 @@ ChunkDemuxer::Status ChunkDemuxer::AddId(const std::string& id,
   DCHECK_GT(codecs.size(), 0u);
   base::AutoLock auto_lock(lock_);
 
-  if ((state_ != WAITING_FOR_INIT && state_ != INITIALIZING) ||
-      stream_parser_map_.count(id) > 0u)
+  if ((state_ != WAITING_FOR_INIT && state_ != INITIALIZING) || IsValidId(id))
     return kReachedIdLimit;
 
   bool has_audio = false;
@@ -565,22 +705,18 @@ ChunkDemuxer::Status ChunkDemuxer::AddId(const std::string& id,
                           base::Unretained(this));
   }
 
-  stream_parser->Init(
-      base::Bind(&ChunkDemuxer::OnStreamParserInitDone, base::Unretained(this)),
+  scoped_ptr<SourceState> source_state(new SourceState(stream_parser.Pass()));
+  source_state->Init(
+      base::Bind(&ChunkDemuxer::OnSourceInitDone, base::Unretained(this)),
       base::Bind(&ChunkDemuxer::OnNewConfigs, base::Unretained(this),
                  has_audio, has_video),
       audio_cb,
       video_cb,
       base::Bind(&ChunkDemuxer::OnNeedKey, base::Unretained(this)),
       base::Bind(&ChunkDemuxer::OnNewMediaSegment, base::Unretained(this), id),
-      base::Bind(&ChunkDemuxer::OnEndOfMediaSegment,
-                 base::Unretained(this), id),
       log_cb_);
 
-  stream_parser_map_[id] = stream_parser.release();
-  SourceInfo info = { base::TimeDelta(), true };
-  source_info_map_[id] = info;
-
+  source_state_map_[id] = source_state.release();
   return kOk;
 }
 
@@ -588,9 +724,8 @@ void ChunkDemuxer::RemoveId(const std::string& id) {
   base::AutoLock auto_lock(lock_);
   CHECK(IsValidId(id));
 
-  delete stream_parser_map_[id];
-  stream_parser_map_.erase(id);
-  source_info_map_.erase(id);
+  delete source_state_map_[id];
+  source_state_map_.erase(id);
 
   if (source_id_audio_ == id) {
     if (audio_)
@@ -689,7 +824,7 @@ void ChunkDemuxer::AppendData(const std::string& id,
     switch (state_) {
       case INITIALIZING:
         DCHECK(IsValidId(id));
-        if (!stream_parser_map_[id]->Parse(data, length)) {
+        if (!source_state_map_[id]->Append(data, length)) {
           ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN);
           return;
         }
@@ -697,7 +832,7 @@ void ChunkDemuxer::AppendData(const std::string& id,
 
       case INITIALIZED: {
         DCHECK(IsValidId(id));
-        if (!stream_parser_map_[id]->Parse(data, length)) {
+        if (!source_state_map_[id]->Append(data, length)) {
           ReportError_Locked(PIPELINE_ERROR_DECODE);
           return;
         }
@@ -735,8 +870,7 @@ void ChunkDemuxer::Abort(const std::string& id) {
   base::AutoLock auto_lock(lock_);
   DCHECK(!id.empty());
   CHECK(IsValidId(id));
-  stream_parser_map_[id]->Flush();
-  source_info_map_[id].can_update_offset = true;
+  source_state_map_[id]->Abort();
 }
 
 double ChunkDemuxer::GetDuration() {
@@ -807,11 +941,7 @@ bool ChunkDemuxer::SetTimestampOffset(const std::string& id, TimeDelta offset) {
   DVLOG(1) << "SetTimestampOffset(" << id << ", " << offset.InSecondsF() << ")";
   CHECK(IsValidId(id));
 
-  if (!source_info_map_[id].can_update_offset)
-    return false;
-
-  source_info_map_[id].timestamp_offset = offset;
-  return true;
+  return source_state_map_[id]->SetTimestampOffset(offset);
 }
 
 bool ChunkDemuxer::EndOfStream(PipelineStatus status) {
@@ -880,11 +1010,11 @@ void ChunkDemuxer::ChangeState_Locked(State new_state) {
 
 ChunkDemuxer::~ChunkDemuxer() {
   DCHECK_NE(state_, INITIALIZED);
-  for (StreamParserMap::iterator it = stream_parser_map_.begin();
-       it != stream_parser_map_.end(); ++it) {
+  for (SourceStateMap::iterator it = source_state_map_.begin();
+       it != source_state_map_.end(); ++it) {
     delete it->second;
   }
-  stream_parser_map_.clear();
+  source_state_map_.clear();
 }
 
 void ChunkDemuxer::ReportError_Locked(PipelineStatus error) {
@@ -938,8 +1068,8 @@ bool ChunkDemuxer::CanEndOfStream_Locked() const {
          (!video_ || video_->CanEndOfStream());
 }
 
-void ChunkDemuxer::OnStreamParserInitDone(bool success, TimeDelta duration) {
-  DVLOG(1) << "OnStreamParserInitDone(" << success << ", "
+void ChunkDemuxer::OnSourceInitDone(bool success, TimeDelta duration) {
+  DVLOG(1) << "OnSourceInitDone(" << success << ", "
            << duration.InSecondsF() << ")";
   lock_.AssertAcquired();
   DCHECK_EQ(state_, INITIALIZING);
@@ -1036,9 +1166,6 @@ bool ChunkDemuxer::OnAudioBuffers(const StreamParser::BufferQueue& buffers) {
     return false;
 
   CHECK(IsValidId(source_id_audio_));
-  AdjustBufferTimestamps(
-      buffers, source_info_map_[source_id_audio_].timestamp_offset);
-
   if (!audio_->Append(buffers))
     return false;
 
@@ -1054,9 +1181,6 @@ bool ChunkDemuxer::OnVideoBuffers(const StreamParser::BufferQueue& buffers) {
     return false;
 
   CHECK(IsValidId(source_id_video_));
-  AdjustBufferTimestamps(
-      buffers, source_info_map_[source_id_video_].timestamp_offset);
-
   if (!video_->Append(buffers))
     return false;
 
@@ -1083,40 +1207,15 @@ void ChunkDemuxer::OnNewMediaSegment(const std::string& source_id,
   lock_.AssertAcquired();
 
   CHECK(IsValidId(source_id));
-  source_info_map_[source_id].can_update_offset = false;
-  base::TimeDelta start_timestamp =
-      timestamp + source_info_map_[source_id].timestamp_offset;
-
   if (audio_ && source_id == source_id_audio_)
-    audio_->OnNewMediaSegment(start_timestamp);
+    audio_->OnNewMediaSegment(timestamp);
   if (video_ && source_id == source_id_video_)
-    video_->OnNewMediaSegment(start_timestamp);
-}
-
-void ChunkDemuxer::OnEndOfMediaSegment(const std::string& source_id) {
-  DVLOG(2) << "OnEndOfMediaSegment(" << source_id << ")";
-  CHECK(IsValidId(source_id));
-  source_info_map_[source_id].can_update_offset = true;
-}
-
-void ChunkDemuxer::AdjustBufferTimestamps(
-    const StreamParser::BufferQueue& buffers,
-    base::TimeDelta timestamp_offset) {
-  if (timestamp_offset == base::TimeDelta())
-    return;
-
-  for (StreamParser::BufferQueue::const_iterator itr = buffers.begin();
-       itr != buffers.end(); ++itr) {
-    (*itr)->SetDecodeTimestamp(
-        (*itr)->GetDecodeTimestamp() + timestamp_offset);
-    (*itr)->SetTimestamp((*itr)->GetTimestamp() + timestamp_offset);
-  }
+    video_->OnNewMediaSegment(timestamp);
 }
 
 bool ChunkDemuxer::IsValidId(const std::string& source_id) const {
   lock_.AssertAcquired();
-  return source_info_map_.count(source_id) > 0u &&
-      stream_parser_map_.count(source_id) > 0u;
+  return source_state_map_.count(source_id) > 0u;
 }
 
 void ChunkDemuxer::UpdateDuration(base::TimeDelta new_duration) {