fix: SymphoniaDecoder::current_span_length now returns None instead of Some(0)

[jgraef]

Fixes #832

My first idea was to make current_span_len decode a new packet if the buffer is empty, but current_span_len only gets an immutable borrow of self and thus can't do that. As a fix I just added a check that returns None instead of Some(0).

After I applied this patch the music started playing 🎶

[roderickvd]

Thank you for the PR to go with your issue 🚀

While this fix works, I rather suggest that in next() we loop / advance until we got a packet with actual samples or the source is exhausted. That will be more correct than masking Some(0).

Also if you would add a changelog entry?

[jgraef]

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 next, we'd check if the buffer is now exhausted, and if it is, we'd fetch the next?

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 current_span_length if you have never called next on the decoder. We'd also have to run that decode loop when the decoder is initialized and really anywhere where we could end up with an empty buffer.

I can work on this, but I would first need to read and understand the code more thoroughly.

[roderickvd]

I see your point. A complete fix is in #786 but was rejected.
See: https://github.com/RustAudio/rodio/blob/feat/comprehensive-seeking-and-bit-depth/src/decoder/symphonia.rs

The issue with returning None is that once you have, filters later in the chain may never check span boundaries again (because they start assuming they don't have to).

[jgraef]

Did you link the right PR? It seems unrelated.

The issue with returning None is that once you have, filters later in the chain may never check span boundaries again (because they start assuming they don't have to).

Imho it's better than returning Some(0) ;)

[roderickvd]

Did you link the right PR? It seems unrelated.

Corrected: #786

[jgraef]

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 init (like Iterator::next does).

The other bug: init checks if the packet belongs to the selected track, while next doesn't. I added a check for this in next.

I'd like to merge both loops into a single function/method, but they have slightly different error handling.

[jgraef]

I also wanted to write a test for this specific bug. Does rodio have an established way of generating small audio files for this?

[roderickvd]

We love tests, but don't have any standard way of generating audio files.