Refactoring StreamParser & StreamParserHost interfaces.

media/filters/chunk_demuxer.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/chunk_demuxer.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "media/base/audio_decoder_config.h"
@@ skipping 430 matching lines @@
 
   if (state_ == SHUTDOWN)
     return;
 
   if (audio_.get())
     audio_->Flush();
 
   if (video_.get())
     video_->Flush();
 
-  byte_queue_.Reset();
   stream_parser_->Flush();
 
   seek_waits_for_data_ = true;
   ChangeState_Locked(INITIALIZED);
 }
 
 bool ChunkDemuxer::AppendData(const uint8* data, size_t length) {
   DVLOG(1) << "AppendData(" << length << ")";
 
   if (!data || length == 0u)
     return false;
 
   int64 buffered_bytes = 0;
   base::TimeDelta buffered_ts = base::TimeDelta::FromSeconds(-1);
 
   PipelineStatusCB cb;
   {
     base::AutoLock auto_lock(lock_);
 
-    byte_queue_.Push(data, length);
-
-    const uint8* cur = NULL;
-    int cur_size = 0;
-    int bytes_parsed = 0;
-    int result = -1;
-
     // Capture |seek_waits_for_data_| state before we start parsing.
     // Its state can be changed by OnAudioBuffers() or OnVideoBuffers()
     // calls during the parse.
     bool old_seek_waits_for_data = seek_waits_for_data_;
 
-    byte_queue_.Peek(&cur, &cur_size);
+    switch(state_) {
+      case INITIALIZING:
+        if (!stream_parser_->Parse(data, length)) {
+          DCHECK_EQ(state_, INITIALIZING);
+          ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN);
+          return true;
+        }
+        break;
 
-    do {
-      switch(state_) {
-        case INITIALIZING:
-          result = stream_parser_->Parse(cur, cur_size);
-          if (result < 0) {
-            DCHECK_EQ(state_, INITIALIZING);
-            ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN);
-            return true;
-          }
-          break;
+      case INITIALIZED: {
+        if (!stream_parser_->Parse(data, length)) {
+          ReportError_Locked(PIPELINE_ERROR_DECODE);
+          return true;
+        }
+      } break;
 
-        case INITIALIZED: {
-          result = stream_parser_->Parse(cur, cur_size);
-          if (result < 0) {
-            ReportError_Locked(PIPELINE_ERROR_DECODE);
-            return true;
-          }
-        } break;
-
-        case WAITING_FOR_INIT:
-        case ENDED:
-        case PARSE_ERROR:
-        case SHUTDOWN:
-          DVLOG(1) << "AppendData(): called in unexpected state " << state_;
-          return false;
-      }
-
-      if (result > 0) {
-        cur += result;
-        cur_size -= result;
-        bytes_parsed += result;
-      }
-    } while (result > 0 && cur_size > 0);
-
-    byte_queue_.Pop(bytes_parsed);
+      case WAITING_FOR_INIT:
+      case ENDED:
+      case PARSE_ERROR:
+      case SHUTDOWN:
+        DVLOG(1) << "AppendData(): called in unexpected state " << state_;
+        return false;
+    }
 
     // Check to see if parsing triggered seek_waits_for_data_ to go from true to
     // false. This indicates we have parsed enough data to complete the seek.
     if (old_seek_waits_for_data && !seek_waits_for_data_ &&
         !seek_cb_.is_null()) {
       std::swap(cb, seek_cb_);
     }
 
     base::TimeDelta tmp;
     if (audio_.get() && audio_->GetLastBufferTimestamp(&tmp) &&
@@ skipping 137 matching lines @@
   duration_ = duration;
   host_->SetDuration(duration_);
   host_->SetCurrentReadPosition(0);
 
   ChangeState_Locked(INITIALIZED);
   PipelineStatusCB cb;
   std::swap(cb, init_cb_);
   cb.Run(PIPELINE_OK);
 }
 
-bool ChunkDemuxer::OnNewAudioConfig(const AudioDecoderConfig& config) {
+bool ChunkDemuxer::OnNewConfigs(const AudioDecoderConfig& audio_config,
+                                const VideoDecoderConfig& video_config) {
+  DCHECK(audio_config.IsValidConfig() || video_config.IsValidConfig());
   lock_.AssertAcquired();
-  // Only allow a single audio config for now.
-  if (audio_.get())
+
+  if (!audio_config.IsValidConfig() && !video_config.IsValidConfig())

[scherkus (not reviewing), 2012/04/24 01:40:04]

Considering we handle the DCHECK (and that the previous code did not check for
valid configs) -- does this mean we require a unit test for this code path?

If so, then it's tricky because we'll DCHECK so either:
  1) Remove the DCHECK and passing in two invalid configs is valid + write test
  2) Promote the DCHECK to a CHECK and remove this handler

[acolwell GONE FROM CHROMIUM, 2012/04/24 16:25:55]

Done. Picked option 2

     return false;
 
-  audio_ = new ChunkDemuxerStream(config);
+  // Only allow a single audio config for now.

[scherkus (not reviewing), 2012/04/24 01:40:04]

nit: you can either dupe this comment below for video or just include video in
this comment OR if you add LOG(ERROR) (or similar) for the false case the code
should be self documenting

[acolwell GONE FROM CHROMIUM, 2012/04/24 16:25:55]

Done.

+  if (audio_config.IsValidConfig()) {
+    if (audio_.get())
+      return false;
+
+    audio_ = new ChunkDemuxerStream(audio_config);
+  }
+
+  if (video_config.IsValidConfig()) {
+    if (video_.get())
+      return false;
+
+    video_ = new ChunkDemuxerStream(video_config);
+  }
+
   return true;
 }
 
-bool ChunkDemuxer::OnNewVideoConfig(const VideoDecoderConfig& config) {
-  lock_.AssertAcquired();
-  // Only allow a single video config for now.
-  if (video_.get())
-    return false;
-
-  video_ = new ChunkDemuxerStream(config);
-  return true;
-}
-
 bool ChunkDemuxer::OnAudioBuffers(const BufferQueue& buffers) {
   if (!audio_.get())
     return false;
 
   if (!audio_->CanAddBuffers(buffers))
     return false;
 
   audio_->AddBuffers(buffers);
   seek_waits_for_data_ = false;
 
   return true;
 }
 
 bool ChunkDemuxer::OnVideoBuffers(const BufferQueue& buffers) {
   if (!video_.get())
     return false;
 
   if (!video_->CanAddBuffers(buffers))
     return false;
 
   video_->AddBuffers(buffers);
   seek_waits_for_data_ = false;
 
   return true;
 }
 
 }  // namespace media