-
Notifications
You must be signed in to change notification settings - Fork 102
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
feat: added seek & rate to SpeakerAudioDestination #504
base: master
Are you sure you want to change the base?
feat: added seek & rate to SpeakerAudioDestination #504
Conversation
8eb778d
to
829f3fa
Compare
@calinalexandru Thanks for submitting this! The code looks good, I'll run this through some manual tests and give the team and chance to look at it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! The new added api looks good to me. Added one comment
@@ -52,6 +52,7 @@ export class SpeakerAudioDestination implements IAudioDestination, IPlayer { | |||
this.privId = audioDestinationId ? audioDestinationId : createNoDashGuid(); | |||
this.privIsPaused = false; | |||
this.privIsClosed = false; | |||
this.privAudio = new Audio(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The undefined
value of privaAudio
means the playback is not supported (e.g. unsupported format, see line 134). This would break some current logic. So pls revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yulin-li can you please elaborate your response? From what I can see calling method format
will re-initialize privAudio
so it doesn't matter if I also set it constructor. I'm only trying to understand if there's a better approach than re-adding undefined
checks (following null object pattern). Thanks.
Hello,
In my application I need to be able to adjust seek & rate of the Audio element, and I don't believe there's a reason for them not to be here.
Also, I set
privAudio = new Audio()
from the constructor to prevent unnecessary undefined check.Went ahead and removed some of those checks, although, more expressions can be simplified in the class.