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 audio support #715

Merged
merged 3 commits into from
Sep 3, 2024
Merged

[ts-sdk] Add audio support #715

merged 3 commits into from
Sep 3, 2024

Conversation

wkozyra95
Copy link
Member

@wkozyra95 wkozyra95 commented Aug 30, 2024

Public API

  • Add props to the InputStream
  • Add hook useInputAudio
  • Modify hook useInputStreams to return map instead of a list
  • Add unregister output
  • If multiple Input streams will present in scene then audio will be summed

Internal

  • Refactor compositor context. Now we have one global store for entire compositor instance and per-output context that is specific to single output. (Output is not a store in the react sense)
  • I need to expose some types between @live-compositor/core and live-compositor so I exported _liveCompositorInternals object that groups everything. Those types are not intended to be used by user
  • I was not able to figure out a better way to shutdown an output, so I added a wrapper component that is waiting for exteranal store with just one boolean value (shutdown). If shutdown is set we change a scene to just a <View />

@wkozyra95 wkozyra95 force-pushed the @wkozyra95/ts-audio-hook branch 3 times, most recently from 3d5a601 to c22dc85 Compare August 30, 2024 23:49
@wkozyra95 wkozyra95 self-assigned this Sep 2, 2024
@wkozyra95 wkozyra95 force-pushed the @wkozyra95/ts-audio-hook branch 3 times, most recently from e82394d to b3e5041 Compare September 2, 2024 12:32
@wkozyra95 wkozyra95 force-pushed the @wkozyra95/ts-audio-hook branch from b3e5041 to e1122ab Compare September 2, 2024 12:46
@wkozyra95 wkozyra95 marked this pull request as ready for review September 2, 2024 12:49
Comment on lines +17 to +24
/**
* Audio volume represented by a number between 0 and 1.
*/
volume?: number;
/**
* Mute audio.
*/
mute?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you add mute? Can't they just set volume=0 if somebody wants to mute audio? Having 2 separate fields for that doesn't make sense IMO. It also introduces ambiguity when both values are specified.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do you add mute?

I think it's a useful API, especially in React you can do <InputStream mute/>. I also considered using muted instead (I think mute is better, but muted is the same as in <video/> HTML tag), but I don't have strong opinions on that.

Can't they just set volume=0

Yes, as you can see below it's literally what it does internally

Having 2 separate fields for that doesn't make sense IMO.

It's often true, but I disagree with it as a general statement. This is a tradeoff, we are getting simpler API to do sth specific, but increase the complexity because user needs to know how those 2 ways interact. You can argue if you want that tradeoff is not worth it here, but not in general.

It also introduces ambiguity when both values are specified.

Does it? Can you imagine someone seeing those 2 options and assuming that volume has higher priority than mute?

ts/@live-compositor/core/src/utils.ts Outdated Show resolved Hide resolved
@wkozyra95 wkozyra95 merged commit ed2ca15 into master Sep 3, 2024
4 checks passed
@wkozyra95 wkozyra95 deleted the @wkozyra95/ts-audio-hook branch September 3, 2024 14:06
Copy link
Member

@WojciechBarczynski WojciechBarczynski left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants