Fix bug in the shutdown behaviour of the decoder

[joetoddsonos]

• This fixes a bug where decoder threads are left hanging waiting for more data to process, when the stream has already finished.
• Switching polling for thread signalling with Events, and also added a Lock when reading/writing the buffer.
• I also updated the FileEncoder to automatically detect the bit depth of the input file, and use this for encoding. An error is raised if it is not 16 or 32 bit PCM. Since this made the dtype variable redundant, I updated the version to v3.
• Added @GOAE 's suggestion of a OneShotDecoder for anyone that just wants to decode a buffer once, rather than in real time.
• Added official support for Python3.12

Closes: #22
Closes: #23
Closes: #24

[conorsleithsonos]

How do you repro the bug? I'd like to be able to verify the fix.

[joetoddsonos]

How do you repro the bug? I'd like to be able to verify the fix.

If you run python3 examples/passthrough.py test.wav and print the time before and after the call to self.decoder.finish() it sometimes happens. You can make it more likely to happen by adding a sleep just before the call to finish (so we get stuck in that loop waiting for more input data).

If you're switching between commits, make sure to run the commands in the CONTRIBUTING doc to be sure you are running that version of the code. (In fact just pip3 install . should be enough as we aren't changing the bindings).

You can also print(flac.decoder._thread.is_alive()) to check that the processing thread has finished.

[GOAE]

Any update on this? The decoder is unfortunately quite useless for batch jobs when the thread never actually joins and instead times out, which also leaves a possible race condition as to whether the decoding actually completed and an undefined situation as to whether the thread ever actually terminates and frees resources based on external code. I was using this project to batch decode FLAC and noticed it's just not viable as is to do so since I have millions of small FLAC audio clips to process and 3 seconds per is just not gonna cut it even if I use a thousand decoder instances.

The fix code looks good as to actually solving the problem in my opinion, but might I suggest one final change; loops that call sleep are very inefficient when it comes to timeliness, I think a semaphore should be used for event-driven signaling to wake the thread up when work is available rather than polling. Then signaling that same semaphore in decoder.finish() can wake the thread and cause it to exit when a flag indicates it's time to quit and the buffer is empty. This would allow the worker thread to be way more responsive than sleep could ever be without using media timers, regular sleep is likely going to wake at the best interval of the kernel's context switching (20 ms for Windows) or every 100 ms as the code is now. Just be mindful of potential spurious wakeups if a semaphore is used, but at least waking every 100 ms is better than a 3000 ms timeout in my case.

[joetoddsonos]

Thanks @GOAE, I took your recommendation and updated the thread from polling to signalling using Event's. I also noticed that there was no locking implemented on the buffer when reading/writing, so added this as well. Appreciate your help in reviewing this, if you are able to test these changes as well?

[GOAE]

Nice, you're fast! The way you implemented Event looks good, and testing the code it decodes quite quickly. In my case I'm just creating one decoder instance per FLAC clip to decode, and unfortunately with thread creation overhead, libflac API calls, and database cursor latency it slows it down quite a bit with only one parent thread. But it's still doing a couple dozen per second or so which is a big improvement; testing with a minibatch of 750 clips it took about 30 seconds to process the queue so 25 clips per second.

I used threading.active_count() to verify there was no lingering worker threads, they are indeed exiting in decoder.finish(). Didn't notice I was able to approve this pull request or I would have just done so before, but I'll approve it now.

Thanks for making the fix!

[GOAE]

The above while loop can be removed if the timeout in the below join is also removed. This way we're closer to being purely event driven; that above while loop is polling.

[GOAE]

Actually nevermind, I see that _done forces the worker to exit even if the buffer's still not empty without that loop. Could make the worker not exit until the buffer's flushed though.

[joetoddsonos]

I do agree with removing the timeout. If join hangs indefinitely it might help spot any shutdown issues before releasing.

[GOAE]

For the sake of performance testing I made a synchronous decoder that just one shots, which for reference decoded more or less as quick.
If you so feel like it you can add it to decoder.py, just also add the export to init.py and maybe add a usage example.

class StreamDecoderSynchronous(_Decoder):
    def __init__(self, write_callback: Callable[[np.ndarray, int, int, int], None], buffer: bytes):
        super().__init__()
        self._done = False
        self._buffer = deque()
        self._buffer.append(buffer)
        self._event = threading.Event()
        self._event.set()
        self._lock = threading.Lock()
        self.write_callback = write_callback
        rc = _lib.FLAC__stream_decoder_init_stream(
            self._decoder,
            _lib._read_callback,
            _ffi.NULL,
            _ffi.NULL,
            _ffi.NULL,
            _ffi.NULL,
            _lib._write_callback,
            _ffi.NULL,
            _lib._error_callback,
            self._decoder_handle
        )
        if rc != _lib.FLAC__STREAM_DECODER_INIT_STATUS_OK:
            raise DecoderInitException(rc)
        while len(self._buffer) > 0:
            _lib.FLAC__stream_decoder_process_single(self._decoder)
        super().finish()

[joetoddsonos]

@GOAE nice idea! Added this in the latest commit, but have rebranded it to a OneShotDecoder.

[conorsleithsonos]

Looks good! I like the event-driven pattern