Streamer2 by dingodoppelt · Pull Request #967 · jamulussoftware/jamulus

dingodoppelt · 2021-02-07T12:16:05Z

This code calls ffmpeg to stream a stereo mix directly from the server either into a streaming service like icecast or a file.

pljones

OK, it will need to be made conditional compile not WIN32.

#ifndef WIN32
....code....
#endif

throughout.

The .qm files... Can you try git reset HEAD~ whatever.qm and see if that fixes it?

hoffie · 2021-02-07T22:58:38Z

+void CJamStreamer::OnStarted() {
+    // create a pipe to ffmpeg called "pipeout" to being able to put out the pcm data to it
+    pcm = 0; // it is necessary to initialise pcm
+    QString command = "ffmpeg -y -f s16le -ar 48000 -ac 2 -i - " + strStreamDest;


It might be worth considering hardening the ffmpeg call as it executes a command and includes a variable in the invocation. I fail to find a popen() alternative which takes an array of arguments instead of a string (i.e. like exec*() to system()). Unless I'm missing something, the only remaining hardening measure would be proper quoting + escaping of the variable. One might argue that the variable is currently supposed to come from a trusted context (command line), but things may change in unexpected ways sometimes. Improving this would also ensure that special chars will not break things even in well-intentioned environments.

Perhaps use QProcess?

Perhaps use QProcess?

Good idea! This should solve the potential security impact and might also get us cross-platform support for free. :)

Hm. I'm thinking you're going to want to know the path the ffmpeg binary. Ideally from either a setting or from a command line option. Better not just to launch whatever executable happens to get picked up from "ffmpeg" getting run.

the only remaining hardening measure would be proper quoting + escaping of the variable

How would I go about and do this? At the moment I just read the output part of ffmpeg from the commandline (which can be anything that ffmpeg understands like "-f mp3 icecast://source::/" or something completely different. I don't know in advance how many arguments will be given in different usecases)

Perhaps use QProcess?

Thanks for the advice! I looked into it and can't see how I could alter my code (I'm not a programmer ;) to do the same as before using QProcess. (popen creates a pipe which I can then stream the pcm data to, does QProcess offer this?) Any hints on what I would be looking for?

Better not just to launch whatever executable happens to get picked up from "ffmpeg" getting run.

I've thought about that before and come to the conclusion that it won't matter for security reasons since you can call anything named "ffmpeg". Allowing the user to specify which program to call doesn't make it more secure in my opinion, since you'd still be able to call anything you want. It'd only be useful if you wanted to call a certain build of ffmpeg besides the one already installed system-wide, but I couldn't think of a usecase where you would want to do that.

Well, it doesn't crash for me

It does for me only when I connect a client but yes, ffmpeg never gets started it seems.

Maybe we should just stick with the popen version...

I think so, too

I'm not seeing a segfault either, but I have found three problems which I have fixed in a PR to @npostavs' branch against this PR. :)

npostavs#1

Awesome! Thanks a lot for your work :)
Would that mean I can get rid of the conditional compile for win32 as well? I can't check since I have no windows machine here.

Would that mean I can get rid of the conditional compile for win32 as well?

I think so, yes, but one should test.

I can't check since I have no windows machine here.

Neither can I (Linux-only here). A first smoke test would probably be removing the ifdefs. CodeQL should probably catch if compiling works. Someone should still run a real test with ffmpeg if possible.

It doesn't compile on windows whatsoever. I've put the conditional compile back in

dingodoppelt · 2021-02-08T01:05:46Z

The .qm files... Can you try git reset HEAD~ whatever.qm and see if that fixes it?

that fixed it, thanks!

To avoid redundant copying.

pljones · 2021-02-10T21:47:46Z

Is this ready for final review?

Make CJamStreamer::process data parameter a reference

Signed-off-by: Christian Hoffmann <mail@hoffmann-christian.info>

- QProcess automatically populates argv[0]. Explicitly passing "ffmpeg" as first argument will end up as argv[1] and will break ffmpeg. - Increase ffmpeg log level to "error" so that startup errors are really output. - Add support for space-delimited multiple parameters for the --streamto argument. Signed-off-by: Christian Hoffmann <mail@hoffmann-christian.info>

