-
Notifications
You must be signed in to change notification settings - Fork 28
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
Added DisplaySurfaceChangeCallback #289
base: gh-pages
Are you sure you want to change the base?
Conversation
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.
Generally LGTM, but some suggestions.
@@ -490,7 +502,8 @@ <h2><dfn>MediaDevices</dfn> Additions</h2> | |||
<li> | |||
<p>This invocation of {{MediaDevices/getDisplayMedia()}} is | |||
now considered to have produced a new | |||
<dfn>capture-session</dfn>.</p> | |||
<dfn>capture-session</dfn>, with the associated | |||
[=capture-source-set=], <var>sources</var>.</p> | |||
</li> |
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.
Should the part below, which dealt with controller.[[Source]]
, be updated to reference the video source from capture-source-set
rather than (as it does now) the "video track's source"?
(GitHub didn't let me attach this comment in the desired location.)
The user agent MUST NOT change the [=display surface=] | ||
associated with a [=capture-session=] unless the user has | ||
explicitly indicated that they want this change to be | ||
performed by interacting with user agent or operating system. |
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.
nit: Could you make editorial changes here so as to discourage anyone from misreading "by interacting" as binding to the wrong part of the sentence? Maybe "...performed. Users can do this by interacting with..."
34aa232
to
ab47c8a
Compare
ab47c8a
to
7c6434f
Compare
|
||
|
||
<p> | ||
The user agent MUST NOT change the [=display surface=] |
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.
I don't think we can validate this, I would tend to reword so that we do not use MUST NOT.
explicitly indicated that they want this change to be | ||
performed by interacting with user agent or operating system. | ||
|
||
If the user indicates a change of [=display surface=], the |
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.
We probably need to specify the order of the callback execution and the configurationchange
event at MediaStreamTrack.
I would tend to have the event be executed after the callback.
|
||
<li> | ||
<p>Stop delivering frames from all sources in the | ||
[=capture-source-set=] associated with the [=capture-session=]. |
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.
If audio source is not changing, should we allow audio to flow?
|
||
|
||
<p> | ||
<var>newSources</var> MUST fulfill the same requirements as the |
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.
It is not really clear to me what this MUST is about.
|
||
<li> | ||
<p>If <var>controller.{{CaptureController/[[DisplaySurfaceChangeCallback]]}}</var> | ||
is null, abort these steps.</p> |
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.
I think we should do this check later inside a task queued since CaptureController/[DisplaySurfaceChangeCallback] can be changed at any point in time by JS, so the checks should be done within the context event loop.
|
||
|
||
<li> | ||
<p>Queue a task to:</p> |
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.
Will it have a link to the definition?
We should probably start using a task source as well.
<li> | ||
<p>Run the [=ApplyConstraints algorithm=] on all tracks in | ||
<var>stream</var> with the appropriate constraints. Should | ||
this fail, abort these steps.</p> |
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.
Why should we fail, user wants to switch, it is ok for the web page to update its constraints as part of the switch.
|
||
<li> | ||
<p>Queue tasks to end all tracks connected to sources in the | ||
[=capture-source-set=] associated with the [=capture-session=]. |
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.
I think the default should be to leave the tracks alive, the future option being for the web page to ask for tracks to be ended.
The principle is that a web page can easily implement ending of the tracks, but it cannot implement the reverse.
|
||
|
||
<li> | ||
<p>If [=this=].{{CaptureController/[[IsBound]]}} is |
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.
It seems restrictive to disallow setting the callback more dynamically.
Why should we add this restriction?
<p>If | ||
[=this=].{{CaptureController/[[DisplaySurfaceChangeCallback]]}} | ||
is not <code>null</code>, [=exception/throw=] an | ||
"{{InvalidStateError}}" {{DOMException}}.</p> |
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.
I am not sure why we should not allow to overwrite the callback either.
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 seems different from w3c/mediacapture-screen-share-extensions#4 (comment):
Something like
captureController.processSourceSwitch(stream => { ... });
orcaptureController.processSourceSwitch(null);
I’m fine with this.
Fixes w3c/mediacapture-screen-share-extensions#4.
Preview | Diff