Fix ChunkDemuxer so it properly outputs buffered ranges.

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 fcb36258bd520a246fa7733a376fc79d14195cba..c7db12e76b67f725b79972b2fd4ea50fcf4a8dff 100644
--- a/media/filters/chunk_demuxer.cc
+++ b/media/filters/chunk_demuxer.cc
@@ -74,6 +74,11 @@ static const SupportedTypeInfo kSupportedTypeInfo[] = {
   { "audio/mp4", &BuildMP4Parser, kAudioMP4Codecs },
 };
 
+
+// The fake total size we use for converting times to bytes

[Ami GONE FROM CHROMIUM, 2012/06/18 16:18:31]

Yowza!  Why not instead teach Pipeline to track buffered times in addition to
buffered bytes?
(i.e. make it keep a Ranges<TimeDelta> in addition to its Ranges<int64> and make
GetBufferedTimeRanges() use whichever is present)

[acolwell GONE FROM CHROMIUM, 2012/06/19 06:12:34]

Based on our offline discussion, I'd like to defer the pipeline changes until
after branch cut.

+// for AddBufferedByteRange() calls.
+static int kFakeTotalBytes = 1000000;

[Ami GONE FROM CHROMIUM, 2012/06/18 16:18:31]

If you need to keep this, prefer enum to static int
(and, if you keep static int, make it const)

[acolwell GONE FROM CHROMIUM, 2012/06/19 06:12:34]

Done.

+
 // Checks to see if the specified |type| and |codecs| list are supported.
 // Returns true if |type| and all codecs listed in |codecs| are supported.
 //         |factory_function| contains a function that can build a StreamParser
