w3c / webrtc-encoded-transform

WebRTC Encoded Transform
https://w3c.github.io/webrtc-encoded-transform/
Other
122 stars 26 forks source link

Rename RTCRtpScriptTransform object #216

Open alvestrand opened 7 months ago

alvestrand commented 7 months ago

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 commented 7 months ago

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

jan-ivar commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago
  • 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 commented 7 months ago

@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 commented 7 months ago

I appreciate that. Feel free to open a separate issue for https://github.com/w3c/webrtc-encoded-transform/issues/216#issuecomment-1833208034 then.