Error handling

[vankasteelj]

I did notice something strange, if I pass by mistake for example:

mpv.loadFile({filename: 'this.mkv', path:'foo/bar'})

it just fails without warning me of anything, it should state that it's unable to read the file

[j-holub]

My guess would be, that MPV throws some kind of error message.

I wouldn't know how to fix this in a good way on a first glance. Because the error will definitely come over the socket, which means I would have to make this return a promise. I'd like to avoid that.

One could check if that file actually exists in the JS part itself. But that would not work for files that are actually not playable, in case some dude gives an image file to mpv or whatever.

[vankasteelj]

It depends if you see your module as a "raw" mpv library or something that could be used to become a fully-featured player. From what I've seen, you're more in the first category (you let mpv do all the work and only provide a comprehensible API) but some events suggest you might be heading towards the second category.

For example, do you see developpers using your module directly, or maybe someone (you, me, anyone) providing a mpv-player module that ships your node-mpv as a dep' ?

[j-holub]

I'm definitely in the first category. It's not really a player it was internally made to help people (in particular me with my MusicServer Meteor project :D) create great music applications.

But I am also strongly in the camp "Make it as easy as possible for the people using it". I'm a big fan of hiding complicated stuff behind simple function calls, so that the developers using it don't have a hard time.

[j-holub]

Just to clarify this. Does it crash because the file is not available? Because I think you didn't use the correct method signature. Usually it's

mpv.loadFile('foo/bar/this.mkv')

But what you actually want to me fix is, that it gives some error notice about the file not being able to load because it is not available, right?

[vankasteelj]

yes, here it doesn't error and acts like everything is fine, but the player never opens. Which is a problem when you try to write an app that uses mpv as a player^^

[j-holub]

I see that. But what I don't get is, that if you develop an app that uses mpv, the user probably has some button he can click, where he selects the file he wants to play, right? Why would you then in your code go and call the function wrong?

There is no mpv.loadFile({filename: 'this.mkv', path: 'foo/bar'}) function the this api.

What would be your preferred way to fix this? I mean I could have all the functions return promises, but I don't know if that just makes the module overly complicated to use. Another option would be to have an error event that is emitted. It just won't be linked directly to the function, but you will see a console output why it failed.

What I could definitely do, is check the optional parameters, for example the mode parameter in the loadFile() method. If it is one of replace, append or append-play and give an error if it is not.

[vankasteelj]

Many things can go wrong with a file. It could be an executable named video.mkv. The issue is that MPV doesn't spawn (or spawns and closes immediately, idk), and absolutely nothing is emmited, so whatever code I could write on my side, it would think the player has started and is playing the file.

[j-holub]

Okay now I get it. I was confused because you had a function call that just didn't exist and I was wondering what the problem was.

I kinda don't wanna have every single function in this module return a promise, but I don't see how to solve this in a different way...

[j-holub]

Of course an error event is always possible. It's just hard to react to that as a programmer. If for example you load a file, something goes wrong, and you wanna inform the user about that. You'd like to have that immediately, right?

It seems like it's going towards something like

mpv.loadFile('foo.mkv')
.then(() => {
   // handle successfull loading
})
.catch((error) => {
    // handle error
});

But that would kind of imply to make everything return a promise. I mean of course with simple calls like seek() I could immediately check if the entered value is actually a number or noted return an error. But that would make the API somewhat inconsistent.

[vankasteelj]

I don't really care about a promise or not, an error event would be fine. I always code using promises but it's not really an "off" point for me. What really matter is that at some point, your module gets back at me saying "hey you asked me to play something, I did not". Now if it's immediate, it's good. If it's with a few MS delay, it's not the end of the world. As long as it doesnt act like everything is fine.

Or i'll have to myself wrap 'isRunning' inside a dirty loop, and assume that for at least a few seconds it might just not be spawned yet, then determine if it hasnt started because something is wrong or because it's just slower than usual...

[vankasteelj]

Now the thing with promises is that you can play with them:

mpv.start().then(() => mpv.loadFile('myfile.mkv')).then(() => {
  console.log('is started');
  mpv.on('ended', mpv.quit);
  mpv.on('progress', console.log);
  mpv.on('error', (err) => throw err);
}).catch((err) => {
  console.error(err)// handles errors from start, loadFile and on('error')
})

[j-holub]

One thing that might have you confused, ist that as soon as you call start()mpv is running in idle mode in the background and isRunning() will return true.

Other than that, this does not look too bad actually.

What I meant by "not directly" was not a delay. I mean if you call loadFile() and something goes wrong, and the error messages comes via an event somewhere like mpv.on('error', (error) => {}) it is not directly coupled in the code. And it might be difficult the react the the failed file load from within the error handling code.

Since this module is doing everything over the IPC socket, even function calls like seek() or even pause() are done like that. In my experience I have never actually had any problems with these functions, but if you do something like

mpv.pause();
// something

it might happen, that something is executed before mpv is paused. Again I never had any problem with it, so for now I would prefer leaving these functions as they are.

The only functions affected by this would be

loadFile() loadSream() loadPlaylist()

and probably append()

For the others I think it's not necessary, though something like

mpv.next();
mpv.seek(50);

might fail because mpv is still loading the next song in the playlist when seek(50) is called.

I wanna avoid turning this into a promise hell, basically making no better than callbacks.

[vankasteelj]

this would be the idea flow for me, I don't have every edge case.

class Player {
  constructor (opts) {
    this.mpv = new (require('node-mpv'))(opts);

    this.mpv.on('error', this.handleError);
    this.mpv.on('quit', this.quit);
    this.mpv.on('ended', this.quit);
  }

  play (file) {
    this.mpv.start().then(() => this.mpv.loadFile(file)).then(() => {
      console.log('playing', file);
    }).catch(this.handleError);
  }

  quit () {
    if (this.mpv.isPlaying()) return this.mpv.quit();
    console.log('player closed');
  }

  handleError (err) {
    console.log('player error', err);
    this.quit();
  }
}

with this, no matter what the user does (play, pause, quit, end, trying to read a correct file, trying to read a corrupt file, mpv working perfectly, mpv not working at all...) I'll always end up inside my Player.quit() function at the end

[j-holub]

I have implemented it in the promise way and merged it into the version 2 branch. Please note, that I have renamed loadFile() to load() and removed loadStream(), since these functions were just exactly the same. A bad decision I made in the beginning of the project.

When I try to do it in the "error bubble fashion" (if you wanna call it like that) I get a warning from node

So I think we have to do it like this

mpv.start()
.then(() => {
    mpv.load('some/video/file.mkv')
   .then(() => {
        // do stuff
   })
   .catch((error) => {
      console.error(error);
   });
})
.catch((error) => {
    console.error(error);
});

So far error is only a human readable string error message, but I'm considering changing this to a JSON object, that also includes something like an error number for easier handling.

[vankasteelj]

No, your example is wrong. The good thing about promises (and thus avoid the "callback hell") is chaining:

mpv.start()
.then(() => {
    return mpv.load('some/video/file.mkv')
})
.then(() => {
    // do stuff
})
.catch((error) => {
    console.error(error);
});

And with es6/7 shorthands:

mpv.start()
.then(() => mpv.load('some/video/file.mkv'))
.then(() => {
    // do stuff
})
.catch((error) => {
    console.error(error);
});

Chaining basically is:

Promise().then((result of Promise) => {
  return anotherPromise();
}).then((result of anotherPromise) =>{
  return aThirdPromise();
}).then((result of aThidPromise) => {
  // do stuff after promise, anotherpromise and athirdpromise, if all were successful
}).catch((err) => {
  // errors in any of the chained promises
})

---

I'm gonna test that update.

[vankasteelj]

you can then truly be async with your stuff, and do nasty things like mixing promises, variables, objects, booleans or arrays

const mainUrl = 'https://raw.githubusercontent.com/00SteinsGate00/Node-MPV/master/package.json';
const devUrl = 'https://raw.githubusercontent.com/00SteinsGate00/Node-MPV/Node-MPV-2/package.json';

// async function
const whatVersion = (url) => {
   return new Promise((resolve, reject) => {
       let req = new XMLHttpRequest();
       req.open('get', url, false);

       req.onreadystatechange = function(ev) {
           if (this.status != 200) return reject('that went wrong');
           resolve(this.responseText);
       };
       req.send();
   })
};

// sync function
const getVersion = (jsonFile) => {
    let parsed = JSON.parse(jsonFile);
    return parsed.version;
}

whatVersion(mainUrl).then((body) => {
    return getVersion(body);
}).then(version => {
    console.log(version)
    if (version == '2.0.0') {
        return true; // we return a bool
    } else {
        return whatVersion(devUrl).then(getVersion); // we return a promise
    }
}).then(final => {
    if (final === true) {
        console.log('yep, main branch is V2');
    } else {
        console.log('nope, main branch is not V2, but dev branch is %s', final);
    }
}).catch(console.error);

[vankasteelj]

or even more nasty but sooo practical things:

let promiseArray = [
    whatVersion(mainUrl),
    whatVersion(devUrl)
];

Promise.all(promiseArray).then(results => {
    for (let body in results) {
        console.log(getVersion(body));
    }
});

[j-holub]

Closing this because it is fixed in Version2