Skip to content

Recover from FLAC decoding errors#2315

Merged
daschuer merged 2 commits intomixxxdj:2.2from
uklotzde:2.2_flac_decoding_errors
Oct 17, 2019
Merged

Recover from FLAC decoding errors#2315
daschuer merged 2 commits intomixxxdj:2.2from
uklotzde:2.2_flac_decoding_errors

Conversation

@uklotzde
Copy link
Copy Markdown
Contributor

@uklotzde uklotzde commented Oct 5, 2019

Retested corrupt files and fixed some hiccups.

@uklotzde uklotzde added this to the 2.2.3 milestone Oct 5, 2019
@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Oct 5, 2019

By chance, is this also fixing https://bugs.launchpad.net/mixxx/+bug/1846409 ?
Did @daschuer forward my affected track to you?
If not I can do so. (Don't have the ressources to built this branch and test it myself right now)

@uklotzde
Copy link
Copy Markdown
Contributor Author

uklotzde commented Oct 5, 2019

By chance, is this also fixing https://bugs.launchpad.net/mixxx/+bug/1846409 ?
Did @daschuer forward my affected track to you?
If not I can do so. (Don't have the ressources to built this branch and test it myself right now)

No, FLAC decoding worked and still works perfectly. This PR only fixes issues with really bad and corrupt files.

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Oct 5, 2019

My mistake, encoding/decoding :)

@uklotzde
Copy link
Copy Markdown
Contributor Author

uklotzde commented Oct 5, 2019

@ronso0 Nevertheless please forward the file also to me (Zulip link or e-mail), just in case.

<< "Resetting decoder after failure to skip preceding frames"
<< precedingFrames;
if (!FLAC__stream_decoder_reset(m_decoder)) {
kLogger.critical()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This crashes Mixxx, right? Doe we have alternatives?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not by default: "It exits if the environment variable QT_FATAL_CRITICALS is not empty."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If even resetting the decoder fails then we are in big trouble. Like a destructor this operation should never fail, but the API allows failure and we need to handle this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Playback was stuttering and Mixxx became almost unusable when not resetting the decoder. This is already our very last chance. If even this option fails everything is lost.

This is still only a DJ application, not the control unit of some deadly weapon. Let's keep things in perspective.

@daschuer
Copy link
Copy Markdown
Member

LGTM. Thank you.

@daschuer daschuer merged commit 47c4848 into mixxxdj:2.2 Oct 17, 2019
@uklotzde uklotzde deleted the 2.2_flac_decoding_errors branch October 17, 2019 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants