added enable_recording api for audiobridge plugin.

[rajneeshksoni]

• Added support in audio bridge plugin to start/stop recording in the middle of call.
• This is using the similar structure as "enable_recording" of videoroom plugin.
• WAV file is created with start timestamp so that audiobridge output can be combined with mrj dumps from videoroom
• i think it still have issue, when the 'janus_audiobridge_mixer_thread' is not closed, we probably need to update the wav header during the audioroom destroy call.

[januscla]

Thanks for your contribution, @rajneeshksoni! Please make sure you sign our CLA, as it's a required step before we can merge this.

[lminiero]

Thanks! We don't use the recording to WAV, so I can't say I care much about the feature, but I understand how you and others may actually benefit from this.

I added a few notes inline, mostly editorial.

[lminiero]

@rajneeshksoni any update on my notes above? As a side note, please notice that there's apparently conflicts now that we've merged some new things in master. I suspect it may impact some code you moved around, since part of the changes are optional support for stereo mixing.

[rajneeshksoni]

@lminiero Sorry for the delay, i'll fix the conflict and update as per review comments.

[rajneeshksoni]

@lminiero can you check the fixes and provide your feedback.

[lminiero]

@rajneeshksoni I already have a review planned, but I'll need time to do that and go into the details, especially since there's recent merges to take into account. I'll get back to you as soon as I do that.

[lminiero]

Added a few minor notes inline. I'll try to make some tests later today 👍

[lminiero]

@rajneeshksoni I tested it and it seems to be working as expected 👍

I think there's something missing in the API, though, that is the filename you want to save to. At the moment, you can only specify a filename when creating the room, but that assumes your willingness to start recording something right away. With your new functionality, you may want to only start recording later, which means you didn't have the option of specifying where to save and with with filename base, but only an on/off trigger: as a result, all files were created in the folder I started Janus from. I think the enable_recording should be extended with a record_file property too (to only be used when starting a recording, of course).

[rajneeshksoni]

@lminiero regarding "record_file" parameter in enable_recording, i have change the recording file naming format. it also contain the time stamp, e.g. for filename 'audiobridge', file is currently saved as audiobridge-timestamp.
so i think we should just have parameter 'record_dir' where file will be saved as record_dir/audiobridge-timestamp.wav
and we dont need 'record_file'. pls confirm

[lminiero]

That's not how the AudioBrdge plugin works: record_dir is not in its API when you create the room (it's in the VideoRoom for different reasons), so I don't see why a different API should have it instead. Since the create API asks to specify a file, the new API should do it too: not providing a filename would fall back to your current automated generation (which is what we do when creating a room without a filename anyway).

[rajneeshksoni]

@lminiero Since i have added "-timestamp" as well in the name, so i am but confused how 'record_file' should handle appending timestamp in the filename. Appending timestamp in filename is important for later combining with video.
if user provide filename as /opt/recording/recfile.wav should i create recording file as - /opt/recording/recfile-'timestamp'.wav or /opt/recording/recfile.wav-'timestamp'.

[lminiero]

That's already what your code is trying to do, which means it would be broken if people passed record_file when creating a room:

	if(audiobridge->record_file) {
		g_snprintf(filename, 255, "%s-%"SCNi64"%s%s", audiobridge->record_file,
			now, rec_tempext ? "." : "", rec_tempext ? rec_tempext : "");
	} else {
		g_snprintf(filename, 255, "janus-audioroom-%s-%"SCNi64".wav%s%s", audiobridge->room_id_str,
			now, rec_tempext ? "." : "", rec_tempext ? rec_tempext : "");
	}

If record_file is provided, you just need to pass %s as the original code did and nothing else. It's the user who has control on filenames, and their responsibility to use different names when starting the recording multiple times.

[lminiero]

Thanks for the changes! I still see some problems with the user-provided filenames, though: I've added a few comments inline.

[lminiero]

Since there's record_file too, now, it should be validates as well.

[rajneeshksoni]

record_file is optional parameter, if user do not supply parameter we are using default name. Do we still need to add it for validation ?

[lminiero]

ALL parameters need to be validated, optional or not. You'll see that we have a lot of parameters defined without JANUS_JSON_PARAM_REQUIRED, for instance.

[lminiero]

This should just be "%s", not "%s-%"SCNi64: record_file contains the .wav extension already, which means you'd be appending the time after the extension, breaking it. There's no need to add the current time to the filename when it's provided by the end user: as explained in another comment, in that case it's the user's responsibility to provide unique filenames, or risk overwriting previous recordings otherwise.

[rajneeshksoni]

I wanted to record the timestamp in filename so that it can be used for audio video synchronisation, i fear if timestamp is added in with the API call, the delay between API call may not reflect the exact time.
Can we add ".wav" after the timestamp ?

[lminiero]

Again, that's not something YOU have to worry about. If people provide record_file, than they may put the timestamp in the filename yourself. If you don't provide it, then your pattern does the job.

Can we add ".wav" after the timestamp ?

No, it would be a breaking change (since yout changes impact how recordings are started after create too).

[lminiero]

Same here, no need for the start time. You can refer to how the code was before, to see how we handled that case ("%s%s%s").

[lminiero]

I've made some tests and it looks like the feature doesn't work if there's no one in the room: in that case, if I stop the recording and then start it again, when a new person joins it will keep on recording media to the old file, and will probably result in a broken file when we try to update the header (because the filename we were keeping track of will be different than before). I suspect that's because of how the mixer thread works, which does close to nothing when the room is empty: as such, when the recording is stopped and then started again, when it reaches your code it finds the recording status the same as before, and so thinks nothing has changed.

That said, I think this is something I can address myself. To speed things up, I'll address my notes above as well (missing parameter, recording file without timestamp), and maybe think of an alternative record_dir to add in the future. As such, I'll merge this as it is, and reference it in the upcoming commits. Thanks!

[rajneeshksoni]

@lminiero I'll analyze the above issue and create separate PR for it.
Thanks for merging.

[lminiero]

No need, I'm looking at it right now 👍
Hopefully I should have a fix soon.

[lminiero]

The above commit fixes it for me: it was enough to move the WAV init/update before the continue for empty rooms. This way, the recording is always updated no matter whether the mixer will do anything or not. This has the side effect of immediately creating an empty .wav for empty rooms, where before we'd only start creating a file at the first join, but I think that's not an issue (we do the same with .mjr files).

[rajneeshksoni]

working for me.