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

Add API and algorithm to expose transform to workers #62

Merged
merged 2 commits into from
Mar 4, 2021

Conversation

youennf
Copy link
Collaborator

@youennf youennf commented Feb 25, 2021

@youennf
Copy link
Collaborator Author

youennf commented Feb 25, 2021

Fixes #18

Copy link
Contributor

@alvestrand alvestrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to flag this one as -1 until I figure out what it takes away.

@alvestrand
Copy link
Contributor

Suggest making one PR to add the new API and another one that takes the old API away, so that we can discuss them separately.

@youennf
Copy link
Collaborator Author

youennf commented Feb 26, 2021

uggest making one PR to add the new API and another one that takes the old API away, so that we can discuss them separately.

I reverted the changes, PR now only adds stuff!

Copy link
Contributor

@alvestrand alvestrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that "removing the old way" is removed, this is a completion of the algorithm already landed, and I approve.

I do wonder if we can get the ability to "polyfill" the old behavior by allowing the "worker" argument to the ScriptTransform constructor to be the Javascript equivalent of "this" - that would make same-process processing possible, but not simpler than worker-based processing.

5. FIXME: transfer |t1|.`[[readable]]` and |t2|.`[[writable]]` to the dedicated worker.
6. FIXME: Create counterpart of |this| in dedicated worker, for instance expose transfered |t1|.`[[readable]]` and |t2|.`[[writable]]`.
5. Let |serializedOptions| be the result of [$StructuredSerialize$](|object|).
6. Let |serializedReadable| be the result of [$StructuredSerializeWithTransfer$](|t1|.`[[readable]]`, « |t1|.`[[readable]]` »).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the << marks be square brackets? (I think the second argument to Serialize is a transfer list)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what style is best here.
I reused the style used in https://streams.spec.whatwg.org/#ws-transfer.

@alvestrand
Copy link
Contributor

Note: I'd be even happier if the PR also added examples to explainer.md.

@youennf
Copy link
Collaborator Author

youennf commented Mar 4, 2021

Note: I'd be even happier if the PR also added examples to explainer.md.

I agree we should do that before landing #64.

@alvestrand
Copy link
Contributor

alvestrand commented Mar 4, 2021

Thinking some more - in all cases the transform object is created on the main thread (constructor has exposed=window).
If "worker" argument is nullable, we can say "no event is fired when worker is null".

If we can then extract the transformer from the transform object that's an attribute of the sender/receiver (hmm.... those are easily confused names, and the difference is importnat ....) we have a very reasonably shimmable interface that gives us the "single thread" functionality. WDYT?

@alvestrand
Copy link
Contributor

Note: DedicatedWorker is still a link that goes nowhere.

@guidou
Copy link
Collaborator

guidou commented Mar 4, 2021

Some examples of how the API is intended to be used in practice would be very beneficial to be able to figure out a plan to implement it. I would prefer that we keep the old API in the spec until implementors have had time to evaluate the new one in practical settings.

};

[Global=(Worker,DedicatedWorker),Exposed=DedicatedWorker]
interface RTCRtpScriptTransformer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After having tried to comprehend the overall algorithm several times, I've concluded that we should rename this object to RTCRtpScriptTransformSocket. It's not a transformer itself, it's what you plug a transform (like SframeTransform) into.

readonly attribute WritableStream writable;
readonly attribute any options;
};

[Exposed=(Window)]
interface RTCRtpScriptTransform {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly, RTCRtpScriptTransform is not a transform. I suggest we call it RTCRtpScriptTransformController.

@alvestrand
Copy link
Contributor

New issues:

  • Add examples
  • Work on naming (align with stream spec, such as DuplexStream and Endpoint Pair)

@alvestrand alvestrand merged commit 84e6ef0 into w3c:main Mar 4, 2021
github-actions bot added a commit that referenced this pull request Mar 4, 2021
…form-worker-apiAdd API and algorithm to expose transform to workers

SHA: 84e6ef0
Reason: push, by @alvestrand

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
foolip added a commit to foolip/webrtc-insertable-streams that referenced this pull request Mar 8, 2021
};

[Global=(Worker,DedicatedWorker),Exposed=DedicatedWorker]
partial interface DedicatedWorkerGlobalScope : WorkerGlobalScope {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With partial interfaces in Web IDL the parent interface shouldn't be specified. I've sent #71 to fix this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #70.

@@ -62,54 +62,6 @@ an additional API on {{RTCRtpSender}} and {{RTCRtpReceiver}} to
insert the processing into the pipeline.

<pre class="idl">
// New dictionary.
dictionary RTCInsertableStreams {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RTCInsertableStreams dictionary was removed here, but it's still used as the return type of two methods in https://w3c.github.io/webrtc-insertable-streams/#specification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that is a mistake from splitting the PR, I will add it back.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#73

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

Successfully merging this pull request may close these issues.

4 participants