Fix VideoRendererBase end of stream logic.

media/filters/video_renderer_base.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/video_renderer_base.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/threading/platform_thread.h"
 #include "media/base/buffers.h"
 #include "media/base/limits.h"
 #include "media/base/pipeline.h"
 #include "media/base/video_frame.h"
 
 namespace media {
 
+base::TimeDelta VideoRendererBase::kLastFrameDuration() {
+  return base::TimeDelta::FromMilliseconds(250);
+}
+
 VideoRendererBase::VideoRendererBase(const base::Closure& paint_cb,
                                      const SetOpaqueCB& set_opaque_cb,
                                      bool drop_frames)
     : frame_available_(&lock_),
       state_(kUninitialized),
       thread_(base::kNullThreadHandle),
       pending_read_(false),
       pending_paint_(false),
       pending_paint_with_last_available_(false),
       drop_frames_(drop_frames),
@@ skipping 449 matching lines @@
   }
 }
 
 void VideoRendererBase::AddReadyFrame(const scoped_refptr<VideoFrame>& frame) {
   // Adjust the incoming frame if its rendering stop time is past the duration
   // of the video itself. This is typically the last frame of the video and
   // occurs if the container specifies a duration that isn't a multiple of the
   // frame rate.  Another way for this to happen is for the container to state a
   // smaller duration than the largest packet timestamp.
   base::TimeDelta duration = get_duration_cb_.Run();
-  if (frame->GetTimestamp() > duration || frame->IsEndOfStream()) {
+  if (frame->IsEndOfStream()) {
+    base::TimeDelta end_timestamp = kNoTimestamp();
+    if (!ready_frames_.empty()) {
+      end_timestamp =
+          std::min(duration,
+                   ready_frames_.back()->GetTimestamp() + kLastFrameDuration());
+    } else if (current_frame_) {
+      end_timestamp =
+          std::min(duration,
+                   current_frame_->GetTimestamp() + kLastFrameDuration());
+    }
+    frame->SetTimestamp(end_timestamp);

[DaleCurtis, 2012/08/07 20:32:14]

Since it wasn't immediately obvious to me: when end_timestamp == kNoTimestamp()
we'll end up sleeping for (kNoTimestamp() - get_time_cb_.Run()), which I assume
is a negative number thus line 236 will skip sleeping => ending immediately.

OOC could all this be be collapsed to:

frame->SetTimestamp(std::min(
    duration, get_time_cb_.Run() + kLastFrameDuration()));

?

[acolwell GONE FROM CHROMIUM, 2012/08/07 21:10:28]

I'm pretty sure the only way to get kNoTimestamp is if the first frame we get is
an EOS which means current_frame_ will never get set. This will trigger the
logic on line 220 and it get to line 236. Skipping sleeping would also be ok in
this case since we never got any valid frames to display so there is no need to
wait.
You can't use get_time_cb_ in the normal cases because ready_frames_ is
typically 4 frames deep and we shouldn't make any assumptions about how far
apart the frames are.

+  } else  if (frame->GetTimestamp() > duration) {

[DaleCurtis, 2012/08/07 20:32:14]

nit: extra space.

[acolwell GONE FROM CHROMIUM, 2012/08/07 21:10:28]

Done.

     frame->SetTimestamp(duration);
   }
 
   ready_frames_.push_back(frame);
   DCHECK_LE(NumFrames_Locked(), limits::kMaxVideoFrames);
   time_cb_.Run(frame->GetTimestamp());

[scherkus (not reviewing), 2012/08/07 20:56:47]

it's now possible to execute this w/ kNoTimestamp if (frame->IsEndOfStream() &&
ready_frames_.empty() && !curent_frame_)

is that OK?

[acolwell GONE FROM CHROMIUM, 2012/08/07 21:10:28]

Whoops..No it isnt' Changed it to set the time to duration on EOS and added a
DCHECK to make sure a kNoTimestamp() doesn't sneak in.

   frame_available_.Signal();
 }
 
 void VideoRendererBase::AttemptRead_Locked() {
   lock_.AssertAcquired();
   DCHECK_NE(kEnded, state_);
 
   if (pending_read_ ||
       NumFrames_Locked() == limits::kMaxVideoFrames ||
       (!ready_frames_.empty() && ready_frames_.back()->IsEndOfStream()) ||
@@ skipping 53 matching lines @@
 
 int VideoRendererBase::NumFrames_Locked() const {
   lock_.AssertAcquired();
   int outstanding_frames =
       (current_frame_ ? 1 : 0) + (last_available_frame_ ? 1 : 0) +
       (current_frame_ && (current_frame_ == last_available_frame_) ? -1 : 0);
   return ready_frames_.size() + outstanding_frames;
 }
 
 }  // namespace media