Using subdecoder discarded state for decoder selection conflicts with buffered ranges reported via SourceBuffer

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

69.50 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
+++ This bug was initially created as a clone of Bug #1058428 +++

Since the patch in bug 1058428 landed, spinning off a new bug for the issue mentioned in bug 1058428 comment 2.

The concept of IsDiscarded() is ultimately flawed.  We are ignoring discarded buffers when seeking and switching decoders, but reporting the same buffers as contributing to the SourceBuffer's track buffer via GetBuffered.
Assignee

Updated

5 years ago
Summary: MediaSourceReader::DecodersContainTime is busted → Using subdecoder discarded state for decoder selection conflicts with buffered ranges reported via SourceBuffer
Assignee

Comment 2

5 years ago
Posted patch p2: Seeking fixes. (obsolete) — Splinter Review
Assignee

Comment 3

5 years ago
The remaining problem is that RequestVideoData is called with aTimeThreshold == 0 when mState == SEEKING in the MDSM.  This causes us to switch (back) to the wrong reader.  Hack around by only switching readers on seek or at EOS.
Assignee

Updated

5 years ago
Attachment #8481052 - Attachment description: p1: Introduce abstraction to manager mapping between SourceBuffers and SourceBufferDecoders for the MediaSourceReader. → p1: Introduce abstraction to manage mapping between SourceBuffers and SourceBufferDecoders for the MediaSourceReader.
Assignee

Comment 4

5 years ago
Attachment #8481054 - Attachment is obsolete: true
Assignee

Comment 5

5 years ago
Posted patch p4: More fixes. (obsolete) — Splinter Review
Assignee

Updated

5 years ago
Attachment #8481092 - Attachment description: More fixes. → p4: More fixes.
Assignee

Comment 6

5 years ago
Note: this ended up overlapping with Karl's work in bug 1041396, so we'll need to reconcile both approaches, likely drawing most of the initialization timing stuff from Karl's patch since he has spent more time analyzing the problem and formulating a complete solution.
The bits that overlap including the initialization timing are very similar, the key difference being that here the initialization is determined on the main thread while in bug 1041396 it is on the decode task queue.

Longer term, we need to think about where we want demuxing and segment analysis to happen, but for now go with what is going to get something working faster.

Updated

5 years ago
Blocks: 1062023
Assignee

Comment 8

5 years ago
Comment on attachment 8481052 [details] [diff] [review]
p1: Introduce abstraction to manage mapping between SourceBuffers and SourceBufferDecoders for the MediaSourceReader.

Rebased patch coming up.
Attachment #8481052 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8481053 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8481090 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8481092 - Attachment is obsolete: true
Assignee

Comment 9

5 years ago
Rolls up p1-4, rebases against inbound, includes some additional fixes:

Store last audio/video timestamps and use them to switch readers rather than
mTimeThreshold (which is the playback currentTime, and therefore somewhere
behind the decode position).  Sample duration is wrong for last frame - see
bug 1062055.

Try to use mTimeThreshold, mDrop{Audio,Video} appropriately, but this needs
closer inspection.

Calls ResetDecode early to clear EOS on subdecoder's queues to avoid
SwitchReaders ignoring previously EOSed streams if we want to seek into
them.

Skip EOSed decoders in SwitchReader; needs fix for slightly non-overlapped
target time resulting in no decoder selection and early EOS.

IsWaitingMediaResources/NotifyWaitingForResourcesStatusChanged is waiting
for decoder initialization which might be slow (e.g. blocked waiting for
additional appends), should perhaps be triggered by the TrackBuffer's
SourceBuffer receving the init segment in an append.

There's also the long standing issue here that the subreaders are trying to
read the first sample after the init segment before ReadMetadata succeeds,
so they will block until sufficient data *beyond* the init segment has been
appended.  This may need to be addressed.

TrackBuffer.h needs a bit of documentation.
Assignee

Comment 11

5 years ago
Minor fixes, TrackBuffer.h documented.
Attachment #8483376 - Flags: review?(cajbir.bugzilla)
Assignee

Updated

5 years ago
Attachment #8483274 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8483278 - Attachment is obsolete: true

Comment 12

5 years ago
Comment on attachment 8483376 [details] [diff] [review]
Introduce abstraction to manager mapping between SourceBuffers and SourceBufferDecoders for the MediaSourceReader.

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

