Closed Bug 1064113 Opened 6 years ago Closed 6 years ago

MediaSource::EndOfStream occurring during seek causes assertion

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cajbir, Assigned: cajbir)

References

(Blocks 1 open bug)

Details

Attachments

(4 obsolete files)

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.
Blocks: MSE, 1056440
Attached file Test cast (obsolete) —
Assignee: nobody → cajbir.bugzilla
Status: NEW → ASSIGNED
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.
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.
Attached patch WIP fix (obsolete) — Splinter Review
This fixes the assertion and the problem identified in comment 3. Behaviour still not conformant to that described in comment 2 though.
Bug 1064160 has the same effective fix for comment 3 so the fix here will need to be removed in the attached patch.
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.
(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.
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.
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.
Attachment #8485560 - Attachment is obsolete: true
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 on attachment 8486937 [details] [diff] [review]
p1: Testing appendData after an endOfStream

Spinning this test case out into bug 1068483 as it now fails.
Attachment #8486937 - Attachment is obsolete: true
Attachment #8485583 - Attachment is obsolete: true
Attachment #8486963 - Attachment is obsolete: true
Assertion was fixed in other patch landing/refactoring. Dealing with the problem mentioned in comment 6 is moved to bug 1068483.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.