js: add javascript scripting support using MuJS #4482

avih · 2017-06-06T09:17:06Z

Implements JS with almost identical API to the Lua support.

Key differences from Lua:

The global mp, mp.msg and mp.utils are always available.
Instead of returning x, error, return x and expose mp.last_error().
Timers are JS standard set/clear Timeout/Interval.
Supports CommonJS modules/require.
Added at mp.utils: getenv, read_file, write_file and few more.
Global print and dump (expand objects) functions.
mp.options currently not supported.

See DOCS/man/javascript.rst for more details.

I agree that my changes can be relicensed to LGPL 2.1 or later.

ghost · 2017-06-07T15:15:27Z

DOCS/man/javascript.rst

+                mp.dispatch_event(e);
+                wait = 0;
+            } else {
+                wait = mp.process_timers() / 1000;


Sounds like it could lead to starvation of timers (but I guess something always has to lose).

It could, but if we're out of CPU cycles to serve both events and timers, I prefer to dequeue from mpv rather than to serve timers - which don't guarantee to call back on time anyway.

It shouldn't be an issue in practice though. On a 15W mobile Haswell CPU, this can serve about 100 events/timers per ms, while the most frequent events which mpv can send, which I think is tick every vsync, say even at 100Hz, is still several orders of magnitude less than the limit.

And again, even if the limit is reached, I'd still prefer to dequeue first.

ghost · 2017-06-07T15:24:20Z

player/javascript.c

+    char *ma;  // mpv alloc -> mpv_free(..)
+    mpv_node result_node;  // a node obtained via an mpv API
+    FILE *f;   // fclose
+    DIR *dir;  // closedir


You could handle special types more elegantly by using an opaque pointer and setting a talloc destructor.

While indeed not generic, I think it serves its purpose and task very well. My hunch is also that a generic approach will add meaningful amounts of code.

Nevertheless, I'll rewrite it to be generic, and we can choose between the approaches later.

Using opaque pointers with destructors is only slightly more elegant IMO, but still means it's limited to the number of pointers the struct has, if I understood you correctly.

If we really want a generic approach, then IMO the af (previously aw) pointer should just be an empty talloc context, where we add to it as many destructed items was we like. For instance this patch does it for the FILE* member:

diff --git a/player/javascript.c b/player/javascript.c index 91a948d..92e3c3e 100644 --- a/player/javascript.c +++ b/player/javascript.c @@ -194,10 +194,24 @@ static bool pushed_error(js_State *J, int err, int def) // Functions with autofree* argument should be inserted as js C functions with // af_newcfunction and used normally, or called from C with the AF macro. + +static void delete_af_file(void *p) +{ + FILE *f = *(FILE**)p; + if (f) + fclose(f); +} + +static FILE **new_af_file(void *parent) +{ + FILE **f = talloc_zero(parent, FILE*); + talloc_set_destructor(f, delete_af_file); + return f; +} + typedef struct autofree { char *ma; // mpv alloc -> mpv_free(..) mpv_node result_node; // a node obtained via an mpv API - FILE *f; // fclose DIR *dir; // closedir } autofree; @@ -213,8 +227,6 @@ static void delete_autofree(autofree *af) { // "If node->format is MPV_FORMAT_NONE, this call does nothing." (client.h) if (af->result_node.format != MPV_FORMAT_NONE) mpv_free_node_contents(&af->result_node); - if (af->f) - fclose(af->f); if (af->dir) closedir(af->dir); @@ -298,7 +310,7 @@ static void af_push_file(js_State *J, const char *fname, int limit, autofree *af return; } - FILE *f = af->f = fopen(filename, "rb"); + FILE *f = *new_af_file(af) = fopen(filename, "rb"); if (!f) js_error(J, "cannot open file: '%s'", filename); @@ -913,7 +925,7 @@ static void script_write_file(js_State *J, autofree *af) fname = mp_get_user_path(af, jctx(J)->mpctx->global, fname); MP_VERBOSE(jctx(J), "Writing file '%s'\n", fname); - FILE *f = af->f = fopen(fname, "wb"); + FILE *f = *new_af_file(af) = fopen(fname, "wb"); if (!f) js_error(J, "Cannot open file for writing: '%s'", fname);

However, I don't think it makes the code more readable or otherwise much better. I do agree that if the struct started growing more members then a generic approach would be better at some stage, but for now, all it needs is 4 members.

Anyway, if you prefer this approach, I'll apply it to the other members too. Let me know.

ghost · 2017-06-07T15:27:32Z

player/javascript.c

+    expr;\
+    js_endtry(J);\
+    delete_autowash(aw);\
+} while (0)


