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

FrameTransform Interface does not match TransformStream transformer Interface #1569

Open
4 tasks done
maikthomas opened this issue Aug 12, 2022 · 1 comment
Open
4 tasks done

Comments

@maikthomas
Copy link
Contributor

maikthomas commented Aug 12, 2022

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

  • I understand that issues created here are only relevant to the samples in this repo - not browser or SDK bugs
  • I have provided steps to reproduce
  • I have provided browser name and version
  • I have provided a link to the sample here or a modified version thereof

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
Copy link
Collaborator

dogben commented Aug 12, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants