Tanza3D / reddark

reddark, but it's in realtime
GNU Affero General Public License v3.0
298 stars 41 forks source link

Possible audio optimisations #9

Closed username-is-required closed 1 year ago

username-is-required commented 1 year ago

I noticed that (unless I’m reading the code wrong), at the moment, a new Audio object is being created every time an audio-notifiable event takes place.

https://github.com/Tanza3D/reddark/blob/7ab9b51c1fad4daaca1d4c130700ebd07a5ef2aa/public/index.js#L1-L4

From my speed-reading of the MDN docs, I believe that the audio file would be fetched from the server every time a new Audio object was created - meaning that the client would potentially be redownloading the same audio file every time a subreddit goes private/public.

Don’t take my word on this because it’s been ages since I last coded in JS, but this looks like it could be solved by having two Audio objects as variables - one for the privated sound, one for the going-public sound - and having the objects created automatically when the page is loaded rather than waiting until an event is triggered. Then, when an event occurs, calling .play() on the relevant variable rather than creating a whole new Audio object.

(I went looking through the code to do with audio processing because I noticed that there was a noticeable delay on my device between tapping “Enable sound alerts” and the actual sound alert playing, including after disabling them and then enabling them again. In the absence of finding any other code that might be causing the delay my speculation is that it might be as a result of having to fetch the audio file from the server before every use - although I may well be wrong on this as the cause as I haven’t looked into it much at all.)

username-is-required commented 1 year ago

Random thought that came to me (feel free to ignore, I have no idea if this would be a better way of doing it or not)

I’m wondering what the differences/benefits/drawbacks would be between using two new Audio(url) constructors, and having two hidden <audio> tags in the HTML itself (and then just referencing those elements in the JS). I’m guessing to a point it may just be a question of semantics, given that in both cases you’d be left with two HTMLAudioElements being referenced with variables in the JS.

Tanza3D commented 1 year ago

although your solution would work, the issue would come that if there are lots privating at once the sound may cut out or not play correctly, which we don't really want. really the browser should be caching the audio file, but if not there might be some way to load it into ram and keep it there while it goes

username-is-required commented 1 year ago

that’s a fair point, i didn’t think of that (and you’re right, it should really be caching the file - idk whats up with safari)

annoyingly HTMLMediaElement doesn’t have anything like a playing property itself — although there is apparently a way to deduce if an element is playing or not.

maybe audioSystem could be modified in such a way that means the audio files only have to be loaded once, but they won’t play the sound again if it’s already playing. i did a rough typeup below of what it could look like (assuming the two audio files are loaded in using hidden <audio> tags, and the custom property linked above is coded elsewhere):

var audioSystem = {
    playAudio: false,
    privatedAudio: document.getElementById("privated-audio"),
    publicAudio: document.getElementById("public-audio"),
    playPrivated: function () {
        if (this.playAudio == true && this.privatedAudio.playing != true)
            privatedAudio.play();
    },
    playPublic: function () {
        if (this.playAudio == true && this.publicAudio.playing != true)
            publicAudio.play();
    }
}

if that looks good to you i can submit a PR, but please test it before merging as i don’t have a PC with me and i’m doing all of this on my phone lol

username-is-required commented 1 year ago

it’s also occurred to me that given the intermittent server issues (#11), not changing the code from creating a new Audio object for each event might lead to your server being overwhelmed with audio requests when lots of subs will be going dark at once on June 12th (coming from browsers that haven’t cached the file)

edit: to be fair, even if the server wasn’t having intermittent issues, a sudden storm of audio file requests could be enough to be an effective ddos attack anyway

Tanza3D commented 1 year ago

i checked, and on both firefox and chrome it's caching the audio - if you'd like to PR this, could you maybe make it only do it on safari? you're right about the server issues, but cloudflare should be saving me a bit there and i'd prefer to keep it how it is on any browsers which cache it anyway

username-is-required commented 1 year ago

if it isn’t a problem on other browsers then i won’t worry about it then - sorry for bothering you about it! (in any case, i guess you’ll be able to tell if it’s an issue with any other browsers by how many server requests you’re getting for the audio files vs for the main website.)