Fix streamer2

dingodoppelt · 2021-02-22T14:48:47Z

Besides that, there is an open usability question: Should this feature be "ffmpeg-based streaming" or "generic PCM-on-stdin streaming"? In other words: Should ffmpeg be hardcoded? My tendency would be towards the generic solution. This would have the huge advantage that it would circumvent the splitCommand problem and would keep the implementation smaller. We could still supply an example streaming script with ffmpeg hardcoded. Any opinions?

I'd go with the generic solution as well. This wouldn't make ffmpeg compulsory and would allow for other usecases to be covered. Another advantage to the existing implementations is to be able to stop and start streaming without having to restart (reconfigure) the server to stop/start the stream or change its destination.

hoffie · 2021-02-22T20:35:22Z

Another advantage to the existing implementations is to be able to stop and start streaming without having to restart (reconfigure) the server to stop/start the stream or change its destination.

Ah, right, didn't even think of it. Cool! :)

"generic PCM-on-stdin streaming"
Would it work on Windows? The benefit of the "internal pipe" solution is that "it should", as you're managing the stream internally. I'm not sure if the server stdout would be easily available to pipe, particularly in the GUI server.

Not sure if we are talking about the same. With "generic PCM-on-stdin streaming" I meant that Jamulus opens a sub-process and streams PCM to stdin of that sub-process. I'm not referring to Jamulus' stdin. This approach is the same with or without QProcess.

Regarding Windows: When using QProcess I'd hope it could work as the code would not use anything which is documented as non-Windows-only. Should be tested, of course.

pljones · 2021-02-22T22:27:40Z

OK - that's good -- I just worry when I see "stdin" and "process" for Windows... :)

hoffie · 2021-02-24T21:54:06Z

I can just connect to the icecast stream as a listener once. The second time I try to connect it won't start playing but ffmpeg seems to be streaming nonetheless. Can anybody check to see if this behaviour is reproducible?

So, you are using --streamto to make ffmpeg send the stream to an Icecast server. When you connect to the Icecast server, the first clients gets audio data, but the second client won't. Do I understand correctly?
I can't think of any reason how the Jamulus-side implementation or ffmpeg could influence this part...?

Do you have a specific way to reproduce this? Maybe even without Icecast but with local files or anything what one could easily replicate?

dingodoppelt · 2021-02-25T00:11:24Z

Do you have a specific way to reproduce this? Maybe even without Icecast but with local files or anything what one could easily replicate?

@hoffie
To replicate the icecast issue I can't think of how I could do that with a file locally.
With an icecast server running you just launch the jamulus server and connect a client so the stream starts. Then connect to the stream to listen. You'll find the first time you connect you'll hear sound. Then disconnect from the stream and rejoin to find the stream won't start anymore. It is just working once. To listen to the stream again you either disconnect all clients from the server (which closes ffmpeg on server idling and restarts ffmpeg on the first client connecting to the server) or restart the server altogether.

pljones · 2021-03-12T19:09:55Z

So you're saying the set up is

Running Jamulus instance with potential session stream to a per session ffmpeg instance
Running Icecast instance listening for audio data and client connections
ffmpeg instance configured to output its input from Jamulus to the Icecast instance

ffmpeg will only be running during a session. Jamulus and Icecast are running all the time.

(1) Right so far?

Then a Jamulus client connects to the Jamulus server and ffmpeg connects to Icecast.

Then an Icecast client connects to the Icecast server.

At this point the Icecast client can hear the Jamulus session.

(2) Right so far?

The Icecast client disconnects.

Another Icecast client connects but now cannot hear audio.

(3) Right so far?

If that's all correct, I'd say "check the logs" to see what's reported as going on.

hoffie · 2021-03-18T22:20:15Z

@dingodoppelt In addition to what @pljones asked, can you confirm that this behavior does not occur when using some other, non-Jamulus icecast client?

I can't really think of anything which would influence this from within Jamulus or the QProcess vs. no-QProcess handling.

dingodoppelt · 2021-03-20T01:43:48Z

