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

Fix export of track-muted and set-track-muted. #986

Merged
merged 13 commits into from
Apr 25, 2024
Merged

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented Jan 22, 2024

Fixes #985

@marcoscaceres is this correct syntax?


Preview | Diff

@youennf
Copy link
Contributor

youennf commented Jan 23, 2024

Fixes #985

@youennf
Copy link
Contributor

youennf commented Jan 23, 2024

We have other uses of data-export in the spec?
Should we update them as well?

@jan-ivar
Copy link
Member Author

We have other uses of data-export in the spec?
Should we update them as well?

I briefly considered it, but then realized any export of generic terms like "source" might conflict with other specs.

I also considered that maybe we shouldn't export everything by default, and that it might be better to do so on a need-to basis.

@alvestrand
Copy link
Contributor

Raising the @dontcallmedom signal for advice.

getusermedia.html Outdated Show resolved Hide resolved
getusermedia.html Outdated Show resolved Hide resolved
@jan-ivar
Copy link
Member Author

jan-ivar commented Feb 8, 2024

Hmm that didn't work either

1275.12-1275.142: error: Attribute “for” not allowed on element “dfn” at this point.
"file:/home/runner/work/mediacapture-main/mediacapture-main.w3c/getusermedia.html":1294.15-1294.149: error: Attribute “for” not allowed on element “dfn” at this point.
❌ Not so good... please fix the issues above.

Trying @youennf's version next

@dontcallmedom
Copy link
Member

for is for bikeshed, respec needs data-dfn-for; I'll look into fixing this PR more generally early next week

getusermedia.html Outdated Show resolved Hide resolved
getusermedia.html Outdated Show resolved Hide resolved
@jan-ivar
Copy link
Member Author

jan-ivar commented Feb 8, 2024

@dontcallmedom ah thanks, will try that next

getusermedia.html Outdated Show resolved Hide resolved
getusermedia.html Outdated Show resolved Hide resolved
@dontcallmedom
Copy link
Member

looking at fixing the markup, there is an ambiguity in the language of the spec: sometimes "muted" is applied to the concept of source (e.g. "If source is stopped or muted, abort these steps"), sometimes to the concept of track (e.g. "A track sourced by a camera or microphone may be forcibly muted").

The <dfn> seems to allude (obliquely) to the source: "Muted refers to the input to the MediaStreamTrack".

I can propose a fix once this is clarified.

@alvestrand
Copy link
Contributor

I think the idea was that "muted" refers to something that happens at the source, and is JS-visible through the "muted" attribute of the track (the source is ~never JS-visible). So "a track is muted" should probably be reworded as "the muted attribute is set on the track".
I think "input of the MediaStreamTrack" should generally be replaced with "source".

@jan-ivar
Copy link
Member Author

looking at fixing the markup, there is an ambiguity in the language of the spec: sometimes "muted" is applied to the concept of source (e.g. "If source is stopped or muted, abort these steps"), sometimes to the concept of track (e.g. "A track sourced by a camera or microphone may be forcibly muted").

I think that's largely an internal inconsistency, unrelated to exporting what to other specs is a track-related concept:

Error: ROR] WebIDL identifier muted for MediaStreamTrack is defined multiple times

The error seems to come from the desire to add data-dfn-for="MediaStreamTrack" which makes it clash with MediaStreamTrack's muted WebIDL attribute of the same name:

Which reminds me there's also this.[[Muted]]:

This same triad also exists for enabled:

...which makes me question moving the muted concept to the source as a solution to the export problem.

How do other specs do this? E.g. HTML appears to have done away with internal slots? What's the state of the art here?

Error: ROR] Couldn't find "set a track's muted state" in this document or other cited documents: [dom], [html], [infra], [permissions], [permissions-policy], [webaudio], and [webidl].

The desire to add data-dfn-for="MediaStreamTrack" broke existing links in this spec (and other specs if we go ahead with it). Fixable by changing

