Skip to content
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

fix: crash when clicking audio insanely fast. #1424

Merged

Conversation

shenlebantongying
Copy link
Collaborator

@shenlebantongying shenlebantongying commented Mar 20, 2024

This can prevent crash of #1423

There are 2 problems

  • QThread unfinished before creating a new one.
  • audioOutput may fail to construct (???? I don't know how yet. AudioOutputPrivate seems to have some special internal).

This works, but I am not sure how to fix this perfectly yet.


I think the original code was to spawn multiple audio simultaneously, which makes no sense to humans as nobody can hear mix audios.

So there will be one and only one working audio thread, right?

@shenlebantongying shenlebantongying changed the title fix: crash when clicking play audio insanely fast. fix: crash when clicking clicking audio insanely fast. Mar 20, 2024
@shenlebantongying shenlebantongying changed the title fix: crash when clicking clicking audio insanely fast. fix: crash when clicking audio insanely fast. Mar 20, 2024
connect( this, &AudioService::cancelPlaying, thread.get(), [ this ]( bool waitFinished ) {
thread->cancel( waitFinished );
} );
thread.reset( new DecoderThread( audioData, this ) );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This reset here has the same meaning as the older thread = std::make_shared because the older code also ensure the existing one is deleted.

@xiaoyifang
Copy link
Owner

xiaoyifang commented Mar 20, 2024

I think the ffmpeg related codes can be removed totally , above qt6.5.* ? ,the ffmpeg has been the backend of qmultimedia module. in another PR?

Copy link

sonarcloud bot commented Mar 20, 2024

@shenlebantongying
Copy link
Collaborator Author

shenlebantongying commented Mar 20, 2024

ffmpeg has been the backend of qmultimedia module.

I don't know. Does open source qmultimedia ships proprietary codecs like mp3? Could this be a problem for Window build?

For Linux, there is no problem to drop ffmpeg along with Qt5. Even the old qmultimedia is good enough, as the older backend of GStreamer supports adding codecs by adding plugins.

@xiaoyifang
Copy link
Owner

So there will be one and only one working audio thread, right?

There is a case that: when users click two different audios ,such as audioA,audioB, when click the second ,the first audio should be stopped.

Can this case still be supported by this modification.

@shenlebantongying
Copy link
Collaborator Author

shenlebantongying commented Mar 20, 2024

Can this case still be supported by this modification.

Based on my testing. Yes. This PR just ensure that the older thread is finished before deleting it to avoid crash.

2 super long audio for test Archive.zip

@shenlebantongying
Copy link
Collaborator Author

Maybe we can wait next Qt LTS for consider dropping ffmpeg and utilize QtMultimedia's ffmpeg backend.

I reported this bug before. For versions below 6.5.2, QtMultimedia simply fatal crash without a way to recover if no backend is installed 😅

https://bugreports.qt.io/browse/QTBUG-114202

@xiaoyifang xiaoyifang merged commit 52c07b9 into xiaoyifang:staged Mar 20, 2024
13 checks passed
@shenlebantongying shenlebantongying deleted the fix/insanly-click-audio-crash branch March 20, 2024 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants