Fix remaining issues with playback stopping intermittently #1080
Conversation
|
Got some AbortError in Firefox, but the next song started playing just fine. Not sure what its abortin as it happens after the pause and ended events. |
|
The AbortError might be due to the fact that currentTime is changed right after loading a new song as aluded to here: https://bugzilla.mozilla.org/show_bug.cgi?id=1507193#c4. Why change currentTime to position when loading a new song? |
|
One thing I'm still experiencing is abrupt skips. I.e. the next song starts playing before the current one is over. I wonder if that's because I'm on a degraded network separate from the server and its missing data during which the onended event is fired. This link maybe supports the above hypothesis: https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/ended_event
However I think this behavior isn't directly related to playback stopping which I have yet to see with this patch. |
|
Tangent: Looks like not providing content length for transcoded music does the following in chrome: Not sure why mediaelement doens't do that in firefox. |
|
Re: changing Re:Chrome appearance, I noticed the same thing. I usually use FF, and it only flashes "Live Broadcast" for a fraction of a second before showing the progress bar. (I personally like having the progress bar, but it's not related to the PR, I think.) Re: the AbortError you saw, can you check that the data streamed from Airsonic is complete? Did you check media files with transcoding or without? I can reliably reproduce the issues I experience with FLAC files, less so with files already in MP3. |
|
This PR has been working fine for me, but I have only compared it to 10.3.1. |
8663c8f to
e8fdb50
Compare
|
Nice, thank you very much for testing this! I haven't seen any playback errors anymore since using this either, so we're on a good track. All: I rebased/edited the commit bit to make that ready for merging, feel free to suggest improvements. |
|
Shouldn't this be fixed upstream in mediaelement.js? |
|
Not sure, we were really using it incorrectly before this commit. And implementing the promises API correctly is probably a big change considering how MEJS works right now. |
|
I guess we can get this merged as soon as you clean the comments up? |
e8fdb50 to
e3c661e
Compare
|
Yup, go ahead! |
|
As soon as you remove the comments :P |
|
Ah, I thought you meant the PR comments! The fact that MEJS had a completely different interface than HTML5 ME threw me off at the beginning, I'd rather leave a big comment explaining that. I'll remove the debug lines but leave the rest, does that sound good? |
|
Would be nice to ship this asap in 10.3.2 because it is getting really annoying! |
|
I just tested this PR + the current state of master @ 4f59c72 . On Firefox 67.0, debian 10 |
|
But comments, in mean the commented debug lines, not the useful ones, sorry :) @jooola Could it be a caching issue in your browser? |
Nope, tried again with a fresh browser. |
|
Is transcoding enabled? Can you try running a tool like mp3check (if
playing as an MP3 stream) on the whole data Airsonic is playing?
…On Wed, 5 Jun 2019, 16:41 Jonas L., ***@***.***> wrote:
@jooola <https://github.com/jooola> Could it be a caching issue in your
browser?
Nope, tried again with a fresh browser.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1080?email_source=notifications&email_token=AAEVZWWYWMOKCOQMHI3U7G3PY7GA3A5CNFSM4HPK7HEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW75VBI#issuecomment-499112581>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEVZWWDNNFZ4MFHWDJ66BTPY7GA3ANCNFSM4HPK7HEA>
.
|
|
I only removed the flac transcoding since it was broken, I will add it back. I'll check this with your tool (if it exists on Linux). But even with flac disabled it shouldn't raise anything right ? |
e3c661e to
3a537f4
Compare
|
If the original MP3 file is corrupted (even if it's very slightly, at the end) I think this error can appear. At least Debian (https://packages.debian.org/source/sid/mp3check) and Arch (https://aur.archlinux.org/packages/mp3check/) have packages. Let me know if you find something! Anyway, debug lines removed! |
|
So I checked my mp3 file and the only error I get is: But I also noticed that if the song isn't played long enough everything is working fine. Also the error happen on flac files, even if trans-coding for flac has been disabled. |
|
I didn't have a lot of time to work on this last week, real life happened ;) I am confident in the first commit (been running my Airsonic instance with it for a few weeks now), what do you think about creating a new PR with it as a workaround? This PR can stay as our main testing branch until we manage to completely fix this in MEJS. |
|
Why not merge this entire PR? We can fix the MEJS thing later as the firefox AbortError doesn't prevent smooth playback. |
|
After building latest master with fxthomas:try-to-fix-playback-intermittent-stops merged in, I have been listening to a few albums and did not have any problem with next song not playing. A strange behaviour I am observing is that the last song of the list is repeating (i.e., as soon as it is finished, it restarts from the beginning). |
|
@Adnn 5e406f0 was supposed to fix it, but apparently it's still a bit racey and doesn't work everytime (including for me). @muff1nman @jvoisin I'll remove the tentative fix for the loop issue @Adnn was talking about as well as the debug logs in the JS console, and we can merge it. |
8e3ba43 to
5183741
Compare
This commit works around a race caused by some of our JS code trying to run play() for the next song while the MEJS player is still cleaning up the last song. MEJS issue: mediaelement/mediaelement#2650
This commit is kind of a hack to force MediaElement.js to show a progress bar when we are loading a song. Normally, in vanilla HTML5 MediaElement, we'd explicitely call 'play()' when loading a song. Here, we cannot ensure playback will work well if we don't wait for the 'canplay' event to be fired, but the progress bar won't show up without 'play()'... This commit emits a fake 'waiting' event to let the MEJS know that it should update the UI.
5183741 to
2e01342
Compare
|
Okay, ready to merge as soon as Travis finishes testing it! |
|
So...as the resident noob to Github, does this mean that I can now go back to the base airsonic...repo? Am I using words right? hahahahahaha I am still running your test branch and just been following this thread. :p |
|
It means you can use the master branch now. However there is no release with this change yet so you'd need to build master yourself. |
|
Perfect. Thank you 👍 |
|
Still stopping with Firefox. Works fine in Edge and Chrome. Also got an error while compiling. Had to shut off testing. |
|
Please do tell us more about this error |
I bypassed it by running Sorry had to run it again to get the error. |
|
Your stacktrace looks completely unrelated to the playback issue. |
|
I think that worked hahaha. Sorry I just wanted to post it here cause it happened right after the changes got put back into the master. I wasn't sure if it was related. I just hit create new issue and copy and pasted the error there. |
This commit is hopefully the final fix on Airsonic's side for airsonic#685. It also fixes airsonic#1160, which was caused by temporary workarounds introduced in airsonic#1080 while we were looking for a solution. The root cause of the issue is the fact that, when we go to the next track in an Airsonic play queue, we change the media source in the `ended` event. In MEJS, this translates as the following two things: * In Airsonic's 'ended' event, we change the media source (set the `src` attribute) and call the `load()` method, followed by the `play()` method. * The 'ended' event was also used internally by the MEJS player, and one of these internal uses called the `pause()` method (presumably in order to make sure that playback was stopped on some media renderers). Unfortunately, the order in which these events are called depends (in all modern browsers) on the order in which they are registered. In our case, the first one is registered inside the `<body>` tag, but the second one is registered with `$(document).ready(...)`. This means that the first event handler is called before the second. This means that, in some cases (when we're unlucky, hence the seemingly random nature of the bug), `pause()` is called after `load()` but before the media has finished loading. Apparently, this causes the `AbortError: The fetching process for the media resource was aborted by the user agent at the user's request.` message to appear (which indicates exactly what's described in the last paragraph), and the playback of the next song is aborted.
This commit is hopefully the final fix on Airsonic's side for airsonic#685. It also fixes airsonic#1160, which was caused by temporary workarounds introduced in airsonic#1080 while we were looking for a solution. The root cause of the issue is the fact that, when we go to the next track in an Airsonic play queue, we change the media source in the `ended` event. In MEJS, this translates as the following two things: * In Airsonic's 'ended' event, we change the media source (set the `src` attribute) and call the `load()` method, followed by the `play()` method. * The 'ended' event was also used internally by the MEJS player, and one of these internal uses called the `pause()` method (presumably in order to make sure that playback was stopped on some media renderers). Unfortunately, the order in which these events are called depends (in all modern browsers) on the order in which they are registered. In our case, the first one is registered inside the `<body>` tag, but the second one is registered with `$(document).ready(...)`. This means that the first event handler is called before the second. This means that, in some cases (when we're unlucky, hence the seemingly random nature of the bug), `pause()` is called after `load()` but before the media has finished loading. Apparently, this causes the `AbortError: The fetching process for the media resource was aborted by the user agent at the user's request.` message to appear (which indicates exactly what's described in the last paragraph), and the playback of the next song is aborted.
This commit is hopefully the final fix on Airsonic's side for airsonic#685. It also fixes airsonic#1160, which was caused by temporary workarounds introduced in airsonic#1080 while we were looking for a solution. The root cause of the issue is the fact that, when we go to the next track in an Airsonic play queue, we change the media source in the `ended` event. In MEJS, this translates as the following two things: * In Airsonic's 'ended' event, we change the media source (set the `src` attribute) and call the `load()` method, followed by the `play()` method. * The 'ended' event was also used internally by the MEJS player, and one of these internal uses called the `pause()` method (presumably in order to make sure that playback was stopped on some media renderers). Unfortunately, the order in which these events are called depends (in all modern browsers) on the order in which they are registered. In our case, the first one is registered inside the `<body>` tag, but the second one is registered with `$(document).ready(...)`. This means that the first event handler is called before the second. This means that, in some cases (when we're unlucky, hence the seemingly random nature of the bug), `pause()` is called after `load()` but before the media has finished loading. Apparently, this causes the `AbortError: The fetching process for the media resource was aborted by the user agent at the user's request.` message to appear (which indicates exactly what's described in the last paragraph), and the playback of the next song is aborted.
This commit is hopefully the final fix on Airsonic's side for airsonic#685. It also fixes airsonic#1160, which was caused by temporary workarounds introduced in airsonic#1080 while we were looking for a solution. The root cause of the issue is the fact that, when we go to the next track in an Airsonic play queue, we change the media source in the `ended` event. In MEJS, this translates as the following two things: * In Airsonic's 'ended' event, we change the media source (set the `src` attribute) and call the `load()` method, followed by the `play()` method. * The 'ended' event was also used internally by the MEJS player, and one of these internal uses called the `pause()` method (presumably in order to make sure that playback was stopped on some media renderers). Unfortunately, the order in which these events are called depends (in all modern browsers) on the order in which they are registered. In our case, the first one is registered inside the `<body>` tag, but the second one is registered with `$(document).ready(...)`. This means that the first event handler is called before the second. This means that, in some cases (when we're unlucky, hence the seemingly random nature of the bug), `pause()` is called after `load()` but before the media has finished loading. Apparently, this causes the `AbortError: The fetching process for the media resource was aborted by the user agent at the user's request.` message to appear (which indicates exactly what's described in the last paragraph), and the playback of the next song is aborted.
This commit is hopefully the final fix on Airsonic's side for airsonic#685. It also fixes airsonic#1160, which was caused by temporary workarounds introduced in airsonic#1080 while we were looking for a solution. The root cause of the issue is the fact that, when we go to the next track in an Airsonic play queue, we change the media source in the `ended` event. In MEJS, this translates as the following two things: * In Airsonic's 'ended' event, we change the media source (set the `src` attribute) and call the `load()` method, followed by the `play()` method. * The 'ended' event was also used internally by the MEJS player, and one of these internal uses called the `pause()` method (presumably in order to make sure that playback was stopped on some media renderers). Unfortunately, the order in which these events are called depends (in all modern browsers) on the order in which they are registered. In our case, the first one is registered inside the `<body>` tag, but the second one is registered with `$(document).ready(...)`. This means that the first event handler is called before the second. This means that, in some cases (when we're unlucky, hence the seemingly random nature of the bug), `pause()` is called after `load()` but before the media has finished loading. Apparently, this causes the `AbortError: The fetching process for the media resource was aborted by the user agent at the user's request.` message to appear (which indicates exactly what's described in the last paragraph), and the playback of the next song is aborted.
|
If some of you can go over at #1254 and let me know if you encounter playback issues with it, I would appreciate it. I don't have any on my machines, but additional testers are nice! |
This commit is hopefully the final fix on Airsonic's side for airsonic#685. It also fixes airsonic#1160, which was caused by temporary workarounds introduced in airsonic#1080 while we were looking for a solution. The root cause of the issue is the fact that, when we go to the next track in an Airsonic play queue, we change the media source in the `ended` event. In MEJS, this translates as the following two things: * In Airsonic's 'ended' event, we change the media source (set the `src` attribute) and call the `load()` method, followed by the `play()` method. * The 'ended' event was also used internally by the MEJS player, and one of these internal uses called the `pause()` method (presumably in order to make sure that playback was stopped on some media renderers). Unfortunately, the order in which these events are called depends (in all modern browsers) on the order in which they are registered. In our case, the first one is registered inside the `<body>` tag, but the second one is registered with `$(document).ready(...)`. This means that the first event handler is called before the second. This means that, in some cases (when we're unlucky, hence the seemingly random nature of the bug), `pause()` is called after `load()` but before the media has finished loading. Apparently, this causes the `AbortError: The fetching process for the media resource was aborted by the user agent at the user's request.` message to appear (which indicates exactly what's described in the last paragraph), and the playback of the next song is aborted.
This commit is hopefully the final fix on Airsonic's side for airsonic#685. It also fixes airsonic#1160, which was caused by temporary workarounds introduced in airsonic#1080 while we were looking for a solution. The root cause of the issue is the fact that, when we go to the next track in an Airsonic play queue, we change the media source in the `ended` event. In MEJS, this translates as the following two things: * In Airsonic's 'ended' event, we change the media source (set the `src` attribute) and call the `load()` method, followed by the `play()` method. * The 'ended' event was also used internally by the MEJS player, and one of these internal uses called the `pause()` method (presumably in order to make sure that playback was stopped on some media renderers). Unfortunately, the order in which these events are called depends (in all modern browsers) on the order in which they are registered. In our case, the first one is registered inside the `<body>` tag, but the second one is registered with `$(document).ready(...)`. This means that the first event handler is called before the second. This means that, in some cases (when we're unlucky, hence the seemingly random nature of the bug), `pause()` is called after `load()` but before the media has finished loading. Apparently, this causes the `AbortError: The fetching process for the media resource was aborted by the user agent at the user's request.` message to appear (which indicates exactly what's described in the last paragraph), and the playback of the next song is aborted.
This is a follow-up of #1035 and #685, and I would love some testers on this!
@tari's patch helped a lot, but I was still seeing intermittent playback failures when switching songs, though not always and not with the same message...
I'm now fairly sure that this is caused by some of our JS code trying to run
play()while the player is still loading the stream, causing a race condition.I experimented a bit with the
skip(index, position)function at the core of the play queue and added some logging to the player. I came to the conclusion that some methods (I identifiedplayer.play(),player.load()or settingplayer.currentTime) will automatically start loading the stream in the background, and calling any of them while the player is still loading will crash and burn.The following screenshot shows the player load a first stream, then a a second (see both logs from
playQueue.view) ; the request is then interrupted by something else and the player does not try again.The following patch makes sure that all code paths in
skipload AND start the stream exactly once, and then start playback when the canplay event fires. This appears to fix everything for me. Tested on FF 68 and Chromium 74.