llaske / sugarizer

Sugarizer is a web implementation of the Sugar platform to run on any device or browser
https://sugarizer.org
Apache License 2.0
198 stars 416 forks source link

Harsh Sound in TamTam Micro activity in keyboard mode on Firefox and Safari #564

Closed s-mag closed 4 years ago

s-mag commented 4 years ago

To reproduce : Change any instrument and play 2 sounds together.. You will hear distorted sounds throughout the activity. @llaske Can you reproduce this bug...

llaske commented 4 years ago

I'm not able to reproduce it. I'm trying with guitar for example.

s-mag commented 4 years ago

@llaske After 5-6 clicks together this happens. Is this a bug or just exceeding the limit which causes internal errors.

llaske commented 4 years ago

I reproduced it on Firefox thought it perfectly work on Chrome. Could you confirm you've tested it on Firefox?

s-mag commented 4 years ago

I reproduced it on Firefox thought it perfectly work on Chrome. Could you confirm you've tested it on Firefox?

Yes it is in firefox, no problem in chrome. I forgot to mention that!

llaske commented 4 years ago

For information, same issue on Safari

pidddgy commented 4 years ago

very very interesting bug.... when I reproduce the bug then play audio on anything else e.g. other tabs, spotify desktop app, system noises that is distorted and crackly too, until i close the tab with sugarizer. tested on firefox

s-mag commented 4 years ago

very very interesting bug.... when I reproduce the bug then play audio on anything else e.g. other tabs, spotify desktop app, system noises that is distorted and crackly too, until i close the tab with sugarizer. tested on firefox

Yes, this might be internal system error in firefox aswell. I am not sure btw

s-mag commented 4 years ago

@llaske @pidddgy according to http://forums.mozillazine.org/viewtopic.php?f=38&t=2542145 forum The issue comes in Firefox 15

llaske commented 4 years ago

@s-mag This Firefox issue is very old, I'm not sure it's related. @pidddgy Is there a way to reproduce the problem with just a Tone.js sample?

s-mag commented 4 years ago

@s-mag This Firefox issue is very old, I'm not sure it's related.

Oh yeah it’s very old...

pidddgy commented 4 years ago

perhaps could be related to https://developers.google.com/web/updates/2017/09/autoplay-policy-changes#webaudio? i'm really not sure. I tried changing the play code in piano.js to:

Tone.context.resume().then(() => {
            var player = new Tone.Player('./audio/database/'+currentPianoMode+".mp3");
            var pitchShift = new Tone.PitchShift({
                pitch: pitchMap[pitchName]
            }).toMaster();
            player.connect(pitchShift);
            player.autostart = true;
            console.log('playing');
        })

as according to https://stackoverflow.com/questions/50281568/audiocontext-not-allowed-to-start-in-tonejs-chrome but that didn't do anything. I am getting console warnings about The AudioContext was not allowed to start. It must be resumed (or created) after a user gesture on the page.

llaske commented 4 years ago

I know this limit regarding playing sound. It happens in some case in other activities. But I don't think it's related to our problem. Plus, the problem is on Firefox, not Chrome. Will be interested to try to reproduce the issue on a simple Tone.js sample so we could ask to Tone.js community.

pidddgy commented 4 years ago

@llaske https://github.com/pidddgy/tonejstest i've written up a short demonstration here

llaske commented 4 years ago

Nice! Could you open an issue in ToneJS repo?

llaske commented 4 years ago

@pidddgy did you see the answer from ToneJS community? Very interesting! Guess your current code could be improved to try to reuse ToneJS objects.

llaske commented 4 years ago

@pidddgy any progress on it? I've got a new TamTam mode planned but don't want to launch before this issue is fixed

pidddgy commented 4 years ago

sorry, I haven't made significant progress on the issue. I'll likely be taking a break from open source for the next few months to focus on OI contests/school.

For anyone interested in tackling this issue, the problem is that Tone.Player objects are being created on every play and not being disposed of. To solve this issue I think the solution is to create a global map of Tone.Player objects, perhaps through the Tone.Players class which is supposed to be a collection of Tone.Players, and start the appropriate player on click. I have tried setting an interval to dispose of a player if it is currently not playing, but that is too slow and while improves the issue it will bug out if you click really fast.

relevant reading: https://tonejs.github.io/docs/13.8.25/Player https://tonejs.github.io/docs/13.8.25/Players https://github.com/pixijs/pixi-sound/issues/65 https://github.com/Tonejs/Tone.js/wiki/AudioContext

I'm not quite familiar with the Tone.js library, but I think it's just a matter of "getting it to work" and finding the right combination of code.

llaske commented 4 years ago

Thanks for your help @pidddgy. I've fixed it in https://github.com/llaske/sugarizer/commit/5f1889a88a8ae25c750cc6743ef23aa5ccd6e49a