melchor629 / node-flac-bindings

Nodejs bindings to libFLAC
ISC License
17 stars 1 forks source link

"Encoder has not been initialized yet" Error raised while using StreamEncoder #22

Closed jbaudanza closed 4 years ago

jbaudanza commented 4 years ago

I am using the StreamEncoder to encode PCM data from a WebSocket and send the result to an S3 Bucket. I am initializing the encoder like this:

flacEncoder = new flac.StreamEncoder({
   channels: 1,
   bitsPerSample: 16,
   samplerate: sampleRate,
   compressionLevel: 7
});

After initialization, I pass the encoder object to the S3 uploader and begin calling flacEncoder.write when I receive PCM data on the WebSocket.

Usually, this works fine. But on one occasion, I saw in my logs that this error was raised: https://github.com/melchor629/node-flac-bindings/blob/67f5d45c60427b868ba1aa27b067008bab7f6e26/src/encoder/encoder.cpp#L562

Is there something I should be doing to wait for the encoder to initialize before streaming data to it? I didn't see anything in the docs or the interface.

Thanks for the great module! It's been very helpful.

melchor629 commented 4 years ago

Hi, thanks for raising the issue. Let's see...

When you use one of StreamEncoder, FileEncoder, StreamDecoder or FileDecoder, the encoder/decoder is initialised when the first write/read is requested from the stream (they are all node.js streams). Talking about StreamEncoder, the encoder is initialised inside the _transform method:

https://github.com/melchor629/node-flac-bindings/blob/67f5d45c60427b868ba1aa27b067008bab7f6e26/lib/encoder.js#L122

It is done before doing any encoder call, and according to node.js documentation, the stream is paused until I call callback (which is done when something fails, or after sending the data to be processed by flac). So, theorically only one _transform method is running at the same time.

Well there is also the _flush method, that is called by node when you (for example) call flacEncoder.end(). This method, according to the docs, is called when there is no more data to read (so after _transform has been called and all their callbacks called back). In the _flush method is where the encoder is de-initialised, but because the stream is being closed, it should not be a problem.

With that facts, you got the Encoder has not been initialised yet error for some reason I cannot imagine currently. So if you are able to provide some stack traces or logs, would be helpful.

Something I never had is some debug logs for the library, so maybe today is a good day to add them to track this kind of weird issues (I will provide more information when I have this done).

Once again, thanks, and glad to hear you like the project :)

jbaudanza commented 4 years ago

Ah ok. I think I see what's happening. My client is closing the connection before sending any PCM data, so _write is never being called. It looks like this:

  1. Client opens connection StreamEncoder() is created stream is passed to S3 Uploader to start streaming to S3. (not sure if this is relevant)
    1. Client immediately closes connection without sending audio data flacEncoder.end() raises 'Encoder has not been initialized yet' error.

Thank you for your help!

melchor629 commented 4 years ago

Mmmm that makes a lot of sense in fact! That's a good point.

I will create a fix for this, so if for some reason, the user does not provide any data (like in your case), send an empty flac file or something like this.

jbaudanza commented 4 years ago

Yeah, I actually think you could argue this is not a bug. I can see two possible "fixes" in this case:

I think both behaviors would be reasonable.

melchor629 commented 4 years ago

Finally implemented the "no data emitted" fix for this ce03102b66821cfa912fb186166f519bc4e8c0ec. Also added tests to cover this scenarios for StreamEncoder, FileEncoder and StreamDecoder (did not see any way to replicate this in FileDecoder).

Should not error anymore when calling .end() without writing anything to the stream :)

Thanks for everything, really helped to improve the library. Closing the issue for now. If you see the same error after the release, feel free to reopen it. I will publish a new version later this weekend.

melchor629 commented 4 years ago

Hi again, I published a new version of the package 2.5.0 with this fix. Just to let you know :)

jbaudanza commented 4 years ago

Thank you so much. I will deploy it and let you know if I encounter and more issues