1041396 - Video displays as all white intermittently in MSE YouTube player

Status: RESOLVED DUPLICATE of bug 1059058

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

When playing the feelings_vp9 YouTube demo video it occasionally shows the video as all white rather than the actual video.

Steps to Reproduce:

1) Enable MSE
2) Visit http://cd.pn/mse/ytdemo/dash-player.html?url=http://cd.pn/mse/ytdemo/feelings_vp9-20130806-manifest.mpd
3) Wait for playback to start

Expected Result

4) Video plays and shows in the video area

Actual Result

5) Sometimes video plays. Press refresh on the page. Intermittently (1 in 3 times or so) the video area shows as all white.

Comment 1

[Matthew Gregan [:kinetik]]

Can you please verify that this isn't caused by OMTC being enabled on Linux?  See https://hg.mozilla.org/mozilla-central/rev/ea2341b06e6f

Comment 2

[Djfe]

For me the whole video is white as well, the audio track is played back but I don't see a video.

Same thing happens on YouTube, just a black background, because the parent div has a black background:
.html5-video-container {
    background: none no-repeat scroll center center rgb(0, 0, 0);
}

If I use the Firefox Addon YouTubeCenter and disable DASH, it will show the video again, but with the option DASH enabled, or YTC (addon) completely disabled, the video is not there.

MSE is enabled, https://youtube.com/html5 shows all 5/6 enabled (except MSE + x264, which is expected behaviour)

I'm using Firefox 31.0 and Win7 32bit.

Comment 3

[Djfe]

Attachment: YouTube black video

Video stays transparent. Audio plays fine.

Comment 4

[Karl Tomlinson (:karlt)]

It reproduces on nightlies after bug 881512 landed, so there is no evidence to indicate that this is a regression.

Comment 5

[Karl Tomlinson (:karlt)]

This happens when MaybeFinishDecodeMetadata() is called when IsVideoDecoding() returns false, because HasVideo() is false.  I guess that's because the video hasn't been added yet.

Comment 6

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

That sounds hard to reliably fix unless there's some way to either tell when script has finished adding new decoders to the MSEDecoder, or delay the state machine exiting DECODER_STATE_DECODING_METADATA state until we can be reasonably certain HasVideo() and HasAudio() are stable.

Comment 7

[Karl Tomlinson (:karlt)]

The spec for MediaSource::addSourceBuffer() says

"Implementations must support at least 1 MediaSource object with the following SourceBuffer configurations. MediaSource objects must support each of the configurations below, but they are only required to support one configuration at a time. Supporting multiple configurations at once or additional configurations is a quality of implementation issue.

    A single SourceBuffer with 1 audio track and/or 1 video track.

    Two SourceBuffers with one handling a single audio track and the other
    handling a single video track."

"If the user agent can't handle any more SourceBuffer objects then throw a
 QUOTA_EXCEEDED_ERR exception and abort these steps.

   Note

   For example, a user agent may throw a QUOTA_EXCEEDED_ERR exception if the
   media element has reached the HAVE_METADATA readyState. This can occur if
   the user agent's media engine does not support adding more tracks during
   playback."

So the browser doesn't need to support adding a second SourceBuffer after
playback has begun.

In this demo, both audio and video SourceBuffers are added in a single task,
before any appendBuffer() happens, so when the browser runs the buffer append
algorithm, it knows that there is both video and audio.  HAVE_METADATA
readyState will not be reached before the browser knows that there are two
SourceBuffers.

When handling the second SourceBuffer appended in a different task, thread
communication may get more complicated because the decoder/playback threads
can't assume the HAVE_METADATA state has been reached until the MediaSource
(main) thread is in HAVE_METADATA readyState (so that it can throw the
appropriate exception).

Also, when appendBuffer() happens in the same task as addSourceBuffer(),
the append buffer algorithm must happen "asynchronously", but I don't know
whether that is necessarily *after* the current task completes.  If it can
happen concurrently, then I guess a subsequent addSourceBuffer() call in
the same task could fail non-deterministically.

