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

Should we rename SFrameTransform and RTCRtpScriptTransform #65

Closed
youennf opened this issue Mar 4, 2021 · 8 comments
Closed

Should we rename SFrameTransform and RTCRtpScriptTransform #65

youennf opened this issue Mar 4, 2021 · 8 comments

Comments

@youennf
Copy link
Collaborator

youennf commented Mar 4, 2021

And also RTCRtpScriptTransformer

@youennf
Copy link
Collaborator Author

youennf commented Mar 4, 2021

@alvestrand mentionned we could find better names.
@jan-ivar mentioned that transforms are called streams in various specs.

@jan-ivar
Copy link
Member

jan-ivar commented Mar 7, 2021

Firstly, the streams spec is particular about what can be called "streams", so we should use "stream" when it fits.

Secondly, transform streams in the platform seem come in pairs:

TextEncoderStream includes GenericTransformStream;
TextDecoderStream includes GenericTransformStream;

CompressionStream includes GenericTransformStream;
DecompressionStream includes GenericTransformStream;

To follow that would mean changing from:

SFrameTransform includes GenericTransformStream;

...to

SFrameEncrypterStream includes GenericTransformStream;
SFrameDecrypterStream includes GenericTransformStream;

This seems more ergonomic in JS. E.g. instead of

transceiver.sender.transform = new SFrameTransform();
transceiver.receiver.transform = new SFrameTransform({role: "decrypt");

...we'd avoid the boolean arg and just have:

transceiver.sender.transform = new SFrameEncrypterStream();
transceiver.receiver.transform = new SFrameDecrypterStream();

@youennf
Copy link
Collaborator Author

youennf commented Mar 8, 2021

Currently, one can simply write:

transceiver.sender.transform = new SFrameTransform();
transceiver.receiver.transform = new SFrameTransform();

The 'role' is only used when using SFrameTransform as part of a JS-based pipe (see https://w3c.github.io/webrtc-insertable-streams/#sframe-transform-algorithm).

I am not a big fan of XXStream names for transforms.
For instance, https://streams.spec.whatwg.org/#rs-pipe-through first parameter name is 'transform'.
And that makes sense since it clarifies what it does: it does not create or consume data, it transforms data.

SFrameTransform mirrors well RTCRtpSender/RTCRtpReceiver 'transform' attribute name.
I would find it odd to rename sender.transform to sender.stream.
sender.transformStream would probably be ok but seems long and unnecessary precise.

@jan-ivar
Copy link
Member

jan-ivar commented Mar 8, 2021

For instance, pipeThrough first parameter name is 'transform'.

True, though I don't know how much to infer from typeless argument names. It looks like the streams spec has specifically blessed the name "stream" for arguments to both pipeThrough and pipeTo, even though their (duck) type signatures are entirely different. E.g.

await src.pipeThrough(a).pipeThrough(b).pipeTo(c); // a, b and c are "streams" (but really only c)

it does not create or consume data, it transforms data.

Or even transforms a stream of data? @domenic any comment on which naming strategy we should follow?

see https://w3c.github.io/webrtc-insertable-streams/#sframe-transform-algorithm

Ah, I missed that. But frame.[[rtcObject]] appears undefined. Should it be frame.[[owner]]?

The 'role' is only used when using SFrameTransform as part of a JS-based pipe

I worry this is too clever and defeats encapsulation, composition, and early error detection. E.g. are you saying the following would work as long as foo is undefined?

onrtctransform = async ({readable, writable}) => {
  const sframe = new SFrameTransform({role: "decrypt"});
  if (foo) {
    readable = readable.pipeThrough(foo);
  }
  await readable.pipeThrough(sframe).pipeTo(writable);
}

I think it would be better to explicitly have a SFrameDecrypterStream class and throw early if JS tries to use it on a sender. This would also match how we do encoding/decoding and compression/decompression.

@domenic
Copy link
Contributor

domenic commented Mar 8, 2021

I like the SFrameEncrypterStream / SFrameDecrypterStream naming; it seems to match the other pairs of transform streams on the platform.

@youennf
Copy link
Collaborator Author

youennf commented Mar 8, 2021

Ah, I missed that. But frame.[[rtcObject]] appears undefined. Should it be frame.[[owner]]?

Right, I should fix that.

I worry this is too clever and defeats encapsulation, composition, and early error detection. E.g. are you saying the following would work as long as foo is undefined?

That should work too given how insertable stream pipeline and data is bound to an owner.

I would like to get a full proposal here for SFrameTransform, RTCRtpScriptTransform, RTCRtpSender.transform and RTCRtpReceiver.transform. I would first judge on consistency inside the spec, then consistency with other specs.

Funnily, we seem to lean towards removing stream from the title of the spec.

@youennf
Copy link
Collaborator Author

youennf commented Mar 9, 2021

Also in scope: RTCRtpScriptTransformer and DedicatedWorkerGlobalScope.onrtctransform

@youennf
Copy link
Collaborator Author

youennf commented Oct 5, 2023

Too late for renaming, closing

@youennf youennf closed this as completed Oct 5, 2023
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

3 participants