meetecho / janode

A Node.js adapter for the Janus WebRTC server
ISC License
98 stars 36 forks source link

add enable_recording request for audiobridge plugin? #9

Closed augustblack closed 2 years ago

augustblack commented 2 years ago

Hi would it be possible to add the enable_recording request for the audiobridge plugin?

Or would you accept a pull request with it?

atoppi commented 2 years ago

I've just pushed the commit https://github.com/meetecho/janode/commit/2b4a0f966d8de39916c6497f9ff8410605fd9631, please check that everything works as expected.

augustblack commented 2 years ago

Hi @atoppi thanks for that!

looks pretty good so far. Did you omit 'record_dir' on purpose? It would be good to have if we could have it in there.

atoppi commented 2 years ago

Did you omit 'record_dir' on purpose? It would be good to have if we could have it in there.

Yes I have omitted it for consistency with other APIs like create, where I only provide the recording filename as an absolute path. I don't like the current approach in some Janus plugins where you can set a path, a filename or a mix of them. I find it a bit confusing. Anyway I'm open to discuss it.

augustblack commented 2 years ago

I might be doing something wrong, but it doesn't seem like you can set an absolute path as the 'filename'

I send

janodeManagerHandle.enableRecording({
  room: <room>,
  secret: <secret>,
  record: true,
  filename: '/full/path/to/my.wav'
})

and get the response:

{ record: true, room: <room> }

but, I can't find the recording anywhere. Is there a default recording directory config setting I might be missing?

atoppi commented 2 years ago

It worked for me. Check Janus logs, maybe it's a matter of permissions.

lminiero commented 2 years ago

Janus will not create the folder if it's part of the path, only if the folder is provided separately. If that folder doesn't exist that's probably why the recording isn't created (even though I think you'll get an error message in the logs).

lminiero commented 2 years ago

IMHO rec_dir should be part of the API because of that.

atoppi commented 2 years ago

In that case I'd rather omit the recording_dir on the janode API and split the user supplied full path in file + dir when requesting the action to Janus.

augustblack commented 2 years ago

fwiw, the problem I was having was an error on my side. It works

atoppi commented 2 years ago

Ended up adding recording folder parameter to the api (commit 272e5424f9ffd53fb135fb0110835ff0afdbafa2) I've noticed that we already used a rec_dir param in the videoroom plugin, so I added it to audiobridge too.

Closing now.