I think it would be better to do something in the style of lua_cpcall. js_try seems to be a macro using setjmp, and setjmp/longjmp have extremely weird and evil semantics. That should be abstracted by a function indirection. See for example: https://stackoverflow.com/questions/1393443/setjmp-longjmp-and-local-variables

Also "autowash" is a bad name. Could at least be called autofree or so.

Also "autowash" is a bad name. Could at least be called autofree or so.

Sure.

I think it would be better to do something in the style of lua_cpcall

This is how a js_cpcall would have been implemented. If you look at the implementation of js_pcall, it's basically just like this, where it returns 1 or 0 at the places where this macro deletes the object.

I don't think I know how to implement a safe call in a cleaner way, but I'm open to suggestions.

A js_pcall would be welcome, because it isolates the tricky setjmp handling into a separate function, and does not bother every caller with those weird semantics for local variables.

So yeah, I'm asking that you change this. There aren't many AW() users anyway.

I'll make it use js_pcall. Hopefully without too much pain.

ghost · 2017-06-07T15:29:26Z

player/javascript.c

+
+    int t = fread(s, 1, n, f);
+    if (t != n)
+        js_error(J, "cannot read data from file: '%s'", filename);


This approach would fail for example when reading files from /proc.

I'll change it to read till EOF, and (quoting from IRC) "wm4: if it blows up, caller's fault".

ghost · 2017-06-07T15:31:05Z

player/javascript.c

+{
+    struct mp_osd_res r = osd_get_vo_res(jctx(J)->mpctx->osd);
+    double ar = 1.0 * r.w / MPMAX(r.h, 1) / (r.display_par ? r.display_par : 1);
+    const char *names[] = {"width", "height", "aspect", NULL};


That's a const array with pointers to mutable strings.

ghost · 2017-06-07T15:37:31Z

player/javascript/defaults.js

+    var cb = hooks[id];
+    if (cb)
+        cb();
+    mp.commandv("hook_ack", cont);


I think writing command names with underscores are deprecated? Not sure though.

I'll recheck and change if it is.

ghost · 2017-06-07T15:38:12Z

player/javascript/defaults.js

+    sect = "input_forced_" + mp.script_name;
+    mp.commandv("define-section", sect, forced.join("\n"), "force");
+    mp.commandv("enable-section", sect, "allow-hide-cursor+allow-vo-dragging");
+}


This is of course all deprecated garbage, to be replaced by nicer mechanisms one day, but you knew that.

I think I didn't know, actually. Is there a different mechanism instead of defining input sections?

No, and in fact the whole "section" mechanism should go away.

The Lua backend uses some of those deprecated things to provide things that are needed by scripts, and I think I said it more than once that the underlying mechanisms are not guaranteed to last. The Lua code also uses some private API, in particular for the OSC.

OK, but I still don't understand what this implies. We do want to support key binding I assume, and currently there's no other mechanism to do so...

Once there's a better mechanism in place, I'll change it. Fair enough?

It implies it could need some heavy maintenance. If the mechanism changes, I'd just rewrite the code in the Lua wrapper. Can I expect you'd do the same in the JS one?

Can I expect you'd do the same in the JS one?

That's my intention.

ghost · 2017-06-07T15:39:33Z

player/javascript/defaults.js

+    if (!base.length && cr[0] != "..")
+        base = ["."];  // relative and not ../<stuff> so must start with ./
+    return base.concat(cr).join("/");
+}


I have no idea what it does, but it looks pretty complicated, and Lua doesn't seem to need it.

Not sure I understand what this comment imply, or what its scope is. Are you referring to the whole normalization process? if yes, it's defined by the spec that relative require IDs are transitive and must end up with the exact same cached exports object, so the normalization must happen to comply with the CommonJS modules spec.

If it's not what you meant, please be more specific.

Yes, that was basically a request for an explanation.

Is it really minimal? Why does MuJS not provide this?

