Repository navigation
fix: SymphoniaDecoder::current_span_length now returns None instead of Some(0) - #833
Conversation
|
Thank you for the PR to go with your issue 🚀 While this fix works, I rather suggest that in Also if you would add a changelog entry? |
|
I briefly thought about this solution too, but saw an issue with it and wanted a quick fix. Checking again, I think I don't see any issues with this solution anymore. So after we get a sample in This would almost make the loop before we get the sample unnecessary, except when the decoder is initialized with an empty buffer (and maybe other edge-cases I'm unaware of). Aha! I remember the issue I saw. Your suggestion would not fix the issue, since it would still return the incorrect I can work on this, but I would first need to read and understand the code more thoroughly. |
|
I see your point. A complete fix is in #786 but was rejected. The issue with returning |
|
Did you link the right PR? It seems unrelated.
Imho it's better than returning |
Corrected: #786 |
ae8529d to
51acc39
Compare
|
Having 2 separate loops that decode the next relevant packet is the problem here. It leads to the bug #775 and I think I spotted another one. Bug #775 is now fixed by properly skipping to the next non-empty frame in the loop in The other bug: I'd like to merge both loops into a single function/method, but they have slightly different error handling. |
|
I also wanted to write a test for this specific bug. Does rodio have an established way of generating small audio files for this? |
|
We love tests, but don't have any standard way of generating audio files. |
|
Is this still in progress? |
|
I haven't figured out a way to make a test or it. Otherwise the fix is ready to be merged, if you don't find any issues with it. |
|
In that case lets merge it, and hope nothing explodes :) Thanks for helping out! |
Every bundled sound loaded as a zero-sample buffer, so join and leave notifications played nothing. rodio 0.22 (from the dependency upgrade) decodes ogg through symphonia, whose decoder ends its first span on the zero-frame vorbis priming packet. Buffered treats that empty span as end of stream and discards the whole file, with no error logged anywhere. Decode each sound fully to a SamplesBuffer at startup instead. A play is a clone of the shared Arc of samples, so triggering stays instant and concurrent plays share one allocation. The upstream fix (RustAudio/rodio#833) is merged but unreleased after v0.22.2. A regression test now decodes every bundled sound through the same path and asserts non-silent output.
rodio 0.22's symphonia decoder keeps only the selected track while it initialises, but its iterator does not, so the first packet of an MP4's video track ended the sound after about a tenth of a second and video messages seemed silent. Pin rodio to the revision with the upstream fix (RustAudio/rodio#833) until a release carries it.
Both pins are commits on their upstream's default branch that no release carries yet: whatsapp-rust f7468ae is on oxidezap/whatsapp-rust main, after 0.7.0, and rodio e89e685 is the merge of RustAudio/rodio#833 on master (0.23.0 was cut from the 0.22 branch without it). The same revisions now come from crmne/whatsapp-rust and crmne/rodio, each held by a `zapfast` branch, so every git dependency is in a repository the maintainer controls. Cargo.lock changes only in the source URLs.
cpal 0.17's ALSA backend reported an underrun as POLLERR in a loop without ever recovering the device; cpal 0.18 recovers it (RustAudio/cpal#1122) and ends a stream whose device went away. rodio's master carries cpal 0.18.2 and RustAudio/rodio#833, which no release has together, so the crmne/rodio fork's `zapfast` branch is now master plus MicrophoneBuilder::with_error_callback, sent upstream. audio::Health takes cpal 0.18's errors: a device that went away, an invalidated stream or a missing host fail it at once, and a rerouted stream or a refused real-time promotion is logged without counting towards a flood.
cpal 0.17's ALSA backend reported an underrun as POLLERR in a loop without ever recovering the device; cpal 0.18 recovers it (RustAudio/cpal#1122) and ends a stream whose device went away. rodio's master carries cpal 0.18.2 and RustAudio/rodio#833, which no release has together, so the crmne/rodio fork's `zapfast` branch is now master plus MicrophoneBuilder::with_error_callback, sent upstream. audio::Health takes cpal 0.18's errors: a device that went away, an invalidated stream or a missing host fail it at once, and a rerouted stream or a refused real-time promotion is logged without counting towards a flood. (cherry picked from commit bb19cfe)
Voice messages and a call's microphone read a fastframe-audio input, whose audio thread hands each block to a ring the reader drains without blocking. The input reopens after failures and follows the default; a reopen in another format ends the reader's stream so it starts again. The call's microphone picker lists and opens devices by their system id. rodio now only decodes and mixes: its playback and recording features are off, and audio/health.rs, which kept rodio's stream errors from spinning, gives way to fastframe-audio's error kinds and maintain(). The rodio fork stays for RustAudio/rodio#833 until a release has it.
Fixes #832
My first idea was to make
current_span_lendecode a new packet if the buffer is empty, butcurrent_span_lenonly gets an immutable borrow ofselfand thus can't do that. As a fix I just added a check that returnsNoneinstead ofSome(0).After I applied this patch the music started playing 🎶