Unify hidden domain usage for recording and transcription #1737
Conversation
…single usage anymore
…iptions are enabled
|
Any progress here? Looking forward for this to be merged |
| @@ -170,6 +172,11 @@ org.jitsi.jigasi.transcription.SAVE_TXT=true | |||
| org.jitsi.jigasi.transcription.SEND_TXT={{ .Env.JIGASI_TRANSCRIBER_SEND_TXT | default "false"}} | |||
| org.jitsi.jigasi.transcription.RECORD_AUDIO={{ .Env.JIGASI_TRANSCRIBER_RECORD_AUDIO | default "false"}} | |||
| org.jitsi.jigasi.transcription.RECORD_AUDIO_FORMAT=wav | |||
| # non-anonymous authentication is required for transcriber | |||
There was a problem hiding this comment.
These properties are already defined at line ~145: what's the difference between JIGASI_TRANSCRIBER_USER and JIGASI_XMPP_USER?
There was a problem hiding this comment.
I don't know. This may be a question to the architects of jigasi, whether a separate authentication for the transcriber is desired. I found preparations for a dedicated authentication in the code and just wanted to make the feature work.
Maybe it has to do with other jigasi features (SIP) that a separate authentication is desired?
There was a problem hiding this comment.
We don't need an extra user. JIgasi can be configured either as a trascriber or a SIP audio bridge, but not both at the same time. Thus, JIGASI_XMPP_USER (and the other ones) should be good.
| @@ -168,7 +168,7 @@ services: | |||
| - XMPP_DOMAIN | |||
| - XMPP_GUEST_DOMAIN | |||
| - XMPP_MUC_DOMAIN | |||
| - XMPP_RECORDER_DOMAIN | |||
| - XMPP_HIDDEN_PARTICIPANT_DOMAIN | |||
There was a problem hiding this comment.
Let's call this XMPP_HIDDEN_DOMAIN for the sake of brevity please.
| @@ -146,14 +146,19 @@ config.etherpad_base = '{{ .Env.ETHERPAD_PUBLIC_URL }}'; | |||
| config.etherpad_base = '{{ $PUBLIC_URL }}/etherpad/p/'; | |||
| {{ end -}} | |||
|
|
|||
| // Hidden domain usage | |||
| // | |||
| {{ if or $ENABLE_RECORDING $ENABLE_TRANSCRIPTIONS -}} | |||
There was a problem hiding this comment.
We should set this unconditionally, there is no harm in doing so.
| @@ -15,7 +15,7 @@ | |||
| {{ $XMPP_MUC_DOMAIN := .Env.XMPP_MUC_DOMAIN | default "muc.meet.jitsi" -}} | |||
| {{ $XMPP_MUC_DOMAIN_PREFIX := (split "." $XMPP_MUC_DOMAIN)._0 -}} | |||
| {{ $JIBRI_STRIP_DOMAIN_JID := .Env.JIBRI_STRIP_DOMAIN_JID | default $XMPP_MUC_DOMAIN_PREFIX -}} | |||
| {{ $XMPP_RECORDER_DOMAIN := .Env.XMPP_RECORDER_DOMAIN | default "recorder.meet.jitsi" -}} | |||
| {{ $XMPP_HIDDEN_PARTICIPANT_DOMAIN := .Env.XMPP_HIDDEN_PARTICIPANT_DOMAIN | default "hiddenpart.meet.jitsi" -}} | |||
There was a problem hiding this comment.
let's call the domain hidden.meet.jitsi please.
| @@ -170,6 +172,11 @@ org.jitsi.jigasi.transcription.SAVE_TXT=true | |||
| org.jitsi.jigasi.transcription.SEND_TXT={{ .Env.JIGASI_TRANSCRIBER_SEND_TXT | default "false"}} | |||
| org.jitsi.jigasi.transcription.RECORD_AUDIO={{ .Env.JIGASI_TRANSCRIBER_RECORD_AUDIO | default "false"}} | |||
| org.jitsi.jigasi.transcription.RECORD_AUDIO_FORMAT=wav | |||
| # non-anonymous authentication is required for transcriber | |||
There was a problem hiding this comment.
We don't need an extra user. JIgasi can be configured either as a trascriber or a SIP audio bridge, but not both at the same time. Thus, JIGASI_XMPP_USER (and the other ones) should be good.
| @@ -291,8 +291,8 @@ VirtualHost "{{ $XMPP_AUTH_DOMAIN }}" | |||
| } | |||
| authentication = "internal_hashed" | |||
|
|
|||
| {{ if $ENABLE_RECORDING }} | |||
| VirtualHost "{{ $XMPP_RECORDER_DOMAIN }}" | |||
| {{ if or $ENABLE_RECORDING $ENABLE_TRANSCRIPTIONS }} | |||
There was a problem hiding this comment.
We could actually define this unconditionally, there is no harm in doing that.
|
I would like to test this PR on my development environment, however as it is now this patch doesn't apply cleanly on the master branch due to recent commits (notably 360361e). |
|
I will try to update the patch versus master in the near future. In the meantime, please check release 9220, it should apply cleanly against this one. |
|
I can confirm this is working nicely on top of stable-9220, i am using this with some small changes (unrelated to this PR, just to get Vosk transcription instead of the default Google Cloud service), thanks. |
More generic approach to proper transcriber inclusion into docker.
I made a new pull request, based on the info & feedback from here: #1663
The patch is bigger because I renamed XMPP_RECORDER_DOMAIN. The name would be confusing if it is not the sole purpose of that domain.
I'm still not 100% sure this is the right way to do it. Please let me know your comments.
Patch has been tested vs release 9220.