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

MSE-in-Workers content update PR #22242 contained a few items needing correction or clarification #22327

Closed
wolenetz opened this issue Nov 15, 2022 · 1 comment · Fixed by #22341
Labels
Content:WebAPI Web API docs effort: medium This task is a medium effort. help wanted If you know something about this topic, we would love your help!

Comments

@wolenetz
Copy link

MDN URL

https://developer.mozilla.org/en-US/docs/Web/API/Media_Source_Extensions_API

What specific section or headline is this issue about?

Multiple cross-cutting areas of the MSE MDN documentation updated by PR #22242 (MSE-in-Workers content additions) contained items I found during my PR review unfortunately after the PR had already been approved and merged.

What information was incorrect, unhelpful, or incomplete?

Below is copied verbatim from the PR #22242 conversations where I originally noted the issues on my eventual review of the PR:

#22242 (comment):

So what's the deal here, and what do you think I should say on the srcObject page?

In Chrome, srcObject on HTMLMediaElement is only currently usable for associating a MediaStream or a Dedicated Worker MediaSource's MediaSourceHandle. Main thread MediaSources cannot provide handles yet, nor does srcObject work for plain MediaSource objects in Chrome.

#22242 (comment):
Preview URL verifications:

  • srcOjbect: Should probably get some kind of note mentioning MediaSourceHandle (from a Dedicated Worker MediaSource's handle getter, transferred to the main thread) is the way to attach a Dedicated Worker MediaSource to a main thread HTMLMediaElement.
  • canConstructInDedicatedWorker: LGTM
  • MediaSource.handle: mostly LGTM, though the worker portion of the example script should probably indicate "// In a dedicated worker" near the top of that example snippet, and indicate something like "// Transfer the handle to the main context" before the postMessage line in that snippet (perhaps also with some clarification, since that postMessage sends to whichever context created the dedicated worker, and it might not be the main thread; MediaSourceHandles cannot be successfully transferred into or via a Shared Worker or Service Worker, so it could assist readers to point out the intended target is the main context in the snippet), and also indicate something like "// Await sourceopen on the MediaSource before creating SourceBuffers and populating them with fetched media" because the dedicated worker MediaSource won't accept creation of SourceBuffers until its readyState is "open" -- which can only happen once the MediaSource (in this case via the MediaSource's handle) is attached to the HTMLMediaElement.
  • MediaSourceHandle: mostly LGTM, though MediaSourceHandle is currently only retrievable from a Dedicated Worker MediaSource, so technically the statement "Each MediaSource object has its own distinct MediaSourceHandle" is false, since there is no MediaSourceHandle for a main thread MediaSource object.
  • MediaSource: mostly LGTM, though similar comments around example snippet of usage of dedicated worker MediaSource as noted above. Also would help to clarify that the handle getter is visible only on Dedicated Worker MediaSource instances. Also noticed a minor mispelling: "constructuring" should be "constructing".
  • Media_Source_Extensions_API: worker part LGTM. I did notice above that, "MSE allows us to replace the usual single track src value fed to media elements..." would be more accurate if it instead said "MSE allows us to replace the usual single progressive src URI fed to media elements..." (this is because such progressive src URI can indeed have multiple tracks muxed inside them, they just provide no ability for the web app to do things like bitrate adaptation, splicing, codec switching, or advanced telemetry reporting).
  • Functions and classes available to workers: LGTM

Finally, on at least one page (MediaSource.handle), the "Note: This feature is available in Web Workers" link is broken. Maybe this is due to the preview linkage or perhaps it's a real broken linkage.

What did you expect to see?

See the detailed list of questions and items I noted as possibly incorrect/needing clarification/incomplete, above.

Do you have any supporting links, references, or citations?

The MSE spec and my mse-in-workers-demo are already both linked appropriately and contain the best information.
This issue is meant to fix some post-merge items I found in my eventual PR review of #22242. See also the related data update PR (mdn/browser-compat-data#18189) and similar issue I filed today on the post-merge items I found with it, too (mdn/browser-compat-data#18220).

Do you have anything more you want to share?

Thank you for doing this content update! I apologize for not getting my review comments entered more rapidly on this, necessitating this issue as a follow-up of the already-merged PR #22242

@github-actions github-actions bot added Content:WebAPI Web API docs needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. labels Nov 15, 2022
@sideshowbarker sideshowbarker added help wanted If you know something about this topic, we would love your help! effort: medium This task is a medium effort. and removed needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. labels Nov 16, 2022
@sideshowbarker
Copy link
Collaborator

heads-up @chrisdavidmills

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Content:WebAPI Web API docs effort: medium This task is a medium effort. help wanted If you know something about this topic, we would love your help!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants