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 draft feature specification #282

Merged
merged 1 commit into from Aug 31, 2021

Conversation

wolenetz
Copy link
Member

@wolenetz wolenetz commented Jun 30, 2021

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

@wolenetz
Copy link
Member Author

wolenetz commented Jun 30, 2021

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
Please don't let those block review.

edit - it looks like 6df1c5d successfully fixes those sped-prod errors.

Copy link
Member

@tidoust tidoust left a 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.

media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
Copy link
Member

@tidoust tidoust left a 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)

@wolenetz
Copy link
Member Author

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?

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 (port to worker / port to main) in specific portions of the algorithms where the communication.

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.

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.

(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)

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.

@tidoust
Copy link
Member

tidoust commented Jul 15, 2021

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.

@wolenetz wolenetz force-pushed the mse-in-workers-initial-spec branch from 80e671f to 98fe6fc Compare July 26, 2021 20:11
@wolenetz
Copy link
Member Author

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.

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
@wolenetz wolenetz force-pushed the mse-in-workers-initial-spec branch from ee8fc38 to 077e233 Compare July 26, 2021 22:34
@wolenetz
Copy link
Member Author

077e233 is a squashed replacement of the previous chain of commits, to ease potential rebasing.

@wolenetz
Copy link
Member Author

@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,
Matt

wolenetz added a commit to wolenetz/media-source that referenced this pull request Jul 31, 2021
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.
@tidoust
Copy link
Member

tidoust commented Aug 9, 2021

@tidoust please help verify that the changes in response to your comments are correct.

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>
Copy link
Contributor

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 ....

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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 ?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member Author

@wolenetz wolenetz Feb 17, 2022

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 )

Copy link
Member Author

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.

Copy link
Member Author

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
Copy link
Contributor

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.

Copy link
Member Author

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.

@mwatson2
Copy link
Contributor

@wolenetz Looks good to me. My only comments are cosmetic suggestions - not technical - and could be done later, if done at all.

@wolenetz
Copy link
Member Author

@ #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).

@wolenetz
Copy link
Member Author

@#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.

@wolenetz wolenetz merged commit fbfb36f into w3c:main Aug 31, 2021
github-actions bot added a commit that referenced this pull request Aug 31, 2021
SHA: fbfb36f
Reason: push, by @wolenetz

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
wolenetz added a commit to wolenetz/media-source that referenced this pull request Sep 1, 2021
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.
wolenetz added a commit that referenced this pull request Sep 1, 2021
* 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)
@wolenetz wolenetz deleted the mse-in-workers-initial-spec branch September 1, 2021 18:15
wolenetz added a commit to wolenetz/media-source that referenced this pull request Feb 18, 2022
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)
wolenetz added a commit to wolenetz/media-source that referenced this pull request Feb 18, 2022
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)
wolenetz added a commit that referenced this pull request Mar 18, 2022
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants