Use midpoint calculation for frame-exact seeking.

Use midpoint calculation for frame-exact seeking.

Previously we would use a "good enough" frame without fully taking duration into account.

BUG=69499
TEST=media_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71676

[scherkus (not reviewing), 2011-01-15 01:53:44]

one line fix!

[acolwell GONE FROM CHROMIUM, 2011-01-15 04:56:21]

LGTM

http://codereview.chromium.org/6321005/diff/1/media/filters/video_renderer_ba...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/6321005/diff/1/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:123: for (int64 i = 0; seeking_;
++i) {
Can't you just bound this loop with i * kDuration < (timestamp + kDuration)? I
think then you could use NewExpectedCallback() instead of OnSeekComplete(). It
also allows the test to always terminate even if the renderer never calls the
callback.

[scherkus (not reviewing), 2011-01-18 18:49:23]

http://codereview.chromium.org/6321005/diff/1/media/filters/video_renderer_ba...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/6321005/diff/1/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:123: for (int64 i = 0; seeking_;
++i) {
On 2011/01/15 04:56:22, acolwell wrote:
> Can't you just bound this loop with i * kDuration < (timestamp + kDuration)? I
> think then you could use NewExpectedCallback() instead of OnSeekComplete(). It
> also allows the test to always terminate even if the renderer never calls the
> callback.

Tried this and turns out we can't because the test doesn't have knowledge of how
much VideoRendererBase like to preroll (i.e., it likes to grab a frame or two
after the timestamp) until it's done "seeking"

I'll add a comment

[acolwell GONE FROM CHROMIUM, 2011-01-18 19:07:09]

http://codereview.chromium.org/6321005/diff/1/media/filters/video_renderer_ba...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/6321005/diff/1/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:123: for (int64 i = 0; seeking_;
++i) {
What if you specify an actual clip duration and bound the loop with that? The
renderer should honor not trying to fetch frames past the end of the clip.

This isn't a blocker of course, but it might be good to check.

On 2011/01/18 18:49:27, scherkus wrote:
> On 2011/01/15 04:56:22, acolwell wrote:
> > Can't you just bound this loop with i * kDuration < (timestamp + kDuration)?
I
> > think then you could use NewExpectedCallback() instead of OnSeekComplete().
It
> > also allows the test to always terminate even if the renderer never calls
the
> > callback.
> 
> Tried this and turns out we can't because the test doesn't have knowledge of
how
> much VideoRendererBase like to preroll (i.e., it likes to grab a frame or two
> after the timestamp) until it's done "seeking"
> 
> I'll add a comment

[scherkus (not reviewing), 2011-01-18 19:19:12]

http://codereview.chromium.org/6321005/diff/1/media/filters/video_renderer_ba...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/6321005/diff/1/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:123: for (int64 i = 0; seeking_;
++i) {
That would end up queuing the duration of the clip in the video renderer, which
isn't quite what we want.

It's a bit crappy but a better alternative would be to have a simulated
VideoDecoder instead of MockVideoDecoder (i.e., the simulated one would properly
generate the frames and seek and flush instead of us trying to simulate it via
gmock).

But then this is part of my bigger idea/plan where we can keep the
seek/flush/decode logic in one place, then drop in different decoders (i.e., for
testing we can drop in a decoder that doesn't really decode) 

On 2011/01/18 19:07:09, acolwell wrote:
> What if you specify an actual clip duration and bound the loop with that? The
> renderer should honor not trying to fetch frames past the end of the clip.
> 
> This isn't a blocker of course, but it might be good to check.
> 
> On 2011/01/18 18:49:27, scherkus wrote:
> > On 2011/01/15 04:56:22, acolwell wrote:
> > > Can't you just bound this loop with i * kDuration < (timestamp +
kDuration)?
> I
> > > think then you could use NewExpectedCallback() instead of
OnSeekComplete().
> It
> > > also allows the test to always terminate even if the renderer never calls
> the
> > > callback.
> > 
> > Tried this and turns out we can't because the test doesn't have knowledge of
> how
> > much VideoRendererBase like to preroll (i.e., it likes to grab a frame or
two
> > after the timestamp) until it's done "seeking"
> > 
> > I'll add a comment
>

[acolwell GONE FROM CHROMIUM, 2011-01-18 19:26:38]

http://codereview.chromium.org/6321005/diff/1/media/filters/video_renderer_ba...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/6321005/diff/1/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:123: for (int64 i = 0; seeking_;
++i) {
Got it. Sounds like something for a later CL. Looks good to me.
 
On 2011/01/18 19:19:12, scherkus wrote:
> That would end up queuing the duration of the clip in the video renderer,
which
> isn't quite what we want.
> 
> It's a bit crappy but a better alternative would be to have a simulated
> VideoDecoder instead of MockVideoDecoder (i.e., the simulated one would
properly
> generate the frames and seek and flush instead of us trying to simulate it via
> gmock).
> 
> But then this is part of my bigger idea/plan where we can keep the
> seek/flush/decode logic in one place, then drop in different decoders (i.e.,
for
> testing we can drop in a decoder that doesn't really decode) 
> 
> On 2011/01/18 19:07:09, acolwell wrote:
> > What if you specify an actual clip duration and bound the loop with that?
The
> > renderer should honor not trying to fetch frames past the end of the clip.
> > 
> > This isn't a blocker of course, but it might be good to check.
> > 
> > On 2011/01/18 18:49:27, scherkus wrote:
> > > On 2011/01/15 04:56:22, acolwell wrote:
> > > > Can't you just bound this loop with i * kDuration < (timestamp +
> kDuration)?
> > I
> > > > think then you could use NewExpectedCallback() instead of
> OnSeekComplete().
> > It
> > > > also allows the test to always terminate even if the renderer never
calls
> > the
> > > > callback.
> > > 
> > > Tried this and turns out we can't because the test doesn't have knowledge
of
> > how
> > > much VideoRendererBase like to preroll (i.e., it likes to grab a frame or
> two
> > > after the timestamp) until it's done "seeking"
> > > 
> > > I'll add a comment
> > 
>