[1.x] Audiobridge global mjr flag ignored for resume mechanic

[ettubrute47]

What version of Janus is this happening on?
1.3.2 a44ebb6

Have you tested a more recent version of Janus too?
No, exists in most recent version

Was this working before?
No

Is there a gdb or libasan trace of the issue?
Not that kind of bug

Additional context
If you set mjrs = true in room definition, or use enable_mjrs to turn on individual mjr recordings, it sets audiobridge->mjrs = true. However if you suspend an individual (with stop_record = true), and then resume them via resume api, it doesn't check this flag, it only resumes recording if you specifically tell it in that api call. Not really a bug since documentation is accurate, it makes clear resume record is false by default.

I think it should default to the global state of the room instead. I believe all that is involved is modifying resume check to look at audiobridge->mjrs too, e.g., if(audiobridge->mjrs || record && json_is_true(record)), and then update documentation to say default is to room mjrs state. This could've been purposeful decision, but in cursory look didn't find any discussions on this.

For my scenario, room management happens in a separate service, and suspend/resume happens per client. They are decoupled in sharing recording state. I can pass that info to clients to make sure they are in sync when using suspend/resume, but its cleaner if only one service needs to know about it.

[lminiero]

I believe all that is involved is modifying resume check to look at audiobridge->mjrs too, e.g., if(audiobridge->mjrs || record && json_is_true(record)), and then update documentation to say default is to room mjrs state

While I understand your point, this would be a breaking change, and could cause a behaviour people using the feature in the wild right now wouldn't expect. In general, when patching features for things that aren't really bugs, we try to avoid changing the behaviour.

My feeling is that this is better handled by explaining it clearly in the docs that resuming recording must be explicit for automatically recorded rooms. As an alternative, we could add a new flag for that, e.g., something like "mjrs_autoresume" that, if set to true for rooms with mjrs=true, does what you're asking here (default being false to ensure nothing changes for people that don't touch anything).

[ettubrute47]

Yea oops I'm being dumb definitely would have ended up with people all of a sudden getting recordings when they weren't expecting it if we did what I suggested...

Yea I'm following you, I agree with the alternative, if there is anything to do about it to begin with (I know extending API shouldn't be taken lightly either). I'd like to try to do a pull request implementing the mjrs_autoresume flag as a small exercise for me though, if that's cool, no pressure in taking it.

[lminiero]

No no, it was not a dumb suggestion at all! I'm just always extra careful with changes: for all I know, you may be the onlyone using automatic recordings and suspend/resume, for instance, but I can't give that for granted.

I'd definitely welcome a PR, so please do feel free to try! And don't hesitate to ask here in case you want feedback on how to do some things. Just as a quick list of things to ensure, since this would be a new property when creating rooms:

1. the property must be available both when creating rooms statically (config file) and dynamically (API)
2. for the API we have a mechanism that autovalidates properties, so make sure your new property is in the object that defines the parameters of the create request
3. parameters that can exist in the configuration file, must also be saved to configuration file when creating "permanent" rooms.

For all the above you can probably refer to how other existing properties are currently handled, so it's mostly a matter of taking care of it as a checklist.

[ettubrute47]

Appreciate the pointers! Its awesome how responsive you are still after all this time! Luckily the mjrs flag has been a great reference as a pattern to follow.

I have run into some implementation questions though, and while I've come up with default answers to these I welcome your input!

Consider you set mjrs = true for the room via create/enable_mjrs, etc., and then the participant sends a 'configure' request with "record": false. It stops recording that user, marks mjrs_active to false, and they can always resume via another 'configure' call, or even after a 'resume' call if they were suspended. However with the mjrs_autoresume, if the user 'configure' to stop recording, then suspends and resumes, they will be autoresumed if they don't explicitly set record: false in the resume call, which may not fit their intention ("I already said I didn't want this user to be recorded in the configure call!").

This seems just a consequence of the API supporting different ways of using it, and so a developer generally wouldn't find themselves both using mjrs_autoresume and changing recording status via 'configure' + suspend/resume. I could add an internal 'mjrs' flag per participant, which remembers them explicitly starting/stopping record on configure/resume, but that seems like I'm furthering complexity around the recording logic if I do. In theory if the workflow involves configuring individual participant recording status, then they have their desired recording state at the participant level and can just always send that for every configure/resume call. My default is to just lean on documentation being accurate and explicit around mjrs_autoresume and the endpoints.

Also some minor points. I can't think of a use case other than mine with mjrs_autoresume where it's a "set and forget" thing, I could add it to "enable_mjrs" endpoint since its all about configuring 'mjrs' for the room, or "edit" since it could be considered room "metadata" but right now I'm not doing either.