webrtc / samples

WebRTC Web demos and samples
https://webrtc.github.io/samples
BSD 3-Clause "New" or "Revised" License
13.75k stars 5.69k forks source link

FrameTransform Interface does not match TransformStream transformer Interface #1569

Open maikthomas opened 1 year ago

maikthomas commented 1 year ago

Please read first!

Please use discuss-webrtc for general technical discussions and questions. If you have found an issue/bug with the native libwebrtc SDK or a browser's behaviour around WebRTC please create an issue in the relevant bug tracker. You can find more information on how to submit a bug and do so in the right place here

Note: If the checkboxes above are not checked (which you do after the issue is posted), the issue will be closed.

Browser affected

No browser affected, just a code question

Description

While looking into Insertable Streams samples e.g. https://github.com/webrtc/samples/blob/gh-pages/src/content/insertable-streams/video-processing/js/canvas-transform.js I noticed that the Transformers implement FrameTransform from here: https://github.com/webrtc/samples/blob/gh-pages/src/content/insertable-streams/video-processing/js/pipeline.js#L73

This Interface is very similar to the transform parameter of the TransformStream constructor: https://developer.mozilla.org/en-US/docs/Web/API/TransformStream/TransformStream#parameters See also typed TS def for Transformer: https://github.com/microsoft/TypeScript/blob/main/lib/lib.dom.d.ts#L1734

The differences I see are in these method names: init -> start destroy -> flush

Would it make sense for the transformers in this repo to match the interface above? Or am I mixing different concepts here?

Steps to reproduce

N/A

Expected results

N/A

Actual results

N/A

dogben commented 1 year ago

Yes, that would make sense.

In that case createProcessedMediaStreamTrack (and createProcessedMediaStream) could accept the same type. It would change the timing of the init/start and flush/destroy calls, so a few other places in the code need to change as well. As currently written, the FrameTransform objects are reused when the source or sink changes without calling init or destroy; with the Transform interface, I believe start and flush would be called multiple times, which means that each implementation needs to be changed to handle that.

I would prefer either making all of the above changes or leaving things as-is. I don't think it's a good idea to use the Transform interface but manually call start/flush in the Pipeline class.