-
Notifications
You must be signed in to change notification settings - Fork 57
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 draft feature specification #282
Conversation
It looks like I need to fix a few spec-prod errs (div in dl, dl in ol, div in ol, div in ol, ol in dl) per https://github.com/w3c/media-source/runs/2947876193?check_suite_focus=true edit - it looks like 6df1c5d successfully fixes those sped-prod errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments on the syntax.
The logic looks sound on the surface (a bit convoluted but it's not obvious that it can be simplified...). I haven't tried to look into details yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into it some more, I wonder what are the benefits of specifying such a detailed message passing logic between the object in the worker and its mirror in Window.
Wouldn't it be enough to mention the need for implementation to mirror information between Window and worker objects using whatever implementation means they want (as alluded to in Cross-context communication model section) and keep algorithms somewhat unchanged except perhaps to note which information needs to have been mirrored and/or where the algorithm step runs?
For instance, considering the following steps:
If the MediaSource was constructed in a DedicatedWorkerGlobalScope : Post an internal end of stream message to port to main whose implicit handler in Window notifies the media element that it now has all of the media data.
Otherwise: Notify the media element that it now has all of the media data.
Why not keep it to what it was before the change?
Notify the media element that it now has all of the media data.
There is only one media element. It is clear that it is attached to the Window context. The term "notify" is vague, allowing arbitrary complex implementations. I do realize that the worker case is going to be more complex to implement but, from a spec perspective, I'm not clear why specifying the extra step that cannot be directly tested in practice is needed.
(There are likely other algorithms where it remains useful to clarify what is to happen, for instance to mention that visible VideoTrack objects attached to a media element are going to be mirrors of the ones in the worker)
The notion of "mirrored" can be interpreted differently in concurrent systems like the main/worker contexts in this feature. For clarity of what information coherency/latency flexibility is allowed, I included the "Cross-context communication model" and use of it (
This example is a good one for your case. The "notify" is indeed vague due to legacy media pipelines and demuxers introducing some information latencies. I will revert the convoluted text specifically for this example. In contrast, the buffered and seekable extensions need more clarity because they are synchronous (on main thread) and reflect current information immediately, so clarify what the information latency limits here with the model and its usage seemed to me to be not only a good idea but required for better interoperability.
Indeed, like the seekable and buffered extensions, the initialization segment received algorithm and removeSourceBuffer's management of tracks is IMHO made much clearer for interop by the detailed spec. Implementors are free to replace internal, unexposed items in the spec like 'update track state' messages with equivalent. tl;dr: I wanted to clarify the expectations for better implementation interop. |
Right and you shouldn't consider my comments as blocking in any way! I didn't spot anything wrong in the pull request. The algorithms can be iterated over to drop bits that won't be observable in any way. These cases will typically appear when test cases get added to the test suite to assess interop between implementations of MSE in workers. |
80e671f
to
98fe6fc
Compare
Sounds good to me. I've added ee8fc38 to address the one known place where there was perhaps too much verbosity added. |
Specifies expansion of MSE API to usage from DedicatedWorker contexts. This is an MSEv2 feature tracked by issue w3c#175. See also earlier WICG explainer [1]. This is a squashed commit of 23 original commits, notably: * Adds `closing` issue and link to w3c#276. * Adds the `github:` respecConfig key and an issues-summary appendix. * Removes redundant 'Repository' otherLinks config (addition of 'github' to respecConfig has make the manual enumeration in the 'Repository' otherLinks portion of respecConfig redundant.) * Adds subsections for each of the extended HTMLMediaElement's .seekable and .buffered attributes. * Adds more normative detail to the .buffered extension. * Specifies what .seekable and .buffered do when worker has terminated, and references w3c#277 for further discussion. * Describes cross-context track object aliasing. AudioTrack, VideoTrack, TextTrack extensions' sourceBuffer attribute must be null in the Window context's alias of the track if the track is an alias to a worker-created track from MSE-in-Worker. This is to prevent any assumption that cross-context object references are somehow now allowed in MSE or HTMLMediaElement+MSE. * Describes a MessageChannel-based cross-context communication mechanism in a new section, including how MessagePorts on that channel are given to each side (media element, MediaSource) for communicating state needed by algorithms. Updates multiple algorithms and methods to use this mechanism where necessary. Resolves w3c#279. * Specifies how to detect this feature using new `canConstructInDedicatedWorker` attribute without requiring the detection happening in a DedicatedWorker context. * Modernizes to use respec's new variable syntax (`|...|` with types inlined)) for related lines. Wider modernization is an editorial item that is out of scope of this feature. * Modernizes to use internal slots for the new cross-context communication mechanism's channel and ports. Wider modernization is an editorial item out of scope of this feature.o Note, multiple simultaneous changes to track's `selected`, `enabled` or `hidden`/`showing` states could be in-flight from window to worker, and from worker to window. A non-normative note might be good to add here to warn API users of this possibility. See w3c#278. Note, {Audio,Video,Track}{,List} IDL need to be exposed in DedicatedWorker. This might need to be directly in the media element spec. See w3c#280. Note, a reference to an MSE-in-Worker example/demo would be good in the merged PR. Such a demo exists already at [2]. [1] https://github.com/wicg/media-source/blob/mse-in-workers-using-handle/mse-in-workers-using-handle-explainer.md [2] https://wolenetz.github.io/mse-in-workers-demo/mse-in-workers-demo.html
ee8fc38
to
077e233
Compare
077e233 is a squashed replacement of the previous chain of commits, to ease potential rebasing. |
@mwatson2 please take a look. Known related work that I'd like to get landed soon, separately, is adding privacy and security sections, and especially (in response to a high-res timer attack concern raised in a response to my intent-to-experiment request in chromium/blink) clarify that the implementation's choice of cross-context communication mechanism needs to be resilient against such attacks (such as providing variable delay akin to postMessage, as the model already describes, if optimizations are made) for information such as MediaSource duration, live-seekable-range (as reported to HTMLMediaElement from worker) and recent error status (as reported to worker MSE from window HTMLMediaElement). @tidoust please help verify that the changes in response to your comments are correct. Thank you, |
Updates `<var>...</var>` to instead use modern respec `|...|`, and in most cases, on first definition of a variable, includes type of variable in as a suffix to `...` in the definition. Links IDL method arguments in the definition to the argument variable's detail by also wrapping those argument names in `|...|`. Includes a couple other minor changes: uses `minus` instead of `-` in algorithms in a couple places. Replaces a couple instances of "source buffer" with "{{SourceBuffer}}" for improved linkage. Note, the MSE-in-Workers initial draft PR (w3c#282) contains some similar changes to lines only relevant to that draft, and will likely need rebasing and manual merge resolution on top of this once this lands.
That looks good to me, thanks! I note that the IPR bot complains because Google (and other companies) needs to re-join the working group following re-chartering. |
|
||
<li> | ||
<dl class="switch"> | ||
<dt>If the {{MediaSource}} was constructed in a {{DedicatedWorkerGlobalScope}}:</dt> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is by no means an essential change, but I think this would be clearer if the "Otherwise" steps below were broken out an given a name (e.g. "Remove an audio track from Media Element audio track list") and then this step can be something like
If the MediaSource was constructed in a DedicatedWorkerGlobalScope, post an internal remove track mirror message ... whose implicit handler runs the Remove an audio track from Media Element audio track list algorithm on ....
Otherwise run the Remove an audio track from Media Element audio track list algorithm on ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same comment applies to the other instances of this pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, it might be possible to encapsulate this whole pattern so that in the text here, you would write something like
Use the mirror if necessary algorithm to run the following steps in Window
And then elsewhere you would define the mirror if necessary( steps ) algorithm to run the provided steps immediately if the MediaSource is in Window or to post an internal mirror( steps ) message whose implicit handler in Window runs steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, the purpose of the mirror messages is to ensure that the HTML Media Element update steps run as their own task on the main event loop, rather than the associated changes happening "magically" in the middle of some other task execution, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. I'll be updating the PR shortly, now that Google has rejoined the WG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, the purpose of the mirror messages is to ensure that the HTML Media Element update steps run as their own task on the main event loop, rather than the associated changes happening "magically" in the middle of some other task execution, right ?
This is typically the case, yes. Especially will be more so once I update this to likely limit any optimizations of the 'cross-context communication mechanism' for web platform security and interop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is by no means an essential change, but I think this would be clearer if the "Otherwise" steps below were broken out an given a name (e.g. "Remove an audio track from Media Element audio track list") and then this step can be something like
If the MediaSource was constructed in a DedicatedWorkerGlobalScope, post an internal remove track mirror message ... whose implicit handler runs the Remove an audio track from Media Element audio track list algorithm on ....
Otherwise run the Remove an audio track from Media Element audio track list algorithm on ....
I'm looking to see if there's a way to keep such a "named step" within the method algorithm steps instead of being moved to an explicit algorithm in the SourceBuffer algorithm's section. Do you have any preference on this? (@mwatson2 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other media WG specs, and internal consistency with existing MSE spec, moving such a "named step" to be an explicit algorithm is probably best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirror if necessary seems the better approach for this, as it is more general.
|
||
The rest of this section describes a model for bounding information latency for attachments of a {{Window}} | ||
media element to a {{DedicatedWorkerGlobalScope}} {{MediaSource}}. While the model describes communication using | ||
message passing, implementations MAY choose to communicate in potentially faster ways, such as using shared |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/potentially faster/other/
Otherwise we have a question of whether an implementation using a method that is not potentially faster is compliant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given causality and existing app-exposed communication mechanism (postMessage), it's likely not a good thing for the feature to allow slower, or worse, variable, latency versus app postMessage-ing it's own state/info to its own counterparts across worker and main contexts. Further, the affordance for a "faster than postMessage from worker to main" mechanism may not be a good idea given the possibility that an over-optimized implementation might enable security issues akin to Spectre/etc by letting apps abuse the over-optimized implementation to create high-resolution timers, for instance. I'll likely restrict this mechanism shortly (perhaps after this PR, in a future PR introducing P&S sections) to specifically preclude such over-optimizations.
@wolenetz Looks good to me. My only comments are cosmetic suggestions - not technical - and could be done later, if done at all. |
@ #282 (comment) (@mwatson2) - Thank you for your review. In general, I agree with your comments. The "potentially faster" text was included to ensure that at least no more than the latency of a postMessage() done by the app would be the limit. Otherwise, apps would need to take yet more care to understand the state of the attachment cross-context. In this regard, I'm also currently undoing some of the optimizations taken in the Chromium prototype of this feature, as they may enable Spectre-like exploits. Since my AC rep hasn't yet rejoined the WG, I'll await until after my vacation (all of next week) to address comments (or land as-is and do a follow-up to address comments). |
@#282 (comment) in the interests of more easily unblocking the follow-on commits that I'd been working on while pending rejoin to the WG, I'll land this as is and follow-up in later PR on your comments, above. |
Updates `<var>...</var>` to instead use modern respec `|...|`, and in most cases, on first definition of a variable, includes type of variable in a suffix to `...` in the definition. Links IDL method arguments in the definition to the argument variable's detail by also wrapping those argument names in `|...|`. Includes a couple other minor changes: uses `minus` instead of `-` in algorithms in a couple places. Replaces a couple instances of "source buffer" with "{{SourceBuffer}}" for improved linkage. Note, the MSE-in-Workers initial draft PR (w3c#282) contained some similar changes to lines only relevant to that draft, requiring manual rebase of this onto it once that landed.
* Modernize var syntax, including type info where known Updates `<var>...</var>` to instead use modern respec `|...|`, and in most cases, on first definition of a variable, includes type of variable in a suffix to `...` in the definition. Links IDL method arguments in the definition to the argument variable's detail by also wrapping those argument names in `|...|`. Includes a couple other minor changes: uses `minus` instead of `-` in algorithms in a couple places. Replaces a couple instances of "source buffer" with "{{SourceBuffer}}" for improved linkage. Note, the MSE-in-Workers initial draft PR (#282) contained some similar changes to lines only relevant to that draft, requiring manual rebase of this onto it once that landed. Also, disambiguates error and error parameter for now: See [1] for PR review comment motivating this disambiguation. ReSpec gets confused if two distinct entities are named the same, in this case, "error" variable in the endOfStream() method algorithm is now distinguished versus the "error parameter" of the end of stream algorithm. [1] #285 (review)
Following one of the suggested alternatives by mwatson2 on previous pull request review [1], this change introduces a "mirror if necessary" algorithm and uses it to simplfiy multiple places in the spec text where the attached MediaSource needs to update the state of the attached HTMLMediaElement. Note, there are several other cross-context pieces of text left unchanged, since they are specific to either setting up the conditionally cross-context communication ports, conditionally tearing them down, or describe how the extended media element buffering or seekable attributes behave relative to the potentially cross-context and asynchronous communication of the state used in those extensions. Note, upcoming PR for privacy and security section addition will also update (restrict) the flexibility for optimizing cross-context communication to help mitigate related timing attacks in implementations. [1] w3c#282 (review)
Following one of the suggested alternatives by @mwatson2 on previous pull request review [1], this change introduces a "mirror if necessary" algorithm and uses it to simplfiy multiple places in the spec text where the attached MediaSource needs to update the state of the attached HTMLMediaElement. Note, there are several other cross-context pieces of text left unchanged, since they are specific to either setting up the conditionally cross-context communication ports, conditionally tearing them down, or describe how the extended media element buffering or seekable attributes behave relative to the potentially cross-context and asynchronous communication of the state used in those extensions. Note, upcoming PR for privacy and security section addition will also update (restrict) the flexibility for optimizing cross-context communication to help mitigate related timing attacks in implementations. [1] w3c#282 (review)
Following one of the suggested alternatives by @mwatson2 on previous pull request review [1], this change introduces a "mirror if necessary" algorithm and uses it to simplfiy multiple places in the spec text where the attached MediaSource needs to update the state of the attached HTMLMediaElement. Note, there are several other cross-context pieces of text left unchanged, since they are specific to either setting up the conditionally cross-context communication ports, conditionally tearing them down, or describe how the extended media element buffering or seekable attributes behave relative to the potentially cross-context and asynchronous communication of the state used in those extensions. Note, upcoming PR for privacy and security section addition will also update (restrict) the flexibility for optimizing cross-context communication to help mitigate related timing attacks in implementations. [1] #282 (review)
Specifies the exposure of MSE API additionally to DedicatedWorkerGlobalScope.
Updates, when MSE is in such a worker scope, cross-context communication between a Window HTMLMediaElement and a DedicatedWorker MSE instance to make it clear to implementors what is necessary for interop on this feature.
Spec feature for MSE-in-Workers is #175
Note that Chromium already has an experimental implementation and there is a demo of the feature with instructions available at https://wolenetz.github.io/mse-in-workers-demo/mse-in-workers-demo.html
Preview | Diff