- [=set a track's muted state=]
+ [=MediaStreamTrack/set a track's muted state=]

...in this spec and all other specs that reference it.

Choices here seem to be:

  1. Export the existing definitions without data-dfn-for="MediaStreamTrack" (is that frowned on?)
  2. adddata-dfn-for="MediaStreamTrack" and rename the concept to "muted state" or something (ugh).
  3. Push back on mediaSession's need for the concept. E.g. could it say "A user agent MAY expose microphone and camera state to web pages via the {{MediaStreamTrack/muted}} attribute."?

@dontcallmedom
Copy link
Member

dontcallmedom commented Mar 25, 2024

exporting muted without a scope would be frowned upon, yes.

I've gone ahead and:

  • made muted exported while distinct from the IDL attribute (using data-dfn-type=dfn)
  • added a <dfn> for muted sources and used it in the couple of places where we had an autolink that didn't make sense in the context of MediaStreamTrack (which was the reason I raised this distinction in the first place)
  • aligned the enabled and ended state to use the same pattern as muted

This will break autolinks in specs that were relying on the definitions with their existing for scope:

  • mediacapture-fromelement (muted, enabled)
  • mediacapture-transform (muted)
  • mediasession (muted)
  • screen-capture (muted, ended)
  • captured-mouse-events (ended)
  • capture-handle-identity (ended)
  • webrtc (ended)

[= source/stopped =].</p>
<p>After the application has invoked the {{MediaStreamTrack/stop()}}
method on a {{MediaStreamTrack}} object, or once the [=source=] of a
{{MediaStreamTrack}} permanently ends production of live samples to its tracks,
whichever is sooner, a {{MediaStreamTrack}} is said to be
<dfn id="track-ended" data-dfn-for="track" data-export>ended</dfn>.</p>
<dfn id="track-ended" data-dfn-for="MediaStreamTrack" data-dfn-type="dfn" data-export>ended</dfn>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<dfn id="track-ended" data-dfn-for="MediaStreamTrack" data-dfn-type="dfn" data-export>ended</dfn>.</p>
<dfn data-dfn-for="MediaStreamTrack" class="export">ended</dfn>.</p>

Copy link
Member

Choose a reason for hiding this comment

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

the id is used in other specs, I see no reason to break it; the dfn-type is needed to distinguish it from the IDL attribute of the name and scope

Suggested change
<dfn id="track-ended" data-dfn-for="MediaStreamTrack" data-dfn-type="dfn" data-export>ended</dfn>.</p>
<dfn id="track-ended" data-dfn-for="MediaStreamTrack" data-dfn-type="dfn" class=export>ended</dfn>.</p>

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... is it defined twice for the two kinds?

Copy link
Member

Choose a reason for hiding this comment

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

once per kind, yes

Comment on lines +904 to 905
<p><dfn class="export" data-dfn-for="MediaStreamTrack" data-dfn-type="dfn" id=
"track-muted">Muted</dfn> refers to the input to the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p><dfn class="export" data-dfn-for="MediaStreamTrack" data-dfn-type="dfn" id=
"track-muted">Muted</dfn> refers to the input to the
<p><dfn class="export" data-dfn-for="MediaStreamTrack">Muted</dfn> refers to the input to the

Copy link
Member

Choose a reason for hiding this comment

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

likewise, dfn-type is needed to distinguish from the IDL attribute

Copy link
Member

@marcoscaceres marcoscaceres Apr 15, 2024

Choose a reason for hiding this comment

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

Ok, I guess... feel free to resolve.

However, I think that might be confusing (even though it's correct)... Maybe there should be a "media stream track" concept?

@@ -954,7 +955,7 @@ <h4>Media Flow</h4>
<var>track</var>.</p>
</li>
</ol>
<p><dfn data-export id="track-enabled" data-lt="track enabled state|enabled" data-lt-noDefault>Enabled/disabled</dfn> on the other hand is
<p><dfn data-export id="track-enabled" data-dfn-for="MediaStreamTrack" data-dfn-type="dfn" data-lt="track enabled state|enabled" data-lt-noDefault>Enabled/disabled</dfn> on the other hand is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p><dfn data-export id="track-enabled" data-dfn-for="MediaStreamTrack" data-dfn-type="dfn" data-lt="track enabled state|enabled" data-lt-noDefault>Enabled/disabled</dfn> on the other hand is
<p><dfn class="export" data-dfn-for="MediaStreamTrack" data-lt="track enabled state|enabled" data-lt-noDefault>Enabled/disabled</dfn> on the other hand is

Copy link
Member

@marcoscaceres marcoscaceres Apr 14, 2024

Choose a reason for hiding this comment

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

@jan-ivar, the data-lt="track enabled state|enabled" doesn't make much sense. This should really be:

<p><dfn class="export" data-dfn-for="MediaStreamTrack">track state</dfn> can be either "enabled" or "disabled"...

See how HTML handles Document/visibility state:

which is either "hidden" or "visible", initially set to "hidden".

Copy link
Member Author

Choose a reason for hiding this comment

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

This is unchanged by this PR, and I'm not sure what's the current respec way of doing things. @dontcallmedom can I bother you to address the feedback from @marcoscaceres here?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p><dfn data-export id="track-enabled" data-dfn-for="MediaStreamTrack" data-dfn-type="dfn" data-lt="track enabled state|enabled" data-lt-noDefault>Enabled/disabled</dfn> on the other hand is
<p>The <dfn data-export id="track-enabled" data-dfn-for="MediaStreamTrack" data-dfn-type="dfn" >enabled</dfn> (resp. disabled) state on the other hand is

getusermedia.html Outdated Show resolved Hide resolved
Co-authored-by: Marcos Cáceres <[email protected]>
@jan-ivar jan-ivar assigned dontcallmedom and unassigned jan-ivar Apr 15, 2024
@jan-ivar
Copy link
Member Author

Thanks @dontcallmedom for the commits!

getusermedia.html Outdated Show resolved Hide resolved
Co-authored-by: Marcos Cáceres <[email protected]>
@jan-ivar jan-ivar merged commit bf8dac0 into w3c:main Apr 25, 2024
2 checks passed
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.

The [=muted=] concept is exported wrongly
5 participants