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

Rename RTCRtpScriptTransform object #216

Open
alvestrand opened this issue Nov 29, 2023 · 12 comments
Open

Rename RTCRtpScriptTransform object #216

alvestrand opened this issue Nov 29, 2023 · 12 comments

Comments

@alvestrand
Copy link
Contributor

After having discussions where I had problems remembering which was which of RTCRtpScriptTransformer and RTCRtpScriptTransform, I think we should rename at least one of them.

My preference would be to rename RTCRtpScriptTransform to RTCRtpScriptTransformController, since it is an object that exists solely to control an RTCRtpScriptTransformer.

@youennf
Copy link
Collaborator

youennf commented Nov 29, 2023

This API shipped and is in active use. Changing the names now would need a high justification.

@jan-ivar
Copy link
Member

Web developers only deal with RTCRtpScriptTransform and SFrameTransform, so it seems too late to change those names.

RTCRtpScriptTransformer has no constructor, and is simply the type of the transformer property of the "rtctransform" event. E.g.

onrtctransform = async ({transformer: {readable, writable, options}}) => {

No instanceof check is needed, making it's name largely irrelevant in practice, so I think it's name is fine.

@alvestrand
Copy link
Contributor Author

It seems a bit odd to argue that "it's too late" to do a simple thing like a name change, given that:

That said, it would also help the quality of the specification if we the RTCRtpScriptTransformer something else - RTCRtpScriptWrapper, for instance.

@fippo
Copy link
Collaborator

fippo commented Nov 30, 2023

Web developers only deal with RTCRtpScriptTransform and SFrameTransform

Can you provide evidence for this claim? https://github.com/search?q=RTCRtpScriptTransform&type=repositories does not show much in terms of opensource repositories.

Jitsi is one of the main ones and still has Safari support still behind a flag due to what turned out to be a broken simulcast implementation (there was another issue reported way earlier, will update if I can find it).

@youennf
Copy link
Collaborator

youennf commented Dec 1, 2023

FWIW, the API is about transforms, we set sender/receiver transform attributes, it makes sense to create transform objects as well.

As I said earlier, the API is in active use (FaceTime, jitsi).

https://wpt.fyi/results/webrtc-encoded-transform/idlharness.https.window.html shows the API names are correctly implemented in two browsers, though Safari should do some WebIDL clean-up.

@alvestrand
Copy link
Contributor Author

My issue with the naming is that we have two objects (Transform and Transformer) that are both Web platform APIs, with very different responsibilities, but both controlling the user's Javascript function that we conventionally call a "transform".

That's 3 objects close to each other where the names are almost impossible to not confuse. We shouldn't do that.

@fippo
Copy link
Collaborator

fippo commented Dec 4, 2023

As I said earlier, the API is in active use (FaceTime, jitsi).

Speaking as a Jitsi committer: this is still disabled by default so there is no backward-compat concern there. Even if it was in production the only ask would be to have a upgrade path with a six month migration window.

@jan-ivar
Copy link
Member

My issue with the naming is that we have two objects (Transform and Transformer) that are both Web platform APIs, with very different responsibilities, but both controlling the user's Javascript function that we conventionally call a "transform".

That's 3 objects close to each other where the names are almost impossible to not confuse. We shouldn't do that.

That's one constructor, one event attribute, and one app function. They're only "objects" in the most confusing sense.

The spec could perhaps do a better of job introducing concepts, but any confusion seems to disappear in actual code:

// main.js
sender.transform = new RTCRtpScriptTransform(worker, {side: "sender"});

...on main, transform = transform.

// worker.js
onrtctransform = async ({transformer: {readable, writable, options}}) => {
    await readable.pipeThrough(new TransformStream({transform})).pipeTo(writable);
    function transform(chunk, controller) { ... }

...over on worker, we react to an rtctransform event given a transformer object, much like a JS-driven stream is given a controller. I.e. the transformer is the controller for the transform.

This somewhat mimics the duality of the streams spec which separates an outside control surface from an inside controller surface. We're leveraging this split further by "outside" being on main thread, and "inside" on the worker.

Note: anything after the await above is app specific, and its use of transform is idiomatic with its use of TransformStream in this example. It can of course call this function anything it likes.

This may be an issue with reading the spec more than anything else.

@jan-ivar
Copy link
Member

jan-ivar commented Dec 18, 2023

There's naming precedence in the TransformStream constructor argument transformer, which the WHATWG chose to define as a dictionary (presumably because there's no need for it to be anything else):

image

In contrast, our transformer needs to be an interface because of the generateKeyFrame method on it, and for it to be an event attribute.

@jan-ivar
Copy link
Member

  • The API does not have consensus in the WG

@alvestrand could you be more specific about which parts of the API lack consensus?

Looking at the history, RTCRtpScriptTransform appears to have consensus from our September 2. 2021 successful CfC on our FPWD (1 objection was over SFrameTransform). From your response:

I support promoting this document to WG draft.

Note that I still think the document is a better document if #111 is merged (I don't think we did a good job of checking consensus on #64), and I think we should resolve #89 according to my suggestions there, but I think we can continue that discussion post FPWD.

You expressed concern that we didn't do a good job checking consensus on #64 which you merged four months prior to CfC following agreement at the April meeting. But isn't the CfC a catch-all for this? No other responses mentioned it.

No concerns seemed raised against the RTCRtpScriptTransform API shape itself, modulo a constructor argument.

@alvestrand
Copy link
Contributor Author

@jan-ivar according to our work mode, "Always keep to the topic of the issue. If there are other things you want to bring up, raise additional issues. The aim should be to keep each issue focused on a specific topic.".

I won't discuss further the question of what the status of the document, or consensus on its part, is here. The totality of issues in the tracker (and referencing the W3C Process document) should be adequate documentation of that.

@jan-ivar
Copy link
Member

I appreciate that. Feel free to open a separate issue for #216 (comment) then.

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

No branches or pull requests

4 participants