can you confirm that this behavior does not occur when using some other, non-Jamulus icecast client?

@hoffie: yes, my code from the original pr works.

hoffie · 2021-03-20T14:45:05Z

I have just installed icecast and tried to use this feature (10400f5, the latest state of this PR). After some fiddling with icecast config/authentication, it worked, even with multiple clients on icecast and/or on Jamulus. So, I'm not able to reproduce the issue you are describing.

Can you share some more details (icecast/ffmpeg version and config)?

$ icecast -v
Icecast 2.4.4

$ ffmpeg -v
ffmpeg version n4.3.2

I used the default icecast config and configured a single listener mountpoint.
I used ./Jamulus -s -n --streamto "-f mp3 icecast://source:hackme@127.0.0.1:8000/stream"

Some further things I've observed:

--streamto does not fail when used in Jamulus as a client mode, yet, it does not work there. I think we should fail in src/main.cpp.
When ffmpeg crashes (e.g. due to an icecast server restart), streaming will stop with no way to restart it besides re-connecting all clients and/or restarting the Jamulus server. Not sure, if/how this could be improved. We could think about restarting on failure, but this adds more complication and risks crash loops.
The current code still uses the ffmpeg-focused approach which requires command line splitting. I think the plan was to go for the generic approach and add an appropriate example script (with a ffmpeg invocation exaxmple). This will still have to be done.
We should find out why it worked for me and did not for you.

pljones · 2021-03-21T12:28:42Z

--streamto does not fail when used in Jamulus as a client mode, yet, it does not work there. I think we should fail in src/main.cpp.

The current code still uses the ffmpeg-focused approach which requires command line splitting. I think the plan was to go for the generic approach and add an appropriate example script (with a ffmpeg invocation exaxmple). This will still have to be done.

We should find out why it worked for me and did not for you.

👍

When ffmpeg crashes (e.g. due to an icecast server restart), streaming will stop with no way to restart it besides re-connecting all clients and/or restarting the Jamulus server. Not sure, if/how this could be improved. We could think about restarting on failure, but this adds more complication and risks crash loops.

Personally, I'd leave this out for now. I need to get a proposal written for better server management. For example, at the moment, the SIGUSR1/2 for the jam recorder isn't portable. If the jam server runs out of space, it simply raises an exception to alert the server operator (that is, it deliberately crashes the server). Neither is particularly nice.

So I'm hoping to have something like a separate "management" thread that non-core service can communicate with. That should enable better and deeper status monitoring and control without impacting the main server too much.

