add fixes for wolenetz comments #18225

chrisdavidmills · 2022-11-16T10:34:18Z

Summary

This PR adds fixes in response to the review comments provided by @wolenetz in #18220

I did the following:

Removed the separate worker_support items from the top-level interface support items, as the extra rows in the support tables were confusing and unnecessary
Marked audioTracks and videoTracks support as experimental, for SourceBuffer and HTMLMediaElement
Marked SourceBuffer.textTracks support as false for Chromium
Reverted VideoPlaybackQuality changes
Verified that MSE-in-workers works in Opera 94, not Opera 93, by testing @wolenetz's demo in beta and developer versions. To fix this, I'm not going to change every Opera support number individually, as that is not maintainable, and the linter complained. Instead, I'm going to fix the mirroring code so that it reports the correct Opera version for Chrome 106/107/108. I've filed an issue for that at Chrome 108 should mirror to Opera 94, but it doesn't #18224, which I'll have a go at fixing next.

Test results and supporting details

Testing was done using the demo available at https://wolenetz.github.io/mse-in-workers-demo/mse-in-workers-demo.html

Related issues

Fixes #18220

…bject data

wolenetz

Looks pretty good. A few nits/questions. Thanks again for working on this!

wolenetz · 2022-11-16T21:21:45Z

api/HTMLMediaElement.json

+              {
+                "version_added": "108",
+                "partial_implementation": true,
+                "notes": "Support added for <code>MediaSource</code> objects transferred from dedicated workers via <code>MediaSource.handle</code> (see <a href='https://crbug.com/878133'>bug 878133</a>)."


Probably best to be precise here: ...Support added for MediaSourceHandle objects transferred from dedicated workers where they were obtained from MediaSource.handle...

Good call, updated!

wolenetz · 2022-11-16T21:34:41Z

api/MediaSource.json

-            "standard_track": true,
-            "deprecated": false
+            "status": {
+              "experimental": true,


I noticed that MSE-in-Workers in MDN in these changes is being marked "experimental". It is shipping on-by-default in Chrome, and is implemented to follow the corresponding changes in the current MSE spec working draft. What does "experimental" mean here, given this information?

This is the definition that MDN uses for experimental: https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Experimental_deprecated_obsolete#experimental. In the time I've been contributing to MDN, we've gone back and forth on this a lot, and it still isn't perfect, but I think it's OK. MSE-in-workers is marked as experimental because it is still only available in one rendering engine, so the advice to devs would be to think carefully when considering using it. If you want to support Fx and WebKit browsers, then you might need to think about a fallback, alternative, or gracefully degrading experience. If you only need Chromium support, then you are OK. This certainly isn't the same as "don't use it".

Thanks for the explanation. https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Experimental_deprecated_obsolete#experimental could probably use clarification around the meaning of "It is implemented and enabled by default in less than two modern major browsers" -- I understand that it is being applied as if s/major browsers/major distinct underlying rendering engines in browsers/, but on the surface it is confusing.

I've filed this: mdn/content#22391. Please add to it if you have any other thoughts on the subject. I'll try to fix this at some point next week, if no one else grabs it.

wolenetz · 2022-11-16T21:41:37Z

api/SourceBuffer.json

@@ -1723,7 +1678,14 @@
            "description": "Available in workers",
            "support": {
              "chrome": {
-                "version_added": "108"
+                "version_added": "108",


Is there a way to note that both SourceBuffer.audioTracks and .videoTracks in a dedicated worker MediaSource's SourceBuffer is not expected to succeed even if the experimental flag is provided (there is further spec work and implementation work required to support those track lists from a worker context, and currently the implementation - if experimental flag is enabled and the app invokes the audioTracks() or videoTracks() getter on a worker SourceBuffer, then Chrome crashes the renderer process to avoid potentially worse behavior. This is known work in both spec and impl (MSE spec, HTML spec, Chrome impl), partially captured by this MSE spec issue: w3c/media-source#280

I've added a note to audioTracks/videoTracks to hopefully cover this; let me know what you think. I wasn't sure that the spec issue you linked to really explained what the issue was, so I decided not to link to it. IS there a better link available, or do you think I should link here, as that is where explanation will be added?

Good point - I've reactivated the most closely related Chromium issue to this piece of the spec. Please link to https://bugs.chromium.org/p/chromium/issues/detail?id=487288#c24 (it also links transitively to the MSE spec issue 280 that I mentioned, above).

Perfect, that is a useful explanation. Link added.

wolenetz

Looks good to me - please add the link to https://bugs.chromium.org/p/chromium/issues/detail?id=487288#c24 as I just commented about a few minutes ago.

queengooborg

Thanks for following up on this, LGTM!

chrisdavidmills added 2 commits November 16, 2022 10:16

add fixes for wolenetz comments

3fcbaf6

fix sourcebufferlist

058e510

github-actions bot added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Nov 16, 2022

chrisdavidmills marked this pull request as draft November 16, 2022 10:34

chrisdavidmills added 2 commits November 16, 2022 10:42

fix mediasourcehandle

6cc29b7

add note about support for MediaSource from dedicated workers to srcO…

5b071c0

…bject data

wolenetz reviewed Nov 16, 2022

View reviewed changes

responding to further wolenetz comments

1c39107

wolenetz reviewed Nov 17, 2022

View reviewed changes

add link to explain note about crash behavior

7b596b5

chrisdavidmills marked this pull request as ready for review November 18, 2022 08:09

queengooborg approved these changes Nov 20, 2022

View reviewed changes

queengooborg merged commit 7fedfda into mdn:main Nov 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add fixes for wolenetz comments #18225

add fixes for wolenetz comments #18225

chrisdavidmills commented Nov 16, 2022

wolenetz left a comment

wolenetz Nov 16, 2022

chrisdavidmills Nov 17, 2022

wolenetz Nov 16, 2022

chrisdavidmills Nov 17, 2022

wolenetz Nov 17, 2022

chrisdavidmills Nov 18, 2022

wolenetz Nov 16, 2022

chrisdavidmills Nov 17, 2022

wolenetz Nov 17, 2022

chrisdavidmills Nov 18, 2022

wolenetz left a comment

queengooborg left a comment

add fixes for wolenetz comments #18225

add fixes for wolenetz comments #18225

Conversation

chrisdavidmills commented Nov 16, 2022

Summary

Test results and supporting details

Related issues

wolenetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolenetz left a comment

Choose a reason for hiding this comment

queengooborg left a comment

Choose a reason for hiding this comment