Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Oct 20, 2020

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Oct 19, 2020

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/+/2486074
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818731}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot changed the title MSE-in-Workers: err if no A/V in WMPI::OnMetadata WebMediaPlayerImpl: err if no A/V in ::OnMetadata Oct 19, 2020
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/+/2486074
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818731}
@Hexcles
Copy link
Member

Hexcles commented Oct 20, 2020

Flakiness will be addressed by https://chromium-review.googlesource.com/c/chromium/src/+/2487834

@Hexcles Hexcles merged commit ebbf27b into master Oct 20, 2020
@Hexcles Hexcles deleted the chromium-export-cl-2486074 branch October 20, 2020 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants