New issue
Advanced search Search tips

Issue 597447 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Make MSE ChunkDemuxer OnNewConfigs+OnSourceInitDone less fragile

Project Member Reported by wolenetz@chromium.org, Mar 23 2016

Issue description

Currently, we only check that audio_ and video_ ChunkDemuxerStreams have been created (if they are expected, based on the [set of] mimetypes used across all currently added SourceBuffers) to gate whether or not a particular stream parser's signalling of init_cb_ means we've got all the initial initialization segments from all the sourcebuffers that we are expecting.

This is a little confusing (especially once we get into multi-track A or V or text-only sourcebuffers) and perhaps leaves a hole where a non-A/V SourceBuffer (such as a text-only webm stream) might trigger repeated transitions to INITIALIZED in OnSourceInitDone, if it's initialization segment is parsed after all the other expected ones. Further, if there are multiple text-only SourceBuffers, this condition could be worsened.

Fixing this bug will likely entail adding an explicit "expected_initialized_source_states" counter that is incremented on successful stream parser creation and decremented in OnSourceInitDone. Once back to 0 in OnSourceInitDone(), we can proceed to INITIALIZED (and HAVE_METADATA).
 
Summary: Make MSE ChunkDemuxer OnNewConfigs+OnSourceInitDone less fragile (was: Strengthen ChunkDemuxer to not become INITIALIZED until precisely the expected number of OnSourceInitDone's occur)
See also https://codereview.chromium.org/1826583003/diff/1/media/filters/chunk_demuxer.cc#newcode972

My comment reply there expands the scope of this bug a little:
Specifically, changing MediaSourceState::OnSourceInitDone to be automatically run *by MediaSourceState::OnNewConfigs* would help reduce fragility of parsers potentially not doing the right sequencing of signalling new_configs_cb and init_cb.
Labels: MSEscrubbed
We've been protected to date by a couple of things:
1) DCHECK() on ChunkDemuxer state (INITIALIZING) in OnSourceInitDone()
2) We don't allow text-only sourcebuffers. We allow maximum one each of audio and video tracks across all sourcebuffers, gated at AddId().
Regardless, this sounds like a good cleanup to do, if only to make less fragile once we go multi-track each for A/V.
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe1e90eeacdc473325519df9c556d1cffa5db3a0

commit fe1e90eeacdc473325519df9c556d1cffa5db3a0
Author: wolenetz <wolenetz@chromium.org>
Date: Wed Mar 30 20:47:35 2016

MSE: Protect better against reaching HAVE_METADATA too early

Adds a counter of expected ChunkDemuxer::OnSourceInitDone() calls and
uses it to gate when to transition to INITIALIZED. Reduces fragility and
improves readability. This corresponds to the first part of fixing bug
597447. Refactoring to enforce StreamParsers' InitCB happens after
one or more ConfigCB, and occurs at most once per parser, is in a
later CL (https://codereview.chromium.org/1843823003/).

BUG= 597447 

Review URL: https://codereview.chromium.org/1839763005

Cr-Commit-Position: refs/heads/master@{#384085}

[modify] https://crrev.com/fe1e90eeacdc473325519df9c556d1cffa5db3a0/media/filters/chunk_demuxer.cc
[modify] https://crrev.com/fe1e90eeacdc473325519df9c556d1cffa5db3a0/media/filters/chunk_demuxer.h

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c42c49d49f2edfc5b2681694685c712f03f02fd

commit 1c42c49d49f2edfc5b2681694685c712f03f02fd
Author: wolenetz <wolenetz@chromium.org>
Date: Wed Mar 30 20:51:00 2016

MSE: Protect better against out-of-sequence parser callbacks

Adds parser initialization state tracking to MediaSourceState and
enforces correct sequencing of parser callbacks with DCHECKs.

BUG= 597447 

Review URL: https://codereview.chromium.org/1843823003

Cr-Commit-Position: refs/heads/master@{#384087}

[modify] https://crrev.com/1c42c49d49f2edfc5b2681694685c712f03f02fd/media/filters/media_source_state.cc
[modify] https://crrev.com/1c42c49d49f2edfc5b2681694685c712f03f02fd/media/filters/media_source_state.h

Labels: M-51
Status: Fixed (was: Started)

Sign in to add a comment