Is it really minimal?

It's as minimal as I could make it to:

Detect whether an id "path" is relative or absolute, including handling dos drives, UNC paths, etc.

If absolute, separate it to its absolute root and its relative part.

Normalize the relative part.

If it's an absolute path, re-combine the root with the normalized relative part.

(CommonJS terminology for absolute path is "top level id").

That being said, if someone has an idea how to implement it with less code/complexity/etc, I'll gladly listen.

Why does MuJS not provide this?

It's not part of ES5. CommonJS is an attempt to standardize some aspects of using javascript across different environments, where modules (i.e. require(id)) is one of them. It's out of scope for MuJS.

ghost · 2017-06-07T15:40:02Z

wscript

+        'name' : '--javascript',
+        'desc' : 'Javascript (MuJS backend)',
+        'func':  check_statement('mujs.h', 'js_setreport(js_newstate(0, 0, 0), 0)', lib='mujs'),
+    }, {


Don't they have a .pc file?

As far as I know - no, but I'll recheck.

I checked with the MuJS guys - they currently don't. If they add it at some point, I'll update wscript to use it.

ghost · 2017-06-07T15:40:54Z

Some comments, which shouldn't amount to too many changes after all.

avih · 2017-06-07T22:06:14Z

Unless I missed something, these commits should cover your comments, with the following exceptions:

It seems we're OK with leaving as is the timers starvation, mujs .pc file, input sections, and require normalization complexity (do correct me if I'm wrong).
I still didn't start the generalization of the autowash system.
WRT the js_pcall, for now I didn't use it, but the comment and usage should make it perfectly safe (it already was safe before - I did take it into consideration). Tomorrow I'll try to make it use the pcall thing.

avih · 2017-06-08T11:18:18Z

So, the last commit replaces the custom autofree object with an empty talloc context, where each function which needs it adds as many destructible items as it wants. I do value its new generic form, but overall not trilled with this additional code. I can live with it though.

Let me know if you want to keep it, revert it, or anything else with it.

As for the js_pcall, I did think more about it, and my conclusion was that using it will add considerable runtime overhead for absolutely no gain (beyond the two commits I already pushed yesterday). The thing is, that right now the macro allows calling it directly as a C function without going through the js vm.

If we're replacing it with js_pcall it will have the following overhead each time it's called:

The function needs to be a js C function and take its arguments from the vm stack.
It should be inserted to the vm before calling it.
All its arguments should be pushed to the vm when calling it.
The talloc context needs to be encapsulated in a ("heavy") userdata object and pushed as an additional argument.
The function itself needs to know and use this talloc userdata object from its stack.

And, it's an overhead which doesn't give us any additional safety compared to using the macro "correctly" i.e. not accessing modified local automatic vars after the macro completes - which the commit js: AW(..) macro: use as a sole statement in a wrapper already make very explicit by stating that it should only be used in pure wrappers.

So I strongly prefer to keep the macro as is, but if you insist, I'll change it. Let me know what you think.

ghost · 2017-06-08T11:49:00Z

player/javascript.c

-    if (af->dir)
-        closedir(af->dir);
+    mpv_node *pnode = (mpv_node*)p;
+    if (pnode->format != MPV_FORMAT_NONE)


This check is unnecessary.

ghost · 2017-06-08T11:49:28Z

player/javascript.c

+{
+    char *ma = *(char**)p;
+    if (ma)
+        mpv_free(ma);


This check is also unnecessary.

ghost · 2017-06-08T11:50:09Z

player/javascript.c

-    talloc_free(af);
+static mpv_node *new_af_mpv_node(void *parent)
+{
+    assert(MPV_FORMAT_NONE == (mpv_format){0});  // else we misdetect on dtor


You can rely on that. Unnecessary.

ghost · 2017-06-08T11:50:52Z

player/javascript.c

 } while (0)

+typedef void *autofree;


Why not just void directly?

ghost · 2017-06-08T11:51:57Z

player/javascript.c

+        mpv_free(ma);
+}
+
+static char **new_af_mpv_alloc(void *parent)


Change this also to use an argument and remove the double pointer return.

avih · 2017-06-08T13:00:40Z

The last commit is the requested functional changes. I'll change autofree to void everywhere in the next commit.

ghost · 2017-06-10T11:51:27Z

