vitotai / BrewManiacEsp8266

BrewManiac on ESP8266. Only ESP8266 needed.
155 stars 71 forks source link

Custom audio error for repeating sounds. #25

Open allthepies opened 6 years ago

allthepies commented 6 years ago

I've created a MP3 file and associated sounds.json file to create custom sound alerts for BM. Currently I've got the same sound for each alert, a klaxon type sound which lasts for 2 seconds.

This works fine in Chrome, but in Firefox the cases where the sound repeats more than once causes an error to be reported in the JS console and a "beep" is played after the first instance of the sound.

The error is "error not playing" and is emitted in the following section of Javascript within the SoundBox function:

        `b.audioTimeout = setInterval(function () {
                if ((Date.now() - j) > k) {
                    if (b.audio.currentTime == d) {
                        console.log("error not playing");
                        b.audio = new Audio(b.audiosrc);
                        b.audioLoaded = false;
                        b.audio.oncanplaythrough = function () {
                            b.audio.oncanplaythrough = null;
                            b.audioLoaded = true
                        };
                        b.audio.load();
                        clearInterval(b.audioTimeout);
                        b.playing = false;
                        b.queue.unshift({
                            s: "beep",
                            t: b.beep.duration
                        });
                        checkQueue()
                    }
                }`

I suspect that it's something to do with entering the "if ((Date.now() - j) > k) " IF statement block after 1 second has elapsed and the code believing that the second invocation of the sound hasn't started.

If I modify the JS to "reset" the variable j each time the alert is played again then all works fine. Amended code again from SoundBox follows:

            `   if (b.audio.currentTime > i) {
                    b.audio.pause();
                    h++;
                    if (h < c) {
                        b.audio.currentTime = d;
                        j = Date.now();    // Added this to fix the issue
                        b.audio.play()
                    } else {
                        clearInterval(b.audioTimeout);
                        b.playing = false;
                        checkQueue()
                    }
                }`

I just added "j = Date.now();" which makes sense as you want to reinitialise the sound start time each time it is initiated.

It's an odd one, do you think you could add this "fix" to the codebase ?

Attached is a zip containing the MP3 + sounds.json which reproduce the error (in Firefox). Although the error also happens with your example audio file with the synthesised speech.

example.zip

vitotai commented 6 years ago

Audio is quite new as a new feature in HTML5. I have only played on Chrome and Safari and found that they don't behavior as what I had learned.

I've googled and tried different way, but I couldn't find a perfectly compatible way to do it.

` var time = b.meta[target.s].s; // code Deleted var played = 0; var end = time + duration; var st = Date.now(); var TIMEOUT = 1000;

        b.audioTimeout = setInterval(function() {
            //console.log("current time:"+b.audio.currentTime);
            if ((Date.now() - st) > TIMEOUT) {
                // somehow the play don't start.
                if (b.audio.currentTime == time) {
                    console.log("error not playing");
                    b.audio = new Audio(b.audiosrc);
                    b.audioLoaded = false;
                    b.audio.oncanplaythrough = function() {
                        //console.log("audio file downloaded");
                        b.audio.oncanplaythrough = null;
                        b.audioLoaded = true;
                    };`

Check the original code above. I found that sometimes the browser, Chrome or Safari, just doesn't start playing the sound. The comparison you point out is to check if the "audio time" has been changed, it is played. Please check the full source code in attached file.

In fact, I found that sometimes, Chrome just don't play the audio. It's like once in ten to twenty times. I couldn't solve it and had no clue.

BTW, I should have push the source of HTML/JS. I am not classic trained JS programmer. The code and the structure is a mess, mixing with testing HTML and PHP files. I am too lazy to organize them.

bm-sound.js.zip

allthepies commented 6 years ago

Hi Vito,

Yes I could see that the comparison against Date.now() is to check if the audio is playing or not. My point is that you should reset the "st" (start) point (to Date.now() ) each time you issue the audio.play. This seems to resolve the "error not playing" issue in FF, perhaps because otherwise for the second/third repeat of a sound, the is-the-audio-playing check happens within 100 ms of those play requests (as the starting point for st is when the first instance of the sound started) and perhaps there is then a race condition on the currentTime check vs the audio playback actually starting. By resetting the st start point then the code will only check if the new instance of the audio is playing after 1 second when everything should be more stable. My tests show this resolves the beep/error issue for repeating sounds.

Plus, a question I did have is why do you set the start time default if 0.01s if it is specified as zero in the JSON file ?

vitotai commented 6 years ago

I see your point. You are right. The timeout checking code was added after I found that sometimes the audio didn't play. Obviously, It is a bug.

Plus, a question I did have is why do you set the start time default if 0.01s if it is specified as zero in the JSON file ?

It's a hack I googled. check: https://gist.github.com/remy/753003

BTW, The audio is not yet finished. There should be a button to trigger audio downloading on iOS. I was frustrated when I couldn't solve the issue that audio does't play sometimes. I just stop and put it in the back burner.

allthepies commented 6 years ago

Great! If you could add that "fix" to the v404pre branch when you get the time then I'll apply to my test rig.

vitotai commented 6 years ago

v404pre was a mistake. I already deleted that branch. It should be v040pre: https://github.com/vitotai/BrewManiacEsp8266/tree/v040pre

allthepies commented 6 years ago

OK, I'll apply v040pre.

allthepies commented 6 years ago

Applied and works well.