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

Audio speed #148

Merged
merged 9 commits into from
Oct 4, 2023
Merged

Audio speed #148

merged 9 commits into from
Oct 4, 2023

Conversation

lechten
Copy link
Contributor

@lechten lechten commented Jun 13, 2022

This MR introduces the new option defaultAudioRate to control the speed of audio playback.

@lechten
Copy link
Contributor Author

lechten commented Mar 11, 2023

Why do you include the call nextAudio.load()?

I do not need it, and it resets the audio element's properties, including the speed. Per W3 school, it re-loads the element, while you seem to aim for a pre-load...

lechten added a commit to lechten/reveal.js-plugins that referenced this pull request Mar 13, 2023
This function resets the audio properties, in particular the speed.
See there:
rajgoel#148 (comment)
@lechten
Copy link
Contributor Author

lechten commented Sep 25, 2023

@rajgoel What do you think about an option to control the speed of audio? My students like that...

@rajgoel
Copy link
Owner

rajgoel commented Sep 25, 2023

@lechten: I think this could be a really nice enhancement (and sorry for letting you wait with this PR for so long). One of the reasons, that I am hesitant about this are the side effects of video-playback linked to the audio controls and to playback of recorded chalkboard drawings. Also the animate plugin somehow requires the correct playback timing. Do you have an idea on how to deal with these side effects? Alternatively, some mechanism would be needed to prevent using the feature in such use cases.

@lechten
Copy link
Contributor Author

lechten commented Sep 25, 2023

Thank you for the explanations. I do not have any experiences with recordings, sorry.

@rajgoel
Copy link
Owner

rajgoel commented Sep 25, 2023

One idea could be to disable the feature when RevealChalkboard or RevealAnimate are loaded (or at least to print a warning to console).

I'll be happy to merge if

  • video playback attached to the audio controls as shown here also works (by copying the playback rate)
  • the potential conflicts with other plugins are reasonable documented,
  • there is a method to RevealAudioSlideshow.getPlaybackRate() or similar that can be used to get the current playback rate.

Can you please also check whether the PR still works with the current version (it looks like it should, but i haven't tested).

@rajgoel
Copy link
Owner

rajgoel commented Sep 25, 2023

Btw. is the playback rate copied or reset to 1 when jumping from on slide to another?

@lechten
Copy link
Contributor Author

lechten commented Sep 26, 2023

(by copying the playback rate)

I am not sure what to copy where. So far, I initialize each audio with defaultAudioRate. Also, in selectAudio I copy the previous audio rate (similarly to what exists for volume and muted).

the potential conflicts with other plugins are reasonable documented,

What about this:
Note that this option changes the speed of all audios, which may interfere with other plugins such as RevealChalkboard or RevealAnimate, which may have their own notion of the "correct" playback rate.

there is a method to RevealAudioSlideshow.getPlaybackRate() or similar that can be used to get the current playback rate.

Should this return defaultAudioRate or the rate of currentAudio (if set) or something else?

Can you please also check whether the PR still works with the current version (it looks like it should, but i haven't tested).

Yes, it does.

Btw. is the playback rate copied or reset to 1 when jumping from on slide to another?

It is reset to 1 by nextAudio.load() in the play event listener, which I suggest to remove (see above). If I pause an audio in your demo and advance to the next slide, it starts with the rate set to 1 (as selectAudio is not called, which would undo the resetting to 1).

@rajgoel
Copy link
Owner

rajgoel commented Sep 26, 2023

(by copying the playback rate)

I am not sure what to copy where. So far, I initialize each audio with defaultAudioRate. Also, in selectAudio I copy the previous audio rate (similarly to what exists for volume and muted).

Each media element on the current slide or fragment with data-audio-controls is synchronised with the audio playback. Thus, the playback rates need to be the same.

there is a method to RevealAudioSlideshow.getPlaybackRate() or similar that can be used to get the current playback rate.

Should this return defaultAudioRate or the rate of currentAudio (if set) or something else?

This function should return the playback radio currently used (so that it could be picked up by the other plugins to synchronize).

Btw. is the playback rate copied or reset to 1 when jumping from on slide to another?

It is reset to 1 by nextAudio.load() in the play event listener, which I suggest to remove (see #148 (comment)). If I pause an audio in your demo and advance to the next slide, it starts with the rate set to 1 (as selectAudio is not called, which would undo the resetting to 1).

I guess nextAudio.load() can be removed, because I indeed believed it would preload not reload. The important thing for me is that for both explicit slide changes and when auto-advancing to the next slide or fragment, the playback rate of media elements with data-audio-controls need to be (un)set appropriately.

@lechten
Copy link
Contributor Author

lechten commented Sep 28, 2023

I rebased the branch on top of master (4.2.3).

I renamed the variable from audio to playback, as video is affected as well and introduced another variable to keep track of the current rate with event handlers.

Note that I no longer set the speed in selectAudio() but upon play. This also works if users pause one audio and navigate elsewhere (and if not every slide contains audio). A similar approach might be more robust for volume and muted as well. Is that worth the effort (adding variables with event handlers)?

As discussed, I removed the call nextAudio.load() (and code leading to it). In setupAudioElement(), you set attribute preload to none. Maybe this could be auto?

@rajgoel rajgoel merged commit 3238936 into rajgoel:master Oct 4, 2023
@rajgoel
Copy link
Owner

rajgoel commented Oct 4, 2023

Thanks a lot for the PR. Please let me know should there be any remaining issues with this.

@lechten
Copy link
Contributor Author

lechten commented Oct 4, 2023

Thanks for merging 😄

@TS-CUBED
Copy link

Just to confirm: there is currently no way to have the playback speed synchronise with chalkboard annotation?

@rajgoel
Copy link
Owner

rajgoel commented Mar 20, 2025

As far as I remember, the chalkboard replay currently does not support different playback rates.

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