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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

+++ 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.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: p1: Introduce abstraction to manage mapping between SourceBuffers and SourceBufferDecoders for the MediaSourceReader.

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: p2: Seeking fixes.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: p3: Hack (not to commit as-is)

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.

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: p3: Handle aTimeThreshold == 0 RequestVideoData when seeking.

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: p4: More fixes.

Comment 6

[Matthew Gregan [:kinetik]]

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.

Comment 7

[Karl Tomlinson (:karlt)]

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.

Comment 8

[Matthew Gregan [:kinetik]]

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

Rebased patch coming up.

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: wip: Introduce abstraction to manager mapping between SourceBuffers and SourceBufferDecoders for the MediaSourceReader.

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.

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: fixup! Bug 1059058 - Introduce abstraction to manager mapping between SourceBuffers and SourceBufferDecoders for the MediaSourceReader.

Forgot this change.

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: Introduce abstraction to manager mapping between SourceBuffers and SourceBufferDecoders for the MediaSourceReader.

Minor fixes, TrackBuffer.h documented.

Comment 12

[cajbir (:cajbir)]

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.

Comment 13

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ecc7e71689

Comment 14

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=47316907&tree=Mozilla-Inbound

Comment 15

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f0afffca59

Comment 16

[Matthew Gregan [:kinetik]]

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.

Comment 17

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8808324ba834

Comment 18

[Matthew Gregan [:kinetik]]

(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.

Comment 19

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a343c27fc7a

Comment 20

[Matthew Gregan [:kinetik]]

(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).

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out for ongoing test failures and leaks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3e6c741c46

https://tbpl.mozilla.org/php/getParsedLog.php?id=47331955&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=47332124&tree=Mozilla-Inbound

Comment 22

[Matthew Gregan [:kinetik]]

https://tbpl.mozilla.org/?tree=Try&rev=58191dcba40d

Comment 23

[Matthew Gregan [:kinetik]]

Attachment: Introduce abstraction to manager mapping between SourceBuffers and SourceBufferDecoders for the MediaSourceReader.

Patch that was landed, including the build bustage fix.  Carrying forward r+

Comment 24

[Matthew Gregan [:kinetik]]

Attachment: Fix test leak/shutdown hang.

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.

Comment 25

[Matthew Gregan [:kinetik]]

Attachment: Handle separate init/media segment appends.

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.

Comment 26

[Matthew Gregan [:kinetik]]

https://tbpl.mozilla.org/?tree=Try&rev=97fe8cddbc3f

Comment 27

[Matthew Gregan [:kinetik]]

Attachment: bug1059058.patch

Rolled up patch to land, green on try.

Comment 28

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a4df73f1ab8

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a4df73f1ab8