Still not good.

you have js_try in a large function
you have js_try in a macro

avih · 2017-06-10T14:15:44Z

you have js_try in a large function
you have js_try in a macro

I addressed both concerns as best as I could. If this is still not what you want, I'd appreciate some handholding.

divVerent · 2017-06-13T19:40:17Z

DOCS/man/javascript.rst

+
+(LE) - Last-Error, indicates that ``mp.last_error()`` can be used after the
+call to test for success (empty string) or failure (non empty reason string).
+Otherwise, where the Lua APIs return ``nil`` on error, JS returns ``undefined``.


Instead of "Otherwise, where", what about "whenever" (so it also applies to the (LE) functions)? Having a clear pattern of

if (functioncall(...) === undefined) { mp.msg.error(mp.last_error()); }

would be relatively little prone to errors.

Also, instead of "on error", make it "due to error".

This covers the question of mp.get_property("haha", null), which in Lua would (in similar case) return nil and an error, but here will return null (and NOT undefined). It's not a straight mapping of EVERY nil/null return value to undefined if there was an error - only those nils that are caused by errors get mapped to undefined.

divVerent · 2017-06-13T20:01:17Z

DOCS/man/javascript.rst

+
+``exit()`` (global)
+    Make the script exit at the end of the current event loop iteration.
+    Note: please reomve added key bindings before calling ``exit()``.


remove (typo)

@divVerent thanks. Good comments, and especially the docs ones. Would you be kind send PRs after it gets merged?

Implements JS with almost identical API to the Lua support. Key differences from Lua: - The global mp, mp.msg and mp.utils are always available. - Instead of returning x, error, return x and expose mp.last_error(). - Timers are JS standard set/clear Timeout/Interval. - Supports CommonJS modules/require. - Added at mp.utils: getenv, read_file, write_file and few more. - Global print and dump (expand objects) functions. - mp.options currently not supported. See DOCS/man/javascript.rst for more details.

avih · 2017-06-13T21:06:15Z

Last commit squashes everything and rebases over current mpv master.

ghost · 2017-06-14T10:30:50Z

Merged.

avih · 2017-06-14T10:33:48Z

That was a long and bumpy road, considering the first PR was december 2014 :) Hopefully will be smoother from now on. Thanks!

Hrxn · 2017-06-14T11:02:57Z

mpv is now Hipster ready..

avih · 2017-06-14T11:05:38Z

mpv is now Hipster ready..

It's always been a meme player, so this fits perfectly...

avih force-pushed the js-pr2-2 branch from 24c08fb to c5109a6 Compare June 6, 2017 09:18

ghost force-pushed the master branch from 1140cb0 to c075b48 Compare June 6, 2017 17:10

ghost reviewed Jun 7, 2017

View reviewed changes

ghost reviewed Jun 8, 2017

View reviewed changes

player/javascript.c Outdated

{

char *ma = *(char**)p;

if (ma)

mpv_free(ma);

Copy link

ghost Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is also unnecessary.

ghost reviewed Jun 8, 2017

View reviewed changes

player/javascript.c Outdated

} while (0)

typedef void *autofree;

Copy link

ghost Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just void directly?

ghost reviewed Jun 8, 2017

View reviewed changes

divVerent reviewed Jun 13, 2017

View reviewed changes

avih force-pushed the js-pr2-2 branch from 588fd6e to f3d5d6a Compare June 13, 2017 21:05

ghost closed this Jun 14, 2017

This pull request was closed.

js: add javascript scripting support using MuJS #4482

js: add javascript scripting support using MuJS #4482

Conversation

avih commented Jun 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avih Jun 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 7, 2017

avih commented Jun 7, 2017 • edited

avih commented Jun 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avih commented Jun 8, 2017

ghost commented Jun 10, 2017

avih commented Jun 10, 2017

divVerent Jun 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avih commented Jun 13, 2017

ghost commented Jun 14, 2017

avih commented Jun 14, 2017

Hrxn commented Jun 14, 2017

avih commented Jun 14, 2017

avih commented Jun 6, 2017 •

edited

avih Jun 7, 2017 •

edited

avih commented Jun 7, 2017 •

edited

avih commented Jun 8, 2017 •

edited

divVerent Jun 13, 2017 •

edited