Comment 8

[Djfe]

(In reply to Chris Pearce (:cpearce) from comment #6)
> That sounds hard to reliably fix unless there's some way to either tell when
> script has finished adding new decoders to the MSEDecoder, or delay the
> state machine exiting DECODER_STATE_DECODING_METADATA state until we can be
> reasonably certain HasVideo() and HasAudio() are stable.

There has to be a way since it works reliably in Chrome...

Hopefully someone finds a way to fix Mozilla's implementation of MSE in this regard ;)

Comment 9

[Karl Tomlinson (:karlt)]

The order of main thread tasks run here is.

1. a. audioSB = msrc.addSourceBuffer();
   b. videoSB = msrc.addSourceBuffer();

2. audioSB.appendBuffer(audioArray1);

3. videoSB.appendBuffer(video1Array1);

4. audioSB.appendBuffer(audioArray2);

5. videoSB.abort();

6. videoSB.appendBuffer(video2Array1);

7. videoSB.appendBuffer(video2Array2);

Expected behavior if the first arrays for each stream contain entire
initialization segments:

  readyState is set to HAVE_METADATA on the media element between 3 and 4, and
  both audio and video are available.

Expected behavior if the first arrays for each stream contain only part of the
initialization segments and the second arrays contain the remainder:

  readyState is set to HAVE_METADATA on the media element, immediately after 7,
  and both audio and video are available.

Actual behavior:

  readyState is set to HAVE_METADATA on the media element after 5, and only
  audio is available.

  Even after 7, only audio is available.

Actual behavior in more detail:

  After 1a, an audio SubBufferDecoder is added to the MediaSourceReader's
  pending decoders, and a decoder task begins and blocks in ReadMetadata() on
  the audio SubBufferDecoder's reader.

  After 1b, the first video SubBufferDecoder is added to the
  MediaSourceReader's pending decoders.

  After 4, the audio reader's ReadMetadata() completes successfully and so the
  MediaSourceReader has audio.

  The decoder task then blocks in ReadMetadata() on the first video
  SubBufferDecoder's reader.

  After 5, the video reader's ReadMetadata unblocks and returns an error.

  Another decoder task runs ReadMetadata() on the (parent) MediaSourceReader
  which indicates that audio is present but not video. 

  After 6, a second video SubBufferDecoder is added to the
  MediaSourceReader's pending decoders.

Comment 10

[Karl Tomlinson (:karlt)]

A quick hack to fix this might be to add another SubBufferDecoder for the
video SourceBuffer on abort, before unblocking the ReadMetadata() on the video reader.

That would still leave race conditions between another addSourceBuffer() on
the main thread and the decoder task queue completing ReadMetadata on each
stream.  That's not a new race though, and perhaps the race would not be so
bad if addSourceBuffer after HAVE_METADATA could add video after audio has
started.

What would be most unfortunate about the fix would be that it relies on
ReadMetadata's blocking behavior.  If we make ReadMetadata non-blocking, which
would seem preferable for purposes of not blocking the decoder task queue
until sufficient data arrives, then we have to fix this bug again.

Comment 11

[Karl Tomlinson (:karlt)]

Currently ContainerParser in SourceBuffer.cpp and ReadMetadata() on the
readers are trying to do much the same thing but getting different answers.

ContainerParser would need to do the same work as ReadMetadata() on the
readers to know when all SourceBuffers have their first initialization segment
and so readyState can be set to HAVE_METADATA and
MediaSourceReader::HaveMetaData() can return provide the necessary
tracks.

Currently ContainerParser can think it has an initialization segment when it
has a partial segment, and it dispatches "update" (instead of expected "error")
even if codecs are not supported or a later initialization segment is not
consistent with the first.

It seems that an ideal solution would be run a non-blocking ReadMetadata() on
the SubBufferDecoders' readers, on the main thread in the next stable state,
before giving the SubBufferDecoders to the (parent) MediaSourceReader (or some other signal) when all SourceBuffers have received their first initialization segments.

Comment 12

[Karl Tomlinson (:karlt)]

An alternative interim solution may be to teach MediaSourceReader about
SourceBuffers, so it can know when ReadMetadata() calls on SubBufferDecoder
readers have processed initialization segments for all SourceBuffers.

That would be a signficantly more complex interim solution than the quick hack of comment 10, and would need replacing with an ideal solution (comment 11 or otherwise) to give spec'ed behavior anyway, and so comment 10's solution seems the preferred approach, if we can depend on SubBufferDecoder reader ReadMetadata() blocking and that is not likely to change soon?

If ReadMetadata() blocking is guaranteed and not a problem, then the only advantage of this solution might be that addSourceBuffer() could be processed on the decoder task queue and so could have deterministic behavior wrt appendBuffer() calls (and resulting ReadMetadata() invocations).

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #11)
> It seems that an ideal solution would be run a non-blocking ReadMetadata() on
> the SubBufferDecoders' readers, on the main thread in the next stable state,
> before giving the SubBufferDecoders to the (parent) MediaSourceReader (or
> some other signal) when all SourceBuffers have received their first
> initialization segments.

(In reply to Karl Tomlinson (:karlt) from comment #12)
> An alternative interim solution may be to teach MediaSourceReader about
> SourceBuffers, so it can know when ReadMetadata() calls on SubBufferDecoder
> readers have processed initialization segments for all SourceBuffers.
> 
> That would be a signficantly more complex interim solution than the quick
> hack of comment 10, and would need replacing with an ideal solution (comment
> 11 or otherwise) to give spec'ed behavior anyway,

I'm probably reading too much into "asynchronously" in the appendBuffer steps
[1], assuming that it would need to run the buffer append in the next stable
state so as to have deterministic state from the initialization segment received algorithm [2] for the HAVE_METADATA decision quoted in comment 7.  However, we are not making that decision.

I guess "asynchronously" can be as long as it takes, which is why there is the
"updating" attribute state and the "updateend" event.

So perhaps the proposal in comment 12 would be a viable long term solution
even.  When we have a demuxer that can detect initialization segments, then
perhaps that can all run on the decoder task queue and we'll be able to
remove ContainerParser.

[1] http://www.w3.org/TR/media-source/#widl-SourceBuffer-appendBuffer-void-ArrayBufferView-data
[2] http://www.w3.org/TR/media-source/#sourcebuffer-init-segment-received

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: WIP

This adds a SourceBufferEngine object to track SourceBuffers on other threads.
In the future it can also proxy messages back to the SourceBuffer on its thread.

TODO: Determine what IsWaitingMediaResources() should return before calling NotifyWaitingForResourcesStatusChanged() from +SourceBufferEngine::InitNewDecoder() so that the result is deterministic wrt the timing of the IsWaitingMediaResources().

Currently buffered ranges increase to 20 on the url and the thumb moves as far as 0:05, and then playback stalls, but this often happens without the patches.

There is also a shutdown hang.  Need to determine whether that is related to this patch.

Comment 15

[Matthew Gregan [:kinetik]]

Bug 1059058 at least partially fixed this (apologies for stealing the fix, the pre-35 branch rush was tricky, the analysis in this bug was awesome), by separating decoder initialization (async) from registration of SourceBuffers with the MediaSourceReader (via the new off-main-thread safe TrackBuffer object), removing the race.  There's bug 1062669 to cover pinning down the precise ordering of which SourceBuffers are considered essential for initialization.

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: patch series

Yes, the changes in bug 1059058 fix the issue discovered here.

I'm attaching where I got to, in case we come back to this, but its probably not so useful with the other approach.  I suspect there are some differences in some odd situations such as removing source buffers, but that can be another bug another time.