TooTallNate / node-lame

Node.js native bindings to libmp3lame & libmpg123
MIT License
567 stars 113 forks source link

mpg123_feed callback not called after several new Decoders created #102

Closed dhensby closed 4 years ago

dhensby commented 4 years ago

I'm using this library to create a basic MP3 player including a skip feature. I am noticing after a few Decoder instances are created that the Decoder fails to transform data and hangs.

From my debugging I've found the issue is because the afterFeed callback passed to mpg123_feed is not called.

This is the most basic script I can create to replicate the problem:

const { Decoder } = require('lame');
const { createReadStream } = require('fs');
const Speaker = require('speaker');
const { EventEmitter } = require('events');

// execute this script via cli: `$ node ./this-file.js [path-to-mp3]`
const [node, path, song] = process.argv;

class Player extends EventEmitter {
    constructor(file) {
        super();
        this.file = file;
    }

    play() {
        this.fileStream = createReadStream(this.file);
        this.fileStream.on('error', (error) => {
            this.emit('error', error);
        });
        this.decoder = new Decoder();
        this.decoder.on('error', (error) => {
            this.emit('error', error);
        });
        this.decoder.once('format', (format) => {
            this.speaker = new Speaker(format);
            this.speaker.once('pipe', () => setImmediate(this.emit.bind(this), 'playing', this));
            this.speaker.once('close', () => setImmediate(this.emit.bind(this), 'stopped', this));
            this.decoder.pipe(this.speaker);
        });
        this.fileStream.pipe(this.decoder);
    }

    stop() {
        if (this.speaker) {
            if (this.decoder) {
                this.decoder.unpipe(this.speaker);
                if (this.fileStream) {
                    this.fileStream.unpipe(this.decoder);
                    this.fileStream.destroy();
                }
                // this.decoder.destroy();
            }
            this.speaker.destroy();
        }
    }
}

const player = new Player(song);
player.on('error', (error) => console.error(error));
player.on('playing', () => {
    // queue a stop in 5 seconds
    console.log('playing');
    setTimeout(() => player.stop(), 5000);
});
player.on('stopped', () => {
    // play the song
    console.log('stopped');
    // if we delay the play, we can get more skips but we still hang after a while
    // setTimeout(() => player.play(), 250);
    player.play();
})
player.play();

This scrips simulates a "skip" - it exhibits the same behaviour even if we have different Player instances and different mp3 files for each Player. With DEBUG=lame:decoder node ./test.js [path-to-mp3] the out put ends like this:

...
stopped
  lame:decoder created new Decoder instance +808ms
  lame:decoder _transform(): (65536 bytes) +44ms
  lame:decoder mpg123_feed() = 0 +24ms
  lame:decoder mpg123_read() = -11 (bytes=0) (meta=3) +24ms
  lame:decoder MPG123_NEW_ID3 +0ms
  lame:decoder new format: {"raw_encoding":208,"sampleRate":44100,"channels":2,"signed":true,"float":false,"ulaw":false,"alaw":false,"bitDepth":16} +34ms
playing
  lame:decoder mpg123_read() = 0 (bytes=73728) (meta=2) +21ms
stopped
  lame:decoder created new Decoder instance +5s

If I add a timeout between the stopped event firing and playing the next song, there is better performance, but still causes the same behaviour after about 65 skips.

I'm wondering if there's a "safe" way to close the decoder that I'm missing; it's implemented with the streams2 API which means there is no 'destroy' function, so maybe the mpg123 handle isn't being closed properly?


OS: Mac OS 10.15.5 Node: 10.22.0 lame (this module): 1.2.4

dhensby commented 4 years ago

After some investigation it actually looks like this is a problem with node-speaker.

Speaker.destroy() is not actually closing the speaker properly, where as Speaker.end() does.