-
Notifications
You must be signed in to change notification settings - Fork 134
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
Fixes a crash that can happen when playing a corrupted file #901
base: trunk
Are you sure you want to change the base?
Conversation
@emilylaguna, I tested and I don't get the crash anymore, but I have a few of questions:
|
That is expected when the Auto Archive when played setting is enabled. This is because the player treats it as a completed episode instead of an error. While looking into this I did try an idea that would check the I didn't use this because adding that check changes how the player works for everyone and could cause more issues if for some reason there's a state where having a 0 length is okay. While I don't think that's likely based on my current knowledge, I am not confident enough to commit to it without more investigation and testing.
The episode stopping or being marked as played is not the desired behavior, ideally the episode should play correctly as it does when its being streamed. However once the episode is downloaded the EffectsPlayer will not play it, nor will forcing the player to use the DefaultPlayer, which is what is used when the episode is being streamed. The default player does throw an error and says the episode is damaged:
Looking at the issue on Sentry, this crash has only occurred for 7 users and only from 1 or 2 episodes (I was only able to see 1 episode from the breadcrumbs, but since this first appeared before that episode was even published I'm assuming there is at least 1 other episode with this issue). Because of the low counts I don't think its necessary to mark this as high priority. |
…ng the EffectsPlayer
I pushed a change that will throw an error if the EffectsPlayer fails due to a 0 frameCount. I also pushed 2 concept branches that convert the AAC m4a files to AAC ones, and adds fallback handling if a player fails to play: |
podcasts/EffectsPlayer.swift
Outdated
@@ -98,6 +98,11 @@ class EffectsPlayer: PlaybackProtocol, Hashable { | |||
// we haven't cached a frame count for this episode, do that now | |||
strongSelf.cachedFrameCount = strongSelf.audioFile!.length | |||
DataManager.sharedManager.saveFrameCount(episode: episode, frameCount: strongSelf.cachedFrameCount) | |||
|
|||
// If the count is still 0 then way may not be able to read the file correctly, so throw an error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be we instead of way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🫣 oops, fixed here: cf19c0e
podcasts/EffectsPlayer.swift
Outdated
|
||
// If the count is still 0 then way may not be able to read the file correctly, so throw an error | ||
if strongSelf.cachedFrameCount == 0 { | ||
throw PlaybackError.unableToOpenFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add tracking here to know how often this happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! I did this in a separate PR since there were some changes that were a bit more involved: #918
…y the playback_failed event
…previously tracked one
Add a playback_failed event
Fixes #898
This fixes a crash that can happen if the EffectsPlayer tries to play a corrupted episode and adds error handling to the EffectsPlayer.
What's the crash
The specific crash we get is:
This can happen if:
framePositionForTime
calculation results in123 / 0 = +Inf
What's the fix?
This PR applies an
isNumeric
check on the percent seeking value which checks for inf/nan and returns false for either of them.This results in the Inf value being ignored, and a default value being returned.
This also adds a check to verify the frameCount value is not 0 after loading it, and will throw an error if that happens.
I want to note that this only fixes this crash from happening and not the reason why a file would get to this state. We'll need to investigate this further, I have made an issue for it here: #900
To test
Checklist
CHANGELOG.md
if necessary.