1005727 - Ogg seeking broken when first sample after seek is keyframe

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: async-seek-fail.html

Ogg seeking is broken when when you seek 320x240.ogg to duration/4 (as test_bug493187.html does).

Observed behaviour is that we seek to t=duration/4 and display the frame there, but when playback starts the current time immediately jumps to the end.

Testcase attached.

This is because the "Decode forwards until we find the next keyframe" loop at OggReader.cpp:1389 doesn't check to see if it's already reached a keyframe before decoding forward, i.e. if we've already found a keyframe, we'll skip the decode forward until the *next* keyframe, and in this file there is only 1 keyframe, so we skip to the end.

Patch to follow.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

When we skip the decode up to the next keyframe after Ogg seeking (to skip non keyframe frames that may appear in an Ogg page containing a keyframe) we should first actually check to see if we've already for a keyframe enqueued in our MediaQueue. There may be frames in the queue because after seeking we decode the first frame to check if it's a keyframe, and if not we seek again back to the preceding keyframe.

Comment 2

[cajbir (:cajbir)]

Comment on attachment 8417131 [details] [diff] [review]
Patch

Review of attachment 8417131 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/ogg/OggReader.cpp
@@ +1389,5 @@
> +    // First, we must check to see if there's already a keyframe in the frames
> +    // that we may have already decoded, and discard frames up to the
> +    // keyframe.
> +    bool foundKeyframe = false;
> +    while (VideoQueue().GetSize() && !foundKeyframe) {

Do the !foundKeyframe test first to avoid the method call if it's true. Make the GetSize() test be "GetSize() > 0" to better reflect the return type of 'GetSize()' (it's an int32_t not a bool and makes reading what the while loop is doing slightly harder).

Why does this use VideoQueue() but the body uses mVideoQueue?

Could this be done more concisely to avoid the 'foundKeyframe' flag and injection of variables into method scope? Something like:

  while ((v = mVideoQueue.PeekFront()) && !v->mKeyframe) 
    delete mVideoQueue.PopFront();

You could put a GetSize() in there if you don't want to rely on PeekFront return nullptr on an empty queue.

@@ +1398,3 @@
>        }
>      }
> +    if (!foundKeyframe) {

This test could be replaced with a 'mVideoQueue.GetSize() > 0' to remove the 'foundKeyframe' flag.

@@ +1405,5 @@
> +        ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +        if (mDecoder->IsShutdown()) {
> +          return NS_ERROR_FAILURE;
> +        }
> +      }

This loop doesn't do the IsShutdown() check if we find the keyframe on the first call. Is that important?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to cajbir (:cajbir) from comment #2)
> Comment on attachment 8417131 [details] [diff] [review]

> Could this be done more concisely to avoid the 'foundKeyframe' flag and
> injection of variables into method scope? Something like:
> 
>   while ((v = mVideoQueue.PeekFront()) && !v->mKeyframe) 
>     delete mVideoQueue.PopFront();

Yes! That's shorter, thanks.


> @@ +1405,5 @@
> > +        ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> > +        if (mDecoder->IsShutdown()) {
> > +          return NS_ERROR_FAILURE;
> > +        }
> > +      }
> 
> This loop doesn't do the IsShutdown() check if we find the keyframe on the
> first call. Is that important?

Shouldn't matter. Control will return MediaDecoderStateMachine::DecodeSeek(), and that will refrain from scheduling more decodes if we're in shutdown state.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Review comments addressed, and landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6535f63e9453

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6535f63e9453