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

[Identity] .getCaptureHandle() seems misplaced on the track #12

Open
jan-ivar opened this issue Feb 11, 2022 · 7 comments
Open

[Identity] .getCaptureHandle() seems misplaced on the track #12

jan-ivar opened this issue Feb 11, 2022 · 7 comments
Labels

Comments

@jan-ivar
Copy link
Member

jan-ivar commented Feb 11, 2022

MediaStreamTrack is a realtime audio/video consumption API, where consumers apply consumption constraints, such as width, height and noiseSuppression. Implementations satisfy them by processing media, to separate tracks from influencing one another.

JS can clone or transfer tracks to other realms, to delegate them to every corner of their app designed to consume them, be it for local playback, recording, or sending over a peer connection. OverconstrainedError resolves conflicts where processing cannot.

track.getCaptureHandle doesn't fit with this model, because:

  1. It's not clear this control surface should follow the track to all consumers. The top-level JS doing the capturing may wish to retain this control for itself and only send tracks to dumb consumers, without worrying that these consumers may be able to bootstrap and send control messages to the source (this seems true whether we add Actions Integrate the Actions compromise presented at January WebRTC interim #6 or not),
  2. Such control messages to the source would affect consumers of all tracks, not just the one track. There's no system to resolve conflicts like we have for constraints.

This suggests the API belongs someplace else.

@jan-ivar
Copy link
Member Author

Possible Solutions

We explored other ways to surface results from getDisplayMedia in #52:

partial interface MediaDevices {
    attribute EventHandler ongetdisplaymediaresult;
}

This seems better organized, even though anyone can register an event handler. At least it doesn't conflate control surfaces meant for different audiences together.

We could also do what fetch did and pass in a controller:

const controller = new CaptureController();
const stream = await navigator.getDisplayMedia({video: true}, {controller});

@alvestrand
Copy link

This seems like a complexification of the API. getDisplayMedia has one result, and there seems no reason not to reuse that result.

Again, not something that should meet the bar for not adopting the document.

@eladalon1983
Copy link
Member

eladalon1983 commented Feb 16, 2022

The top-level JS doing the capturing may wish to retain this control for itself and only send tracks to dumb consumers

That's a corner case with no concrete examples cited thus far. More importantly, it case can be addressed easily, for example (just spitballing here) by specifying that a transferred track loses access to getCaptureHandle(). Dealing with this hypothetical corner case does not require massive restructuring of getDisplayMedia - which you effectively suggest.

But, @jan-ivar, when you manage to convince the Working Group to add ongetdisplaymediaresult, let's discuss restructuring Capture Handle accordingly. Until that (theoretical) day, let's not block progress by predicating one thing on another.

I suggest we close this issue for now and adopt the document. We can reopen in the future.

@jan-ivar
Copy link
Member Author

jan-ivar commented Feb 25, 2022

getDisplayMedia has one result, and there seems no reason not to reuse that result.

@alvestrand I gave two reasons in the OP. getDisplayMedia produces one result today: a MediaStream for consumption. Trying to add a two-way capturee (bootstrapping) communication/control surface to that, I count as two results.

Redefining well-known types to carry tangential APIs simply because one method (out of the many that returns said type) needs to return more information, seems like an OO anti-pattern we should not adopt.

it ... can be addressed easily, ... by specifying that a transferred track loses access to getCaptureHandle()

@eladalon1983 That sounds hacky, but helps highlight the issue: that this new API probably doesn't belong on that object in the first place. Having clone with side-effects seems like another anti-pattern, but let's discuss that in w3c/mediacapture-main#857.

I think questioning whether a proposed API is misplaced, is a legitimate issue to bring up ahead of adopting said API.

@jan-ivar
Copy link
Member Author

The more I think about this, the more I think the following is the right surface for both Actions (#15) and Identification:

const controller = new CaptureController();
const stream = await navigator.getDisplayMedia({video: true, controller});
const {handle, origin} = controller;
const actions = controller.getSupportedActions();
await controller.sendAction("next");

This requires no "massive restructuring" of MediaStreamTrack.

I suggest we close this issue for now and adopt the document. We can reopen in the future.

I think part of the value of the CfC/CfA process is to surface issues and document them. If and when we adopt this document, I would expect we transfer these issues to the w3c github repo, since they are still valid, not close them. We're at the beginning of standardization here, not the end.

@eladalon1983
Copy link
Member

eladalon1983 commented Feb 28, 2022

Trying to add a two-way capturee (bootstrapping) communication/control surface to that, I count as two results.

By this logic, id, kind, label etc. are all additional results, and we have a double-digit of results already. I find this argument unconvincing.

Having clone with side-effects seems like another anti-pattern

I believe intentionally dropping some information when cloning has precedents and makes sense.

This requires no "massive restructuring" of MediaStreamTrack.

It's a big changes to getDisplayMedia. Consensus here will not be easy. I personally find an out-parameter quite awkward. Maybe even "footgunny". For example, what happens to old values of controller if the Promise is rejected? Would an application run the risk of using an old controller then?

These are not discussions I am anxious to have. I am excited by the prospect of extending the capabilities of Web-applications. By extending the Web's reach. I don't think our time will be well-spent lengthily debating minor issues of API shape, that make no functional difference.

I would expect we transfer these issues to the w3c github repo

The issues will be transfered. And I may still advocate for their closure, and you may still object. Progress entails concluding discussions. It is reasonable to disagree about this.

@jan-ivar
Copy link
Member Author

I believe intentionally dropping some information when cloning has precedents ...

What are they?

I personally find an out-parameter quite awkward.

It's technically an in-parameter: an object that will be associated only upon resolve, but I know what you mean, and I agree it's not ideal, but backwards compatible things rarely are.

However, having this capture controller object might be a hub for several capture controls in the hopper:

  1. Capture handle
  2. Capture actions
  3. Conditional focus Conditional Focus (When Display-Capture Starts) mediacapture-screen-share#190

@youennf WDYT?

Maybe even "footgunny". For example, what happens to old values of controller if the Promise is rejected?

The controller would never be associated, which seems fine. We could also prevent reuse if we want.

@eladalon1983 eladalon1983 changed the title .getCaptureHandle() seems misplaced on the track [Identity] .getCaptureHandle() seems misplaced on the track Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants