IsSuEat / open-livestreamer-firefox-addon

Addon for firefox to quickly open stream urls in vlc using livestreamer
GNU General Public License v3.0
18 stars 7 forks source link

Deprecation #47

Closed ZatsuneNoMokou closed 7 years ago

ZatsuneNoMokou commented 8 years ago
const { Cc, Ci } = require("chrome");

You are using a deprecated api, launching livestreamer isn't possible with system/child_process?

IsSuEat commented 8 years ago

Sure is possible, I think as it is used in the plugin aswell. It was not available when the inital code was written. Thanks for pointing that out, I will look into this.

ZatsuneNoMokou commented 8 years ago

I tried with my own addon, but the command doesn't seems to be able to launch the player, I don't understand why, the livestreamer output seems normal. There's a special thing to do?

julianrichen commented 8 years ago

I remember trying to get system/child_process working initially but for whatever reason it refused to create a child process. It was refusing the arguments passed. It seems like it has been significantly worked on since I used it so maybe it was a bug before? https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/system_child_process

We currently use it to get the json string from livestreamer about the different quality levels on lib/main.js#L218 so clearly it works to a degree.

They don't say when they plan to deprecate but they do say:

If you use this API you can expect your add-on to get an extra security review by addons.mozilla.org.

That alone is worth the switch.

julianrichen commented 8 years ago

I think I sorta understand the issue. I changed runLivestreamer () to the following and placed the require system process at the top of main.js

// Run Livestreamer
function runLivestreamer(args) {
    var path,
        file;
    // Notify
    panel.width = 250;
    panel.height = 97;
    panel.port.emit("status", locale("status_lauching"));
    panel.port.emit("loading", true);
    // Get livestreamer path
    path = getLivestreamerPath();
    // Launch livestreamer
    var live = process.spawn(path, args);

    live.stdout.on('data', function (data) {
      console.log('stdout: ' + data);
    });

    live.stderr.on('data', function (data) {
      console.log('stderr: ' + data);
    });

    live.on('close', function (code) {
      console.log('child process exited with code ' + code);
    });
}

The console returned the following:

onsole.log: open-livestreamer: stdout: [cli][info] Found matching plugin twitch for URL http://www.twitch.tv/esl_csgo

console.log: open-livestreamer: stdout: [cli][info] Available streams: audio, high, low, medium, mobile (worst), source (best)
[cli][info] Opening stream: source (hls)

console.log: open-livestreamer: stdout: error: The default player (VLC) does not seem to be installed. You must specify the path to a player executable with --player.

I think the issue is because it creates a child process. This process is then isolated so it can't scan the system for vlc? We need to provide the path with --player.

However, this won't work:

args.push('--player /usr/bin/vlc');
var live = process.spawn(path, args);

And this sorta works:

args.push('--player','/usr/bin/vlc');
var live = process.spawn(path, args);

but returns:

console.log: open-livestreamer: stdout: [cli][info] Found matching plugin twitch for URL http://www.twitch.tv/elvinemod

console.log: open-livestreamer: stdout: [cli][info] Available streams: audio, high, low, medium, mobile (worst), source (best)
[cli][info] Opening stream: source (hls)

console.log: open-livestreamer: stdout: [cli][info] Starting player: /usr/bin/vlc

console.log: open-livestreamer: stdout: [cli][info] Player closed

console.log: open-livestreamer: stdout: [cli][info] Stream ended

console.log: open-livestreamer: child process exited with code 0

It's temporarily works then right before it launches it stops :/

We might need to detached the process or spawn it in a shell: https://nodejs.org/api/child_process.html#child_process_child_process_spawn_command_args_options

Detaching it will allow it to run when FF is closed, spawning it in a shell might give more control.

ZatsuneNoMokou commented 8 years ago

I know :/ it's because the version that Firefox API use of child process break completely the env part from NodeJS

That written here

The SDK versions don't: so when you specify a command you must pass in a complete path to the command or use the env option to set up the child process environment.

So... dunno, I dunno child process enough to know how defining environment work.

ZatsuneNoMokou commented 8 years ago

For information my addon is this one https://gitlab.com/ZatsuneNoMokou/live_notifier

IsSuEat commented 8 years ago

I tried it aswell yesterday, but wasnt able to launch any player either. Appending -v to livestreamer prints out any player errors that happen. This shows some crashed somewhere in firefox, but I couldn't really figure out a way around it.

It seems that spawn() will only spawn one child process, and won't allow that child to fork() (I think this somehow makes sense in the light of security)

So currently I think @julianrichen's idea with spawning a shell and executing livestreamer from there seems the best.

edit: as for figuring the path to the player out: I think it's best to provide a user configurable option to set the path to the player, maybe try the default installation folders, if this fails display a warning that the path needs to be set.

ZatsuneNoMokou commented 8 years ago

If you find how to make it work well, I'm interested

julianrichen commented 8 years ago

Some more testing.

Adding {shell: true} does nothing:

args.push('--player=/usr/bin/vlc');
var live = process.spawn(path, args, {shell: true});

Try to launch a shell and passing livestreamer as an argument does not work

args.unshift(path); // /usr/bin/livestreamer
args.push('--player=/usr/bin/vlc');
var live = process.spawn("/bin/sh", args, {detached: true, shell: true});

Detaching the process does not work:

args.push('--player=/usr/bin/vlc');
var live = process.spawn(path, args, {detached: true});

Why? Because it's not supported. Wooohooo!

console.warn: open-livestreamer: `detached` option is not yet supported for `child_process`

What about keeping the connection open?

var live = process.spawn(path, args, {detached: true});
live.unref()

Nope .unref() is not supported.

I'm out of ideas. I guess we could asking on the forums: https://discourse.mozilla-community.org/c/add-ons

ZatsuneNoMokou commented 8 years ago

Well, that api is written as "Experimental" on Mozilla doc, so, it's not surprising

julianrichen commented 8 years ago

True but the fact that they are already starting to deprecate one method while not having the other method ready is annoying. I love Mozilla and all that they stand for but they have done this kind of stuff in the past with extension signing -_-

ZatsuneNoMokou commented 8 years ago

True, I'm using a self-host version for my dev version, because using console.*() shouldn't be used "in production" build...

(And the update.rdf file that jpm generate doesn't work because too easy)

IsSuEat commented 8 years ago

Okay, I'm at the same conclusion as @julianrichen . Tried all that came to mind, but no success yet. I'd say we wait for this API to mature before actually bothering with it again.

I looked through the open defects on their bugzilla but couldn't really find a similar issue. I'm trying to get some actual details out of firefox where it fails and will eventually open a bugreport

ZatsuneNoMokou commented 8 years ago

I was thinking that it could be for security but dunno, maybe it's just a bug