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

[ts-sdk] Add Blob as MP4 input #1007

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

noituri
Copy link
Member

@noituri noituri commented Mar 10, 2025

Closes #1001

@noituri noituri requested a review from wkozyra95 March 10, 2025 15:35
Copy link
Member Author

Choose a reason for hiding this comment

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

This was not used by any example, but MultipleOutputs had pretty much the same implementation so I put everything here

Comment on lines 15 to +16
| Api.RegisterInput
| { type: 'mp4_blob'; blob: any }
Copy link
Member Author

Choose a reason for hiding this comment

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

Api.RegisterInput is a generated type and already contains definitions for mp4; hence, the new type mp4_blob

Copy link
Member

Choose a reason for hiding this comment

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

I don't like adding new type just for that, it might make sense if it was more generic to handle some other cases types in the future, but not in this form

Copy link
Member

@wkozyra95 wkozyra95 left a comment

Choose a reason for hiding this comment

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

I don't think this will work with Mp4 components. Is it?

I don't like the idea of just patching additional input type that would only work in wasm just to handle this one specific. I would consider first a more generic/future-proof approach, and only go with this approach if there is no other choice

Comment on lines 15 to +16
| Api.RegisterInput
| { type: 'mp4_blob'; blob: any }
Copy link
Member

Choose a reason for hiding this comment

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

I don't like adding new type just for that, it might make sense if it was more generic to handle some other cases types in the future, but not in this form

@@ -23,7 +23,11 @@ export async function handleRegisterInputRequest(
): Promise<RegisterInputResult> {
if (body.type === 'mp4') {
assert(body.url, 'mp4 URL is required');
return handleRegisterMp4Input(ctx, inputId, body.url);
const arrayBuffer = await downloadToArrayBuffer(body.url);
Copy link
Member

Choose a reason for hiding this comment

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

the whole point of having functions like handleRegisterMp4Input is to separate mp4-specific code from common code.

Preparing an array buffer here only unnecessarily complicates this function which should be only a simple if-else tree to call appropriate handlers. Additionally, it spreads potential functions that can throw an error all over the place.

Comment on lines +28 to +31
export async function downloadToArrayBuffer(url: string): Promise<ArrayBuffer> {
const response = await fetch(url);
return await response.arrayBuffer();
}
Copy link
Member

Choose a reason for hiding this comment

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

in my opinion this util is not helpful

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.

[ts-sdk] Add support for mp4 blob
2 participants