::: content/media/mediasource/TrackBuffer.cpp
@@ +129,5 @@
> +  // XXX check default if mDecoders empty?
> +  double highestEndTime = 0;
> +
> +  // TODO: Need to adjust mDecoders so it only tracks active decoders.
> +  // Once we have an abstraction for track buffers, this needs to report the

Does this comment need to be adjusted now that we have that abstraction?

@@ +184,5 @@
> +            this, aDecoder.get(), reader);
> +
> +  MediaInfo mi;
> +  nsAutoPtr<MetadataTags> tags; // TODO: Handle metadata.
> +  nsresult rv = reader->ReadMetadata(&mi, getter_Transfers(tags));

Can we assert that the parent decoders monitor isn't held here since ReadMetadata potentially blocks?

::: content/media/mediasource/TrackBuffer.h
@@ +115,5 @@
> +  double mLastStartTimestamp;
> +  double mLastEndTimestamp;
> +
> +  // Set when the first decoder used by this TrackBuffer is initialized.
> +  bool mHasAudio;

Some comment of what members need to be protected by the parent decoders monitor would be useful - I see that it's done in some of the methods of the implementation.
Attachment #8483376 - Flags: review?(cajbir.bugzilla) → review+
Assignee

Comment 16

5 years ago
Sorry!  I always build with warnings-as-errors before pushing, but a clean inbound was breaking due to warnings for me so I had it disabled for this build/push cycle.  Pushed again with bustage fixes.
Assignee

Updated

5 years ago
Depends on: 1062347
Assignee

Comment 18

5 years ago
(In reply to Matthew Gregan [:kinetik] from comment #17)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8808324ba834

This change causes some weirdness with the test; pushed a test tweak to avoid it for now so that patch sticks.  I'll chase up the test issue in bug 1062347.
Assignee

Comment 20

5 years ago
(In reply to Matthew Gregan [:kinetik] from comment #19)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3a343c27fc7a

Totally dumb bug, !x||!y instead of !x&&!y.  Fixed, and test change reverted (fixing bug 1062347).
Assignee

Updated

5 years ago
No longer depends on: 1062347

Updated

5 years ago
Blocks: 1041374

Updated

5 years ago
Blocks: 1040552
Assignee

Updated

5 years ago
Blocks: 1062661
Assignee

Updated

5 years ago
Blocks: 1062664
Assignee

Updated

5 years ago
Blocks: 1062669
Assignee

Comment 23

5 years ago
Patch that was landed, including the build bustage fix.  Carrying forward r+
Attachment #8483376 - Attachment is obsolete: true
Attachment #8483935 - Flags: review+
Assignee

Comment 24

5 years ago
Posted patch Fix test leak/shutdown hang. (obsolete) — Splinter Review
Both of the inbound failures seem to be the same root cause, this fixes it
locally.  Pushed to try to be sure.

I'll land this rolled into the main patch, posting diff for easier review.
Attachment #8483938 - Flags: review?(cajbir.bugzilla)

Updated

5 years ago
Attachment #8483938 - Flags: review?(cajbir.bugzilla) → review+
Assignee

Comment 25

5 years ago
Karl ran into a case this change broken, where the init segment and media
segment were appended separately but with very little delay between them.
This exposed the existing definition of HasInitSegment was incorrect -- it
reported when a decoder had initialized, so it was possible to have an init
segment but not have the decoder initialized.  This would cause
MSR::ReadMetadata to fail due to there being zero tracks configured.

Posted separately for ease of review, but I'll roll it into the main patch
for landing.
Assignee

Updated

5 years ago
Attachment #8483982 - Flags: review?(cajbir.bugzilla)

Updated

5 years ago
Attachment #8483982 - Flags: review?(cajbir.bugzilla) → review+
Assignee

Updated

5 years ago
Depends on: 1062733
Assignee

Comment 27

5 years ago
Rolled up patch to land, green on try.
Attachment #8483935 - Attachment is obsolete: true
Attachment #8483938 - Attachment is obsolete: true
Attachment #8483982 - Attachment is obsolete: true
Attachment #8484011 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8a4df73f1ab8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Duplicate of this bug: 1041396
Assignee

Updated

5 years ago
Duplicate of this bug: 1061400

Updated

5 years ago
Depends on: 1067785
You need to log in before you can comment on or make changes to this bug.