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

src/audio/audioplayer: do not use open() on QNetworkReply #224

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

SpaghettiBorgar
Copy link
Contributor

For some reason, calling open() on the network reply for an audio clip, clears out the buffer so you can no longer read data from it anymore, unless it is internally handled as a zero-copy buffer, which is not public.

During testing, trying to play an audio clip for a word from assets.japanesepod101.com results in an empty temp file being written. Calling bytesAvailable() before open() gives a valid number, but after the open() it becomes 0 and no data can be read.
However, trying to retrieve audio for non-existing words (With the voiced response "The audio for this clip is currently not available...") works fine and bytesAvailable() will show about 50KB even after open() is called.

After a lot of debugging, it turns out that in the latter case, the data is stored in the private downloadZerocopyBuffer of the QNetworkReplyHttpImplPrivate class, while it was stored in a "normal" buffer in the other cases, perhaps as an optimization further down the network stack that only happens with larger payloads (Most other audio clips seem to be only 2-5KB in size).

This honestly seems like a Qt bug to me, but I couldn't find any report and struggled to set up a minimal example to make one myself. I also couldn't find any example of people using open() on a QNetworkReply object. It looks like it's not something necessary to do, so removing it fixes the issue.

I tested this on Arch Linux with kernel 6.9.3-zen and Qt 6.7.1, but remember audio clips already being broken for some while before that.

For obscure and unknown reasons, calling open() on a QNetworkReply
sometimes resets the buffer in a way that only allows retrieving the
data under conditions out of our control, resulting in an empty file
being written. It doesn't appear to be necessary anyway so omitting it
works just fine.
@ripose-jp
Copy link
Owner

Good find. I hadn't noticed the bug since that code actually works with older versions of Qt. You're probably right about there being some sort of regression in more recent versions. I don't know why I called open on that code originally. It's possible that I had seen some example code do it, but I honestly don't remember.

remember audio clips already being broken for some while before that.

JapanesePod had moved their API to a new TLD and had the old API redirect. Qt by default doesn't follow redirects for security reasons, so that's probably why they were broken initially.

@ripose-jp ripose-jp merged commit 7f24f54 into ripose-jp:master Jun 18, 2024
3 checks passed
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.

2 participants