-
Notifications
You must be signed in to change notification settings - Fork 845
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
Shim RTCRtpScriptTransform. #1145
base: main
Are you sure you want to change the base?
Conversation
I don't think this should be done by adapter. adapter's job was that of a small polyfill during the early life of WebRTC. I do not consider the unwillingness of two parties in the W3C Working Group to ignore that "web developers" have adopted what Chromium has been doing for years (and I do not recall any contributions to the underlying native C++ code for insertable streams). The spec's lack of adoption by "web developers" is pretty clear from the comments here, the refusal to merge w3c/webrtc-encoded-transform#105 is rather harmful for "web developers". Not that "web developers" have a strong voice in the working group because the WG is not tackling problems relevant to them... Using "adapter provides a polyfill" as an excuse to not implement "legacy" as done by Safari in the case of the rather widely used |
Web developers should not suffer while vendors reach agreement. The role of adapter.js has always been to assist these developers write the same WebRTC code on all browsers without blocking on vendors, using the latest spec. Web developers need an adapter here to write the same WebRTC code across browsers, so this being a "phase" that is "over" seems demonstrably not so. This 34-line polyfill makes the same application work in Chrome, Safari and Firefox. This is the right place. No-one's forced to use adapter, so blocking tools for web developers on the basis of no-one's using them seems flawed. I'm happy to debate the marginal merits of main-thread parsing vs the perceived difficulty of writing worker code over in w3c/webrtc-encoded-transform#89 (comment), but let's not have that discussion bleed in here. |
Web developers do suffer from the lack of documentation caused by w3c/webrtc-encoded-transform#105. The article does not mention that Chromium has supported this way longer than others browsers,
In cases where the spec was showing consensus. There are clear signs for lack of consensus in this case.
No, this makes a tiny sample application work, by chance. Real applications are a bit more complicated The polyfill unconditionally calls createEncodedStreams upon setting .transform. This flag can not be set unconditionally as, being built for E2EE, Chromium refuses to Also the flag has performance implications noted here All of these suggest this is not the right place. |
My understanding is that historically, the idea with adapter.js was to shim APIs that had consensus, allowing developers to write modern code without worrying about implementation status, but they could be rest assures that any code they wrote was modern - they shouldn't have to migrate again in the future. The more feature got implemented, the more adapter.js became a NO-OP. And honestly I thought we had already reached the finish line here... I've heard so little about adapter.js in the past 5 years that I had assumed this library was dead (both in terms of web developer usage and in terms of being ~NO-OP on up-to-date browser versions). Am I mistaken here? Bridging the gap assumes the shim represents what is about to be implemented. Which assumes agreement. Which I don't think we have? If this is about removing developer pain points, then instead of shimming A on top of B, could we shim B on top of A? I'm not seriously suggesting this, I'm just trying to highlight the importance of knowing where we're going with this, and what the plans for forming consensus are. Especially if we're implicitly encouraging the use of adapter.js in... 2024! |
I applaud wanting to make web developers suffer less, and I admit I don't fully understand the state of things, I just think we need a plan so that people don't migrate from option A to option B and then finally consensus lands on some other option C. |
RTCRtpScriptTransform appears to have consensus from the WebRTC WG's September 2. 2021 successful CfC to publish FPWD with RTCRtpScriptTransform in it and no createEncodedStreams. This shim addresses that gap.
This applies a different standard. What do you mean by "about to be implemented"? Safari and Firefox already implement https://www.w3.org/TR/webrtc-encoded-transform. If you mean "about to be implemented" in Chrome, then I cannot answer what Google intends to implement. But we also haven't applied that standard in the past, e.g. with Plan-B. |
Thanks @fippo for the detailed feedback!
That seems no different from requiring nonstandard
This is not the MDN repo. But if you're suggesting we shouldn't shim unless it can fool users who are unaware they are using a shim, then I disagree. I also don't recall that being the bar in the past (e.g. getParameters in Firefox). I think if we draw a Venn diagram of features in different browsers and a shim covers the shared center of it then it's useful.
These seem like inherent limitations in Chrome's implementation regardless of API. Glad to hear crbug 1502781 (comment) is "improving conformance incrementally"! — From looking at it:
So it sounds like encodedInsertableStreams will soon no longer be required, which sounds great! It should simplify things a great deal. I'm happy to wait for that to merge to not have to address it in the shim. At the same time it seems orthogonal to someone aware of these limitations.
Again, if the Chrome implementation cannot support that then that is not the fault of the shim. It is also not a reason not to shim IMHO. That said, I'm happy to improve on the transform = null case in the shim. It should probably close previous streams. |
It is no different in the sense that unified-plan/plan-b was not a thing that was meaningfully poylfillable either (or we would not have a pointless and painful migration for years)
I'll ask folks on the MDN advisory board then.
getParameters in Firefox was notably written by someone familiar with Firefox (i.e. you). The insertable streams API is used for things like end-to-end encryption which are highly sensitive and require full transparency. "Fooling" with a polyfill is a bad idea in such a case. And it might be harmful for sites that gate such a feature on the existence of the spec API.
So you are proposing to do a half-broken shim. Which will cause what beyond developers getting frustrated by adapter.js and Chromium? While the native implementation in Chromium is doing just what it is supposed to be and already in production (which includes Facetime). adapter has been helping where it made sense, reducing the friction. This change will not do that. I'd suggest doing your own polyfill for "web developers" to include. It can be done ontop of adapter even. I do not think you will get much adoption though. |
I see, I was reacting quite strongly to this being a motivation for the PR:
But if there is agreement here, LGTM from me. Still sad to see adapter.js being revived - @alvestrand Can you sign off on this PR and does Chromium have a plan to implement this as per shim? |
Description
Add a chrome shim for RTCRtpScriptTransform.
Purpose
Help web developers write WebRTC encoded-transform code to spec that works across browsers.
Working example: https://jan-ivar.github.io/samples/src/content/insertable-streams/video-analyzer/ (uses temp shim)
Usage
Making the shim work requires adding the following line in workers: