1064113 - MediaSource::EndOfStream occurring during seek causes assertion

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

If we are in this loop in MediaSourceReader::Seek:

  while (!TrackBuffersContainTime(target) && !IsShutdown() && !IsEnded()) {
    MSE_DEBUG("MediaSourceReader(%p)::Seek waiting for target=%f", this, target);
    static_cast<MediaSourceDecoder*>(mDecoder)->WaitForData();
  }

And MediaSource::EndOfStream happens such that IsEnded() returns true, we hit the following assertion laster in the seek call:


    MOZ_ASSERT(ok && static_cast<SourceBufferDecoder*>(mAudioReader->GetDecoder())->ContainsTime(target));

Because the loop exited due to IsEnded() we do not necessarily have the target time in the reader.

Comment 1

[cajbir (:cajbir)]

Attachment: Test cast

Comment 2

[cajbir (:cajbir)]

When a seek is in progress and MediaSource::EndOfStream() is called the behaviour should be to keep the seek request in progress. If data is appended later that is for the range the seek is waiting then the seek resumes.

Comment 3

[cajbir (:cajbir)]

When a seek is waiting for data and new data is appended the notiftifcation of this data occurs before the decoder is registered as an initialized decoder in the track buffer. As a result the seek continues waiting for data as it never gets a further notification.

Comment 4

[cajbir (:cajbir)]

Attachment: WIP fix

This fixes the assertion and the problem identified in comment 3. Behaviour still not conformant to that described in comment 2 though.

Comment 5

[cajbir (:cajbir)]

Bug 1064160 has the same effective fix for comment 3 so the fix here will need to be removed in the attached patch.

Comment 6

[Matthew Gregan [:kinetik]]

Ah sorry, didn't see you had made similar changes to the wait/notify stuff.

(In reply to cajbir (:cajbir) from comment #2)
> When a seek is in progress and MediaSource::EndOfStream() is called the
> behaviour should be to keep the seek request in progress. If data is
> appended later that is for the range the seek is waiting then the seek
> resumes.

If I understand correctly, we can't really ever use IsEnded() because it's always possible to reopen the MediaSource by appending new data.  That may be problematic for the use in On{Audio,Video}EOS too.

There's a bit of other work necessary to support reopening MediaSource objects too, so a complete fix will require modifications to other areas of the code.

Comment 7

[cajbir (:cajbir)]

(In reply to Matthew Gregan [:kinetik] from comment #6)
> 
> If I understand correctly, we can't really ever use IsEnded() because it's
> always possible to reopen the MediaSource by appending new data.  That may
> be problematic for the use in On{Audio,Video}EOS too.
> 
> There's a bit of other work necessary to support reopening MediaSource
> objects too, so a complete fix will require modifications to other areas of
> the code.

Right, that's what I'm currently doing. Just returning the IsEnded check in the seek loop prevents the assertion but there's other IsEnded usage as you note.

Comment 8

[cajbir (:cajbir)]

Attachment: p1: Testing appendData after an endOfStream

Adds a test for the simple case of an appendBuffer after an endOfStream. This triggers bug 1065221 but the test still passes beyond that as noted in that bug report.

Comment 9

[cajbir (:cajbir)]

Attachment: p2: Adds test for endOfStream() while seek is in progress

This refactors the previous test case to work with recent test changes. An endOfStream() occurs during a seek.

This currently triggers the assertion in this bug.

It should result in the seek continuing and playing through to the end of the file.

With a fix for the asssertion that test case still fails as the playback does not resume after the seek.

Comment 10

[cajbir (:cajbir)]

Bug 1065218 removes the assertions that are triggered by testcase in attachment 8486963 [details] [diff] [review]. The remaining errors occur as outlined in comment 9. I'll rename this bug if that one lands.

Comment 11

[cajbir (:cajbir)]

Comment on attachment 8486937 [details] [diff] [review]
p1: Testing appendData after an endOfStream

Spinning this test case out into bug 1068483 as it now fails.

Comment 12

[cajbir (:cajbir)]

Assertion was fixed in other patch landing/refactoring. Dealing with the problem mentioned in comment 6 is moved to bug 1068483.