(I'm hoping a similar approach can be taken with the client, too, to allow a nicer "headless" mode there.)

(I live in hope my day job work load will ease a bit so I've brain-space left to work out the details, along with everything else!)

olibac · 2021-03-25T18:33:13Z

Would be possible to redirect every client channel arriving to server to ReaRoute? (this is related to #1305 @ann0see mentioned a few days ago).
The same way every channel output is redirected to a file when recording maybe it could be redirected to ReaRoute (or any virtual cable) so you can use Reaper (or any DAW) to mix every channel, add effects (eq, compressor, etc) and deliver final streo mix to a streaming service or to record live session with a better sounding mix.
I think it could be useful for audio techs as they can have more control on the "tracks" in order to improve quality of the final mix.

pljones · 2021-03-25T18:43:20Z

The client can direct all of it's two outputs (i.e. left and right channels) wherever you want.

It only has the stereo mix. That's not going to change in the lifetime of "Jamulus" as "Jamulus". If you need a multi-channel, locally mixed solution, you'll need to look elsewhere.

olibac · 2021-03-25T19:03:08Z

@pljones Thank you for the response. @ann0see explained to me the mixing is done in the server and client only has the stereo mix, that's why I am asking if it can be done in the server (not the client). The same way every channel in the server is redirected to a file when recording audio maybe it could be redirected to a virtual cable in order to be able to mix in a DAW.

pljones · 2021-03-25T19:17:19Z

The server isn't attached to any audio hardware currently. Doing so would slow it down. Which would mean the number of clients a single server could support would drop. The jam recorder outputs each audio file unsynchronised. If you're trying to get multichannel realtime audio for mixing externally, you need it synchronised, which means keeping in time with an external clock.

Again, that's unlikely to happen. I've already said, someone could take the existing jam recorder quite easily and replace each file with a stream. What each stream was attached to would then be up to the user. But they wouldn't be "in time with each other".

olibac · 2021-03-25T19:21:34Z

Thanks for the clarification about synchronization. And thanks for the efforts too.

WolfganP · 2021-03-25T20:06:47Z

I did some tests with Reastream VST plugins on the server side (see #146 (comment) and subsequent comments) but never had the time to make a workable prototype with the server (I even worked out to develop a Wireshark dissector for the protocol https://github.com/WolfganP/reastream-wireshark but that was the end of it due other time consuming tasks)

hoffie · 2021-04-29T19:14:02Z

@dingodoppelt Are you still planning to work on this? If not, would you be ok with someone else taking over?

dingodoppelt · 2021-04-29T19:47:39Z

@hoffie
I actually worked on the streamer by introducing a protocol message to turn streaming on and off on the fly. but that commit went into my old code since i have problems getting the changes done to this pull request to work. i am using my streaming code on about 20 servers and combined it with a feature to chat with the outside world for which I had to introduce a second protocol message. that is not something i'd like to have merged since it is too much of a "special usecase" and hacked together to have something working for me. I'd rather see something like an API (accessible through a management thread) and dedicated protocol messages for controlling things.
I really think that extensions like this are better integrated into something like @pljones proposed:

So I'm hoping to have something like a separate "management" thread that non-core service can communicate with. That should enable better and deeper status monitoring and control without impacting the main server too much.

I thought that this pr might be better off being held back until something like a management thread is introduced. that would make the stream or a chat with the outside modules or plugins. I don't really know if it is wise to merge an in-between since ffmpeg would have to go first ( and i'm still not a programmer ;)
We should first make clear what is really necessary to have a stream and possible future extensions working without messing up the code too much.

hoffie · 2021-05-10T20:46:15Z

Thanks for your reply. In my opinion, it should be possible to bring this into mergable state with little effort. #967 (comment) outlines what should be done, IMO.

I don't know how much demand for this feature there is, but I think we should either move this forward for the next+1 (3.9.0) release or close it for now.

While I agree with @pljones that having such a management interface would be really nice, I doubt that it will be as simple as this PR, so it might make take some more time.

What do people think? Is there demand? (Maybe show via a thumbs-up reaction on this comment)

If there is demand and you (@dingodoppelt) don't plan to move this any further, then someone else could possibly step in and try to finish it?

ann0see · 2021-09-05T15:17:27Z

I think this could go to a feature branch and be discussed later. A management interface etc. could be created - based on #1975

A stereo mix server is a local TCP server that outputs the mixed sound as s16le stereo @ 48000 hz. The output can be streamed to ffmpeg for sending to (e.g.) an Icecast server: nc localhost $STEREO_MIX_PORT \ | ffmpeg -f s16le -ar 48000 -ac 2 -i - \ -acodec libmp3lame -ab 128k -f mp3 $ICECAST_URL The implementation for `CServer::MixStream` comes from the Streamer2 PR: jamulussoftware#967 By making the stream system pull-based (consumers ask Jamulus for data), rather than push-based (Jamulus sends data to a streaming server), the implementation is greatly simplified. * No need to implement ways to toggle streaming on/off, as streaming starts when a client connects and ends when the client disconnects. * No need to deal with launching, monitoring, and killing sub-processes. * Works with all OSes. Co-authored-by: dingodoppelt <dexxter@top-email.net>

ann0see · 2022-01-02T17:54:55Z

@dingodoppelt I now pushed it to a feature branch: https://github.com/jamulussoftware/jamulus/tree/feature/ffmpeg-streamer2 therefore closing. I think @dtinth is working on something related to this.

ann0see · 2022-03-04T21:08:48Z

As soon as the management interface (JSON-RPC) is merged, the feature branch could be revived: https://github.com/jamulussoftware/jamulus/blob/feature/ffmpeg-streamer2/README.md

dingodoppelt added 7 commits February 7, 2021 13:09

proof of concept jamstreamer first implementation (thanks to hselasky)

19419ac

refactor, get rid of unneeded stuff. add comments

02cddea

integrate better into the jamulus server

f097f3d

fix for mono-in

f5f829a

get rid of float2short conversion and mix on an int16_t vector

a17ec71

make the streaming and stream destination a commandline option

884fd20

handle jamulus server stoped and started signals

1f34458

pljones reviewed Feb 7, 2021

View reviewed changes

hoffie requested changes Feb 7, 2021

View reviewed changes

fix build2

082dca5

dingodoppelt force-pushed the streamer2 branch from 79a4505 to 082dca5 Compare February 8, 2021 00:35

dingodoppelt force-pushed the streamer2 branch from 2e9c9a5 to 082dca5 Compare February 8, 2021 13:59

fix build win32

909a6b0

npostavs reviewed Feb 9, 2021

View reviewed changes

Comment thread src/streamer/jamstreamer.cpp Outdated

npostavs reviewed Feb 9, 2021

View reviewed changes

Comment thread src/streamer/jamstreamer.cpp Outdated

refactor, get rid of redundant code (thanks to npostavs)

cc48ad5

npostavs mentioned this pull request Feb 10, 2021

Make CJamStreamer::process data parameter a reference dingodoppelt/jamulus#2

Merged

Make CJamStreamer::process signal & slot pass vector by const ref

2fef426

To avoid redundant copying.

dingodoppelt and others added 5 commits February 11, 2021 01:36

Merge pull request #2 from npostavs/streamer2.np.changes

7f5a61d

Make CJamStreamer::process data parameter a reference

Use qproc

8829d1a

make qproc into a pointer

e3001c5

add some error checking

2374af5

adjust logging

e8e2692

hoffie reviewed Feb 12, 2021

View reviewed changes

Comment thread src/main.cpp

hoffie and others added 4 commits February 13, 2021 00:28

streamer: Add logging of ffmpeg output

7b38912

Signed-off-by: Christian Hoffmann <mail@hoffmann-christian.info>

add help text

b8a8297

Merge pull request #3 from hoffie/fix-streamer2

c28e784

Fix streamer2

ann0see mentioned this pull request Mar 19, 2021

Individual output channel mapping #1305

Closed

ann0see mentioned this pull request Jul 24, 2021

Enable mixing with external DAW #1925

Closed

ann0see closed this Jan 2, 2022

Conversation

dingodoppelt commented Feb 7, 2021

Uh oh!

pljones left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoffie Feb 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dingodoppelt Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dingodoppelt commented Feb 8, 2021

Uh oh!

Uh oh!

Uh oh!

pljones commented Feb 10, 2021

Uh oh!

Uh oh!

dingodoppelt commented Feb 22, 2021

Uh oh!

hoffie commented Feb 22, 2021

Uh oh!

pljones commented Feb 22, 2021

Uh oh!

hoffie commented Feb 24, 2021

Uh oh!

dingodoppelt commented Feb 25, 2021

Uh oh!

pljones commented Mar 12, 2021

Uh oh!

hoffie commented Mar 18, 2021

Uh oh!

dingodoppelt commented Mar 20, 2021

Uh oh!

hoffie commented Mar 20, 2021

Uh oh!

pljones commented Mar 21, 2021

Uh oh!

olibac commented Mar 25, 2021

Uh oh!

pljones commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olibac commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pljones commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olibac commented Mar 25, 2021

Uh oh!

WolfganP commented Mar 25, 2021

Uh oh!

hoffie commented Apr 29, 2021

Uh oh!

dingodoppelt commented Apr 29, 2021

Uh oh!

hoffie commented May 10, 2021

Uh oh!

ann0see commented Sep 5, 2021

pljones left a comment •

edited

Loading

hoffie Feb 7, 2021 •

edited

Loading

dingodoppelt Feb 9, 2021 •

edited

Loading

pljones commented Mar 25, 2021 •

edited

Loading

olibac commented Mar 25, 2021 •

edited

Loading

pljones commented Mar 25, 2021 •

edited

Loading