Update media duration if data is appended after the previous duration

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 067fc0eeea75ac72196934ee7e283f6ab01c6dd5..907b0bdcd394c1c3b2e6fdae27058593af26f224 100644
--- a/media/filters/chunk_demuxer.cc
+++ b/media/filters/chunk_demuxer.cc
@@ -523,8 +523,7 @@ ChunkDemuxer::ChunkDemuxer(ChunkDemuxerClient* client)
   DCHECK(client);
 }
 
-void ChunkDemuxer::Initialize(DemuxerHost* host,
-                              const PipelineStatusCB& cb) {
+void ChunkDemuxer::Initialize(DemuxerHost* host, const PipelineStatusCB& cb) {
   DVLOG(1) << "Init()";
   {
     base::AutoLock auto_lock(lock_);
@@ -788,13 +787,7 @@ bool ChunkDemuxer::AppendData(const std::string& id,
       std::swap(cb, seek_cb_);
     }
 
-    if (audio_ && !video_) {
-      ranges = audio_->GetBufferedRanges();
-    } else if (!audio_ && video_) {
-      ranges = video_->GetBufferedRanges();
-    } else {
-      ranges = ComputeIntersection();
-    }
+    ranges = GetBufferedRanges();
   }
 
   for (size_t i = 0; i < ranges.size(); ++i)
@@ -968,15 +961,13 @@ void ChunkDemuxer::OnStreamParserInitDone(bool success, TimeDelta duration) {
   }
 
   if (duration > duration_)
-    duration_ = duration;
+    UpdateDuration(duration);
 
   // Wait until all streams have initialized.
   if ((!source_id_audio_.empty() && !audio_) ||
       (!source_id_video_.empty() && !video_))
     return;
 
-  host_->SetDuration(duration_);
-
   ChangeState_Locked(WAITING_FOR_START_TIME);
 }
 
@@ -1035,6 +1026,8 @@ bool ChunkDemuxer::OnAudioBuffers(const StreamParser::BufferQueue& buffers) {
   AdjustBufferTimestamps(
       buffers, source_info_map_[source_id_audio_].timestamp_offset);
 
+  IncreaseDurationIfNecessary(buffers);
+
   return audio_->Append(buffers);
 }
 
@@ -1049,6 +1042,8 @@ bool ChunkDemuxer::OnVideoBuffers(const StreamParser::BufferQueue& buffers) {
   AdjustBufferTimestamps(
       buffers, source_info_map_[source_id_video_].timestamp_offset);
 
+  IncreaseDurationIfNecessary(buffers);
+
   return video_->Append(buffers);
 }
 
@@ -1093,6 +1088,9 @@ void ChunkDemuxer::OnNewMediaSegment(const std::string& source_id,
     video_->Seek(start_time_);
   }
 
+  if (start_time_ > duration_)
+    UpdateDuration(start_time_);

[acolwell GONE FROM CHROMIUM, 2012/07/27 00:03:23]

nit: Could this be up where start_time_ is initially set?
Should we have a comment here indicating that this should be changed to
(start_time - start_time_) once http://crbug.com/137275 is addressed?

[vrk (LEFT CHROMIUM), 2012/07/28 01:42:05]

Done.

+
   // The demuxer is now initialized after the |start_timestamp_| was set.
   ChangeState_Locked(INITIALIZED);
   base::ResetAndReturn(&init_cb_).Run(PIPELINE_OK);
@@ -1123,4 +1121,32 @@ bool ChunkDemuxer::IsValidId(const std::string& source_id) const {
       stream_parser_map_.count(source_id) > 0u;
 }
 
+void ChunkDemuxer::UpdateDuration(base::TimeDelta new_duration) {
+  duration_ = new_duration;

[acolwell GONE FROM CHROMIUM, 2012/07/27 00:03:23]

DCHECK(duration_ != new_duration) to make sure code only notifies on changes?

[vrk (LEFT CHROMIUM), 2012/07/28 01:42:05]

Done.

+  host_->SetDuration(new_duration);

[acolwell GONE FROM CHROMIUM, 2012/07/27 00:03:23]

Does this cause double duration notifications on muxed content appended to the
end? If so I wonder if we should defer notifying the host until the end of
AppendData() & EndOfStream()?

[vrk (LEFT CHROMIUM), 2012/07/28 01:42:05]

Yes this happens!
Talked offline; it's a little icky to do this elegantly so I'll keep as-is for
now!

+}
+
+void ChunkDemuxer::IncreaseDurationIfNecessary(
+    const StreamParser::BufferQueue& buffers) {
+  DCHECK(!buffers.empty());
+  if (buffers.back()->GetTimestamp() <= duration_)
+    return;
+
+  Ranges<TimeDelta> ranges = GetBufferedRanges();
+  base::TimeDelta last_timestamp_buffered = ranges.end(ranges.size() - 1);
+  DCHECK(last_timestamp_buffered > duration_);

[acolwell GONE FROM CHROMIUM, 2012/07/27 00:03:23]

I don't think this is right. The buffers aren't appended yet so they won't be
reflected by GetBufferedRanges(). Also calling GetBufferedRanges() seems a
little heavy-weight here especially since the caller knows whether the buffers
are being appended to audio_ or video_. You could just pass a ChunkDemuxerStream
pointer to this method and call GetBufferedRanges() on that.

[vrk (LEFT CHROMIUM), 2012/07/28 01:42:05]

AHH you are right! I must have fiddled with this before testing, this breaks
pretty immediately when tried out :) 

Fixed, and the duration change tests I added also fail with this un-fixed.
Done, but FYI this changes the behavior a little bit. If in a muxed stream, the
audio data ends at 25s and the video ends at 30s  (and the original duration
was, say, 20s), the code here would say the duration is 25s (which matches
sourceBuffered()), while your suggested changes makes the duration 30s. I think
30s makes more sense, though it would mean that the duration would always exceed
sourceBuffered() if the audio and video don't line up perfectly at the end. Do
you have thoughts on this?

+  UpdateDuration(last_timestamp_buffered);
+}
+
+Ranges<TimeDelta> ChunkDemuxer::GetBufferedRanges() const {
+  lock_.AssertAcquired();
+
+  if (audio_ && !video_)
+    return audio_->GetBufferedRanges();
+  else if (!audio_ && video_)
+    return video_->GetBufferedRanges();
+
+  return ComputeIntersection();
+}
+
 }  // namespace media