Broken on Node 10 and 11

[sholladay]

Hello and thanks for this awesome project!

I have been successfully using this on Node 8, but in trying to upgrade our app to Node 10, I discovered that node-opus seems to not output anything in newer versions of Node. It caused our test suite to hang for some reason. I haven't been able to make a minimal reproduction of node-opus hanging even though it seems to in our app. In the example below, it just outputs zero bytes rather than hanging. Either way, seems to be very broken.

Reproducible example

The code below is more or less the same as examples/stream-to-alsa.js, except it simply writes the decoded bytes to a file.

'use strict';

const fs = require('fs');
const ogg = require('ogg');
const opus = require('node-opus');

const oggDecoder = new ogg.Decoder();
const opusDecoder = new opus.Decoder();

fs.createReadStream('input.opus')
    .on('error', console.error)
    .pipe(oggDecoder)
    .on('error', console.error)
    .on('stream', (stream) => {
        stream
            .pipe(opusDecoder)
            .on('error', console.error)
            .on('format', (format) => {
                opusDecoder
                    .pipe(fs.createWriteStream('out.pcm'))
                    .on('error', console.error);
            });
    });

Good (Node 8)

306 Kilobytes

❯ node --version
v8.14.0

❯ node convert.js

❯ ls -lh out.pcm
-rw-r--r--  1 sholladay  staff   306K Dec  8 19:02 out.pcm

Bad (Node 10)

0 Bytes

❯ node --version
v10.14.1

❯ node convert.js

❯ ls -lh out.pcm
-rw-r--r--  1 sholladay  staff     0B Dec  8 19:55 out.pcm

[Rantanen]

I noted something similar with node-opus's own tests (as you can see the CI builds are somewhat broken). Unfortunately it seemed to be caused by node-ogg module. :|

Looked into this a bit further. This is a bug in Node:

nodejs/node#20503

The ogg DecoderStream uses highWaterMark: 0 when constructing the decoder stream. This causes Node to terminate the stream before it's fully consumed.

[Rantanen]

On the other hand, that specific case seems to have been fixed in Node v10.9, but I still have the issue with Node v10.10.

[Rantanen]

Reported in Node: nodejs/node#24915

A really dirty workaround to this is to force highWaterMark to 1 for the ogg-stream:

fs.createReadStream('input.opus')
    .on('error', console.error)
    .pipe(oggDecoder)
    .on('error', console.error)
    .on('stream', (stream) => {
        stream._readableState.highWaterMark = 1;  // Force highWaterMark > 0.
        stream
            .pipe(opusDecoder)
            .on('error', console.error)
            .on('format', (format) => {
                opusDecoder
                    .pipe(fs.createWriteStream('out.pcm'))
                    .on('error', console.error);
            });
    });

I just did that in our own unit tests:

node-opus/test/Decoder.js

Lines 42 to 48 in 9faf2e4

	oggDecoder.on( 'stream', function( stream ) {
	// Workaround to Node issue #24915
	// https://github.com/nodejs/node/issues/24915
	stream._readableState.highWaterMark = 1;
	stream.pipe( decoder );

[Rantanen]

Also reported in node-ogg (TooTallNate/node-ogg#16) and referencing here so we'll see if they apply the workaround of highWaterMark > 0 in the constructor. That would allow us to get rid of the _readableState.highWaterMark workaround.

[Rantanen]

The fix for the Node issue landed in Node master: nodejs/node@37a5e01

I don't really know how Node point releases work so can't say when that might be available in the 11.x branch. I'll leave this issue open until the fix becomes available in a Node release (mainly so I remember to remove the workaround from the Decoder tests).

[sholladay]

That's quite a rabbit hole to go down. Thanks so much for getting on top of this and quickly. :)

[sholladay]

Node 11.5 has been released with your patch!

https://github.com/nodejs/node/blob/845fdd02f40b529a47c51f88b723ebccbff9f095/doc/changelogs/CHANGELOG_V11.md#2018-12-18-version-1150-current-bethgriggs

[sholladay]

And our test suite passes now, using Node 11.5.0. Awesome work and thanks again. If there is somewhere I can donate money for this project, please let me know. :)