@@ -464,8 +469,7 @@ void ChunkDemuxerStream::CreateReadDoneClosures_Locked(ClosureQueue* closures) {
 ChunkDemuxer::ChunkDemuxer(ChunkDemuxerClient* client)
     : state_(WAITING_FOR_INIT),
       host_(NULL),
-      client_(client),
-      buffered_bytes_(0) {
+      client_(client) {
   DCHECK(client);
 }
 
@@ -647,6 +651,25 @@ bool ChunkDemuxer::GetBufferedRanges(const std::string& id,
     return CopyIntoRanges(video_->GetBufferedTime(), ranges_out);
   }
 
+  return ComputeIntersection(ranges_out);
+}
+
+bool ChunkDemuxer::CopyIntoRanges(

[Ami GONE FROM CHROMIUM, 2012/06/18 16:18:31]

I realize this is just a move of an existing function, but it highlights a
question: why does ChunkDemuxer::Ranges exist, instead of re-using
SourceBufferStream::TimespanList ?

And, either way, can both types be replaced with a
media::Ranges<base::TimeDelta> ?  ISTM we probably want a new 
Ranges<T> IntersectionWith(const Ranges<T>& other);
method on Ranges, but otherwise it seems silly to have these various
range-maintaining one-offs.

(of course, it's quite possible Ranges didn't exist when this code was first
written; I'm just saying now it does exist can it be used here?)

Maybe/probably belongs in a different CL.

[acolwell GONE FROM CHROMIUM, 2012/06/19 06:12:34]

I've updated the CL to use media::Ranges and eliminated the other types. I
believe the code that introduce Timespans existed before Ranges existed and that
just propagated over the last few weeks instead of taking advantage of your
handy Ranges class.

+    const SourceBufferStream::TimespanList& timespans,
+    Ranges* ranges_out) const {
+  for (SourceBufferStream::TimespanList::const_iterator itr = timespans.begin();
+       itr != timespans.end(); ++itr) {
+    ranges_out->push_back(*itr);
+  }
+  return !timespans.empty();
+}
+
+bool ChunkDemuxer::ComputeIntersection(Ranges* ranges_out) const {
+  lock_.AssertAcquired();

[Ami GONE FROM CHROMIUM, 2012/06/18 16:18:31]

ranges_out->clear() ?

[acolwell GONE FROM CHROMIUM, 2012/06/19 06:12:34]

Done.

+
+  if (!audio_ && !video_)

[Ami GONE FROM CHROMIUM, 2012/06/18 16:18:31]

s/&&/||/ since you unconditionally deref them a few lines down (and, also, by
definition a missing stream intersected w/ anything else will be the empty set).

[acolwell GONE FROM CHROMIUM, 2012/06/19 06:12:34]

Done.

+    return false;
+
   // Include ranges that have been buffered in both |audio_| and |video_|.
   SourceBufferStream::TimespanList audio_ranges = audio_->GetBufferedTime();
   SourceBufferStream::TimespanList video_ranges = video_->GetBufferedTime();
@@ -695,16 +718,6 @@ bool ChunkDemuxer::GetBufferedRanges(const std::string& id,
   return success;
 }
 
-bool ChunkDemuxer::CopyIntoRanges(
-    const SourceBufferStream::TimespanList& timespans,
-    Ranges* ranges_out) const {
-  for (SourceBufferStream::TimespanList::const_iterator itr = timespans.begin();
-       itr != timespans.end(); ++itr) {
-    ranges_out->push_back(*itr);
-  }
-  return !timespans.empty();
-}
-
 void ChunkDemuxer::AddIntersectionRange(
     SourceBufferStream::Timespan timespan_a,
     SourceBufferStream::Timespan timespan_b,
@@ -732,7 +745,7 @@ bool ChunkDemuxer::AppendData(const std::string& id,
   DCHECK(data);
   DCHECK_GT(length, 0u);
 
-  int64 buffered_bytes = 0;
+  Ranges ranges;
 
   PipelineStatusCB cb;
   {
@@ -773,13 +786,25 @@ bool ChunkDemuxer::AppendData(const std::string& id,
       std::swap(cb, seek_cb_);
     }
 
-    buffered_bytes_ += length;
-    buffered_bytes = buffered_bytes_;
+    if (duration_ > base::TimeDelta() && duration_ != kInfiniteDuration()) {
+      if (audio_ && !video_) {
+        CopyIntoRanges(audio_->GetBufferedTime(), &ranges);
+      } else if (!audio_ && video_) {
+        CopyIntoRanges(video_->GetBufferedTime(), &ranges);
+      } else {
+        ComputeIntersection(&ranges);

[Ami GONE FROM CHROMIUM, 2012/06/18 16:18:31]

Note ignored return value; a hint that CI shouldn't be returning bool in the
first place.

[acolwell GONE FROM CHROMIUM, 2012/06/19 06:12:34]

Done.

+      }
+    }
   }
 
-  // Notify the host of 'network activity' because we got data, using a bogus
-  // range.
-  host_->AddBufferedByteRange(0, buffered_bytes);
+  for (Ranges::iterator itr = ranges.begin(); itr != ranges.end(); ++itr) {
+    // Notify the host of 'network activity' because we got data.
+    int64 start =
+        kFakeTotalBytes * itr->first.InSecondsF() / duration_.InSecondsF();

[Ami GONE FROM CHROMIUM, 2012/06/18 16:18:31]

at l.789 you needed to test for duration_ being TimeDelta(), but here you divide
by it unconditionally?  Divide-by-0?

[acolwell GONE FROM CHROMIUM, 2012/06/19 06:12:34]

ranges won't get populated if duration_ is 0 so this loop body won't run. Added
a DCHECK to make sure this assumption holds.

+    int64 end =
+        kFakeTotalBytes * itr->second.InSecondsF() / duration_.InSecondsF();
+    host_->AddBufferedByteRange(start, end);
+  }
 
   if (!cb.is_null())
     cb.Run(PIPELINE_OK);
@@ -948,6 +973,8 @@ void ChunkDemuxer::OnStreamParserInitDone(bool success,
       (!source_id_video_.empty() && !video_))
     return;
 
+  if (duration_ > base::TimeDelta() && duration_ != kInfiniteDuration())
+    host_->SetTotalBytes(kFakeTotalBytes);

[Ami GONE FROM CHROMIUM, 2012/06/18 16:18:31]

Hopefully this can go away.

   host_->SetDuration(duration_);
 
   ChangeState_Locked(INITIALIZED);