Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

POC: nodejs control plane (Feedback wanted) #1533

Draft
wants to merge 66 commits into
base: master
Choose a base branch
from

Conversation

Julusian
Copy link
Member

@Julusian Julusian commented Apr 24, 2024

I am very interested in feedback from others on whether they think this is a good idea or not.
The code is far from complete or ready, this is still very much at the proof of concept stage.

If anyone has any questions or concerns please share them here. Or you can PM me on the forum if you have something you would prefer not to say/share publicly.


I've had this idea for a while now, which is to replace the C++ AMCP implementation with nodejs. This spawned from thinking about #826 where the scope grew quite a lot, and frustration with the current AMCP parsing, particularly for producers/consumers which have many properties. Additionally I was thinking about adding a http api as an alternative to AMCP, but didn't find a http server that looked decent and wouldn't be a burden as a dependency which also had an openapi generator.

This nodejs control plane would provide a few key benefits:

  • It makes supporting other protocols much easier, the whole nodejs ecosystem can be utilised
    • Such as http with openapi definitions.
    • Adding security will be easier and can be more easily verifiable
  • It makes the control/api layer more accessible to developers
    • Perhaps to make a custom api for a locked down/limited playout system
    • There are a couple of github issues talking about server-side scripting, which would be more achievable with this.

Also some downsides:

  • it adds complexity in translating nodejs types to c++
    • there is a risk of new bugs being introduced there, but that is the same for any new feature.
  • this complicates the building and distribution.
  • there is more risk of old versions of casparcg being unbuildable in the future. I'm not sure how much more, based on my experience of compiling 2.0.7 which needed old tools a couple of years ago.

A key thing to note is that no frames go near the nodejs portion. When loading a producer we do pass the pointer to nodejs, but not in a way that it can be inspected or operated on. So this should have no impact on the ability of caspar to do playout well. This only impacts the amcp part, and producer/consumer creation up until they are added to the channel.

This does run if compiled from source (on linux), and existing clients can connect and work as expected.
If we go ahead with this, more work is needed to:

  • Figure out packaging/distribution
  • Ensure everything AMCP is implemented
  • Reimplement OSC (data is in js already, just needs piping into the osc library)
  • General tidying of the new code, it is quite a mess
  • Finish the config xml parsing, and config propogation to modules
  • Remove the remaining amcp parsing from modules, nodejs should handle that with it generating a ptree structure to be used by the c++
  • Consider combining the scanner into this nodejs layer, probably as a worker_thread. That will simplify setup and deployment, and being in a thread will minimise risk of impact.

@jpc0
Copy link
Contributor

jpc0 commented Jun 11, 2024

Is it wise to directly depend on nodejs? This effective create a node addon and has the casparcg server depend on that. I haven't spent a lot of time looking at this and definitely will want to but is it at all feasible to rather export a C API that then get called from the node side. That way any future implementation isn't restricted to nodejs and can be done in golang/C++/swift/zig/rust or whatever language has compatibility with C.

I'm not against this at all. I find the current AMCP parsing impossible to reason about so any change is probably for the better.

@Julusian
Copy link
Member Author

Is it wise to directly depend on nodejs? This effective create a node addon and has the casparcg server depend on that.

Yes, this does effectively change casparcg to be a nodejs library requiring the use of that to run it.
That is a large part of my hesitation to continue with this, but also the purpose of this.

I chose nodejs as that it the main language I am using these days and my hope is that as many users will need to know some js to be able to make html graphics, they can reuse the same knowledge to understand this new layer of the server.
Of course if there is a good reason to avoid nodejs and consider a different language, I am open to that.

I haven't spent a lot of time looking at this and definitely will want to but is it at all feasible to rather export a C API that then get called from the node side. That way any future implementation isn't restricted to nodejs and can be done in golang/C++/swift/zig/rust or whatever language has compatibility with C.

The nice thing about this api exposed to nodejs, is that because it is being wrapped in nodejs types, it should be impossible (assuming no bugs in that translation layer) to cause any memory issues/crashes in the c++ portion.
Doing this as a c api, will mean that the caller has to consider memory safety of pointers and things, which I think will make the control plane be a lot less approachable.
In theory nodejs might be able to use a c api like this, through some ffi library, but then there will be a layer of c level programming written inside js which feels horrible.

@jpc0
Copy link
Contributor

jpc0 commented Jun 12, 2024

I chose nodejs as that it the main language I am using these days and my hope is that as many users will need to know some js to be able to make html graphics, they can reuse the same knowledge to understand this new layer of the server.

I find, at least for me, there is a large enough API difference between node and the browser that I need to relearn a lot, however I can agree with the sentiment that JS is significantly more popular than C++ and more people are likely to know the language in general.

The nice thing about this api exposed to nodejs, is that because it is being wrapped in nodejs types, it should be impossible (assuming no bugs in that translation layer) to cause any memory issues/crashes in the c++ portion.

I can see that, I agree with finding out what the users want and to my knowledge the largest users are probably using Superconductor or Sofie which are both in JS which means it makes a lot of sense to stick to JS there.


I've looked around some more in this branch and I like a lot of it. If I'm understanding correctly this is effectively the entire "external" interface into casparcg server. It's really nice to see a it all in one place that is very easy to trace. It also makes it much easier to understand what would be needed if I wanted to for instance fork and do a C API version...

Consider combining the scanner into this nodejs layer, probably as a worker_thread. That will simplify setup and deployment, and being in a thread will minimise risk of impact.

Another thing I would love is to have the documentation ( read wiki but doesn't need to be a wiki, could be a website served by node ) pulled into this repo as well. There are so many undocumented capabilities that when trying to work in the codebase you need to keep in mind. Being able to check that any changes are also document at pull request time would likely be an amazing benefit.

Two easy examples is the key + fill streams (

std::stable_sort(av_streams.begin(), av_streams.end(), [](auto lhs, auto rhs) {
return lhs->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && lhs->codecpar->height > rhs->codecpar->height;
});
std::vector<AVStream*> video_av_streams;
std::copy_if(av_streams.begin(), av_streams.end(), std::back_inserter(video_av_streams), [](auto s) {
return s->codecpar->codec_type == AVMEDIA_TYPE_VIDEO;
});
// TODO (fix) Use some form of stream meta data to do this.
// https://github.com/CasparCG/server/issues/832
if (video_av_streams.size() >= 2 &&
video_av_streams[0]->codecpar->height == video_av_streams[1]->codecpar->height) {
filter_spec = "alphamerge," + filter_spec;
}
) in the ffmpeg producer and the low bandwith mode(
const bool low_bandwidth = contains_param(L"LOW_BANDWIDTH", params);
) in the NDI producer which are both undocumented. The ffmpeg producer example actually adds a decent amount of complexity to the code whereas the ndi producer doesn't but I'm sure there are other examples.

Likewise in the wiki there is the ability to do batched commands which I can't find any reference to in the current codebase or this branch of yours so the wiki is just blatantly wrong on that front, at least for 2.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants