Commit c31f15ed authored by Matt Wolenetz's avatar Matt Wolenetz Committed by Commit Bot

WebMediaPlayerImpl: err if no A/V in ::OnMetadata

MSE worker thread termination or worker removal of SourceBuffers can
race the thread hopping of the OnMetadata signal. If MSE track removals
win the race such that pipeline's reported metadata contains no audio or
video, then when handling that metadata in WMPI, this change issues
player load error instead of transitioning to HAVE_METADATA. This fixes
a crash resulting otherwise because transition to HAVE_METADATA without
MSE pipeline metadata having any A/V previously enabled beginning
playback (in WMPI::Play()) without having a WatchTimeReporter.

I tried reproducing similar failure first using MSE from the main
thread (not MSE-in-Workers) and trying to get removeSourceBuffer to
occur precisely in time to win the race, but was unable to get a repro
with a bit of trying. However, I still suspect this issue could have
pre-existed MSE-in-Workers.

The change is not specific to MSE, since WMPI should prevent successful
playback start if the resource has neither audio nor video metadata.

This change also includes a test note to add a feature to MSE-in-Workers
(feature-detection of MSE-in-Workers support from main/Window context)
and to use it to deflake the test itself which could flakily fail (not
crash as is fixed by this change) on implementations that do not support
MSE-in-Workers yet.

BUG=878133,1139854

TEST=Updated WebMediaPlayerImplTest.NoStreams (blink_media_unittests),
and manually verified on linux locally: flaky crash appears fixed for
.../dedicated-worker/mediasource-worker-play-terminate-worker.html

Change-Id: I8a9d8428417555089c8b09f4bc1e19849bbe0162
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2486074Reviewed-by: default avatarDale Curtis <dalecurtis@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818731}
parent 195d80d7
......@@ -2021,6 +2021,22 @@ void WebMediaPlayerImpl::OnMetadata(const PipelineMetadata& metadata) {
delegate_id_, delegate_has_audio_, HasVideo(),
DurationToMediaContentType(GetPipelineMediaDuration()));
// It could happen that the demuxer successfully completed initialization
// (implying it had determined media metadata), but then removed all audio and
// video streams and the ability to demux any A/V before |metadata| was
// constructed and passed to us. One example is, with MSE-in-Workers, the
// worker owning the MediaSource could have been terminated, or the app could
// have explicitly removed all A/V SourceBuffers. That termination/removal
// could race the construction of |metadata|. Regardless of load-type, we
// shouldn't allow playback of a resource that has neither audio nor video.
// We treat lack of A/V as if there were an error in the demuxer before
// reaching HAVE_METADATA.
if (!HasVideo() && !HasAudio()) {
DVLOG(1) << __func__ << ": no audio and no video -> error";
OnError(PipelineStatus::DEMUXER_ERROR_COULD_NOT_OPEN);
return; // Do not transition to HAVE_METADATA.
}
// TODO(dalecurtis): Don't create these until kReadyStateHaveFutureData; when
// we create them early we just increase the chances of needing to throw them
// away unnecessarily.
......
......@@ -1654,8 +1654,19 @@ TEST_F(WebMediaPlayerImplTest, NoStreams) {
EXPECT_CALL(*surface_layer_bridge_ptr_, GetSurfaceId()).Times(0);
EXPECT_CALL(*compositor_, EnableSubmission(_, _, _)).Times(0);
// Nothing should happen. In particular, no assertions should fail.
// Since there is no audio nor video to play, OnError should occur with
// resulting network state error update, and transition to HAVE_METADATA
// should not occur.
EXPECT_CALL(client_, NetworkStateChanged()).Times(1);
EXPECT_CALL(client_, ReadyStateChanged()).Times(0);
// No assertions in the production code should fail.
OnMetadata(metadata);
EXPECT_EQ(wmpi_->GetNetworkState(),
blink::WebMediaPlayer::kNetworkStateFormatError);
EXPECT_EQ(wmpi_->GetReadyState(),
blink::WebMediaPlayer::kReadyStateHaveNothing);
}
TEST_F(WebMediaPlayerImplTest, Encrypted) {
......
......@@ -18,6 +18,13 @@ function terminateWorkerAfterMultipleSetTimeouts(test, worker, timeouts_remainin
}
function startWorkerAndTerminateWorker(test, when_to_start_timeouts, timeouts_to_await) {
// TODO(https://crbug.com/878133): Enable main-thread feature detection of
// whether or not the implementation supports MSE-in-Workers, and fail the
// test rapidly here rather than flakily pass/failing the test on those
// implementations. If the timeout occurs near to when the worker's report
// of lack of MSE support reaches the main thread, then the test could
// pass in some cases (when timeout occurs prior to handling that error)
// and fail in others (when worker.onerror dispatch occurs first).
const worker = new Worker("mediasource-worker-util.js");
worker.onerror = test.unreached_func("worker error");
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment