meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.25k stars 2.48k forks source link

[1.x][Audiobridge] Couple minor issues in "enable_mjrs" request handling #3171

Closed RSATom closed 1 year ago

RSATom commented 1 year ago

First issue is very minor: https://github.com/meetecho/janus-gateway/blob/961b5764b34b5f84c56825f8acf05890d4551aa4/src/plugins/janus_audiobridge.c#L3823-L3833 it looks like room_prev_mjrs_active can be removed safely and replaced directly with mjrs_active.

Second issue is almost in the same place: https://github.com/meetecho/janus-gateway/blob/961b5764b34b5f84c56825f8acf05890d4551aa4/src/plugins/janus_audiobridge.c#L3824-L3830 i.e. If incoming enable_mjrs request contains only mjrs_dir but not mjrs - dir will not be updated. And if next enable_mjrs will not contain msrs_dir but only mjrs = true, recording will start to wrong dir.

RSATom commented 1 year ago

And btw I didn't find enable_mjrs request in docs :thinking:

lminiero commented 1 year ago

And btw I didn't find enable_mjrs request in docs thinking

You should look harder, then, it is there :stuck_out_tongue_closed_eyes:

https://github.com/meetecho/janus-gateway/blob/master/src/plugins/janus_audiobridge.c#L269-L285

I'll have a look at the code with respect to your other notes. IIRC, at the time that variable was meant to detect whether there had been a change, but we may have changed the logic since the first PR.

RSATom commented 1 year ago

You should look harder, then, it is there :stuck_out_tongue_closed_eyes:

Oops, my fault, I think I was looking to videoroom docs :man_facepalming: Sorry.

lminiero commented 1 year ago

I'll have a look at the code with respect to your other notes. IIRC, at the time that variable was meant to detect whether there had been a change, but we may have changed the logic since the first PR.

Looks like this dates back to #2674, which we used as a basis for the enable_mjrs request: as such, it's likely the same problem happens when enabling recordings in general too (and I guess in the VideoRoom as well, which also has enable_recording as part of its API). I think the main problem is that those variables with prev in the name are improperly called, and can lead to confusion: about the dir change, not sure if we should always allow it (getting rid of that extra check) or if we should only allow it either if mjr recording was active before, or if we're activating it now (which would mean extending the extra check).

lminiero commented 1 year ago

Fixed enable_mjrs in both master and 0.x, since it just updates a string. No need for now to do the same for enable_recording, since in that case that actually results in a creation of folders.