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: Add missing sign in pause(). #18676

Merged
merged 1 commit into from
Feb 19, 2020
Merged

Audio: Add missing sign in pause(). #18676

merged 1 commit into from
Feb 19, 2020

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Feb 19, 2020

see #18664. Sorry, I've missed to add the + sign 😇 .

@Mugen87 Mugen87 added this to the r114 milestone Feb 19, 2020
@Mugen87 Mugen87 merged commit 892ad91 into mrdoob:dev Feb 19, 2020
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 19, 2020

Updated test fiddle with latest dev build:

https://jsfiddle.net/4m0c67x8/

@mrdoob
Copy link
Owner

mrdoob commented Feb 19, 2020

That... doesn't look right to me... 🤔

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 19, 2020

I've tested the fiddle quite a while and it seems it's the correct approach to compute the offset parameter for AudioBufferSourceNode.start(). The value is relative to the when parameter which is something I've realized when working with the fiddle from @SUDOCS.

@SUDOCS
Copy link

SUDOCS commented Feb 20, 2020

@mrdoob @Mugen87
I think this.context.currentTime - this._startedAt means the duration between two pauses. It could be a negative number when user clicks pause in the time less than delay(also with other condictions). So I agree with the implementation like so:

this._pausedAt += Math.max( this.context.currentTime - this._startedAt, 0 ) * this.playbackRate;

However, pause() also goes wrong when audio looping. I tried to improve it in play() like so:

source.start(this._startedAt, (this._pausedAt + this.offset) % this.duration, this.duration);

but this.duration always be undefined.

My mother language isn't English and my understanding of AudioBufferSourceNode.start () is not very thorough, so sorry for my possible mistakes in advance.

@shootTheLuck
Copy link

Hello All!

I noticed this same issue with pausing/restarting looped audio and was working on it in a similar way. Hopefully, mentioning this here is ok as opposed to opening another issue. I was able to make it work properly by changing the line @SUDOCS mentions in the play() method to:

source.start( this._startedAt, ( this._pausedAt + this.offset ) % this.buffer.duration, this.duration );

If loop.Start/loop.End are set I guess it might get more complicated...

r115

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 31, 2020

but this.duration always be undefined.

The property is undefined unless you assign a value to it. It is optional and can be used to overwrite the default duration of the audio buffer.

@shootTheLuck It seems your code does not work if Audio.duration is set by the user since you always use the duration of the buffer. Maybe like so?

var offset = ( this._pausedAt + this.offset ) % ( this.duration || this.buffer.duration );
source.start( this._startedAt, offset, this.duration );

@shootTheLuck
Copy link

shootTheLuck commented Apr 2, 2020

@mugen Awesome! I didn't know about using duration either but I see it now in the docs.

I spent yesterday playing with this sound: https://opengameart.org/sites/default/files/audio_preview/54321GO.ogg.mp3 😳 and have a suggestion for pausing with loopStart / loopEnd that seems to work pretty well.

I'd be thrilled to be able to share it in operation and I signed up with jsfiddle but I'm not able to get your fiddle https://jsfiddle.net/4m0c67x8/ to work anymore...nor the one I'm trying to set up here: https://jsfiddle.net/shootTheLuck/cnf14gv9/latest/ .

If you'd like to consider them though, my suggestions for .pause and .play (and stop) are in the fiddle and marked with ///***. I'd be interested to know what you think!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 2, 2020

Can you try it with the following fiddle? https://jsfiddle.net/smhrdac4/

I've only updated the audio file. It's now one of the repository.

@shootTheLuck
Copy link

Hi and Thanks!

https://jsfiddle.net/shootTheLuck/6jsnLuq2/32/

Hopefully that works to demonstrate

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 5, 2020

However, pause() also goes wrong when audio looping. I tried to improve it in play() like so:

@SUDOCS I've tested your code and I don't think it works. Why do you think that the computed offset should be modulo the audio buffer's duration? Even if an audio is looped, the duration does not change.

@shootTheLuck
Copy link

@Mugen87 Hello, my apologies if I'm mistaken, but did you mean me? Looking back on this, I seem to have been the one to bring up the buffer duration... and you rightly corrected me with:

Maybe like so?

var offset = ( this._pausedAt + this.offset ) % ( this.duration || this.buffer.duration );
source.start( this._startedAt, offset, this.duration );

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 8, 2020

@shootTheLuck Sorry, I was confused by my own suggestion because I've tested with a badly audio file. This one made it much more easier to debug the issue:

https://audiojungle.net/item/male-voice-counting-1-to-10/21677488

File #19079 for fixing the issue.

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.

4 participants