Only send audio to clients that have been identified. by softins · Pull Request #1589 · jamulussoftware/jamulus

softins · 2021-04-30T21:56:21Z

Fixes #1243.

Fixes jamulussoftware#1243.

softins · 2021-04-30T21:57:37Z

This is an alternative to #1582, implementing option 2 from #1243 instead.

At the moment, it has been successfully compiled, but not yet tested.

Will also add some comments before merging.

hoffie

Thanks! Code looks good.

This will probably prevent Jamulus clients before version 3.4.5 from working with newer servers completely as pointed out in #1243 (comment).
I'm not saying that this is a problem though as those clients already had a vastly degraded user experience (lack of faders or at least fader names), the version is old and there have been no explicit compatibility guarantees for these earlier versions, as far as I understand.

The PR seems to do what the title promises. ;)
#1243 (comment) Option 2 would have also included stopping to accept audio from non-identified clients. Should this also be done? Should it be done as part of this PR?

For me, the undetected eaves-dropping case sounds more important and should be covered by this PR.

Not approving yet as I haven't tested either. :)

hoffie · 2021-04-30T22:01:27Z

                                   const CVector<uint8_t>& vecbyNPacket,
                                   const int               iNPacketLen )
 {
+    if (bIsServer && !bIsIdentified)


Sadly, this is another two checks for each packet of each client, as far as I understand. I don't see a way to change this though and I can't judge what the performance impact is. I'd say that two comparisons should be cheap, but I lack knowledge about how much this is percentage-wise.

I would think compared with all the other per-channel operations (codec, mixing, etc.), it's infinitesimal.

hoffie

Hrm, I suspect that a bIsIdentified = false is missing somewhere (SetEnable()?) as Channel instances are re-used, IIRC?

hoffie · 2021-04-30T22:34:34Z

I have now tested this and it seems to work as expected:

No audio for clients which did not send channel infos
Proper audio for clients which behave correctly.

So, regarding correctness only the channel-reuse issue above remains.

softins · 2021-05-01T08:40:21Z

Hrm, I suspect that a bIsIdentified = false is missing somewhere (SetEnable()?) as Channel instances are re-used, IIRC?

This was one of the things I was planning to check today: the actual lifecycle of a channel instance.

softins · 2021-05-01T08:51:11Z

This will probably prevent Jamulus clients before version 3.4.5 from working with newer servers completely as pointed out in #1243 (comment).
I'm not saying that this is a problem though as those clients already had a vastly degraded user experience (lack of faders or at least fader names), the version is old and there have been no explicit compatibility guarantees for these earlier versions, as far as I understand.

It might even be an advantage, so long as we pointed out the minimum compatible client version in the release notes. But I'll look to see if we can also easily be compatible with what previous client versions did (send channel name instead of channel info?).

The PR seems to do what the title promises. ;)
#1243 (comment) Option 2 would have also included stopping to accept audio from non-identified clients. Should this also be done? Should it be done as part of this PR?

Obviously, it is the reception of audio that creates the channel, but it would be worthwhile excluding an unidentified channel from the mix. I'll look at that.

pljones · 2021-05-01T08:54:17Z

the actual lifecycle of a channel instance.

As far as I remember, they're a pool (fixed size on server start). Unused entries get reused lowest number first. You can connect, stop your packet stream, connect again, repeat, and use up channels using (currently none or) the same user details, if you're cycling faster than the server is timing you out.

pljones · 2021-05-01T08:56:21Z

I still see absolutely no benefit here. Anyone can put anything in. We're no living in a police state that requires people never, ever to even think about withholding information about themselves.

About the only "benefit" is to Jamulus Explorer.

softins · 2021-05-01T09:10:45Z

I still see absolutely no benefit here. Anyone can put anything in. We're no living in a police state that requires people never, ever to even think about withholding information about themselves.

This is really part of the conversation in #1243 itself, as to whether it is an issue that needs to be addressed. The present PR is one way to address it, if that is considered necessary. I'm not aware of any real-life situation that has arisen.

But people are still free not to set their profile information, and Jamulus will send empty info. I see no downside to wait until that has happened before starting to send audio.

About the only "benefit" is to Jamulus Explorer.

Obviously, I'm biassed :) But I think some people find the "fetch welcome message" feature in Explorer useful, and would be disappointed if it were removed again. And I think (many) other people would find the flashing up of a transient client, from Explorer doing so, annoying and distracting. I am in both sets, so would hope to change your mind!

I also see some value in the approach taken in this PR. In an ideal world, Jamulus would behave like SIP, where a connection is established between client and server, and the audio characteristics negotiated, before either end starts sending any audio at all.

pljones · 2021-05-01T09:15:02Z

I think the flashing client welcome fetch connect is yet another example of why we should have a server API... rather than a reason to do this.

softins · 2021-05-01T09:19:13Z

I think the flashing client welcome fetch connect is yet another example of why we should have a server API... rather than a reason to do this.

Maybe, but let's go for the easy wins first!

softins · 2021-05-01T09:36:57Z

This will probably prevent Jamulus clients before version 3.4.5 from working with newer servers completely as pointed out in #1243 (comment).
I'm not saying that this is a problem though as those clients already had a vastly degraded user experience (lack of faders or at least fader names), the version is old and there have been no explicit compatibility guarantees for these earlier versions, as far as I understand.

It might even be an advantage, so long as we pointed out the minimum compatible client version in the release notes. But I'll look to see if we can also easily be compatible with what previous client versions did (send channel name instead of channel info?).

I have now checked the history. This PR should in fact be compatible with all client versions from 3.3.0 onwards (24 Feb 2013), as that is the version which introduced Channel Infos.

softins · 2021-05-01T11:04:53Z

Hrm, I suspect that a bIsIdentified = false is missing somewhere (SetEnable()?) as Channel instances are re-used, IIRC?

Fixed in 908d356

pljones · 2021-05-01T12:39:18Z

OK, if it's as old as 3.3.0, I'll have to back down. It's worth updating the protocol to note that the ChanInfo is now (effectively) required. We should also note it in the release notes.

If we're moving in this direction, I'd actually like to be able to drop all or selected protocol messages received from a client whilst they're in particular states - i.e. unidentified, not having OK'd the licence. (And licence first, of course. No point them identifying if they don't agree.)

softins · 2021-05-01T13:27:08Z

OK, if it's as old as 3.3.0, I'll have to back down. It's worth updating the protocol to note that the ChanInfo is now (effectively) required. We should also note it in the release notes.

I'll add a comment in the code to this effect too.

If we're moving in this direction, I'd actually like to be able to drop all or selected protocol messages received from a client whilst they're in particular states - i.e. unidentified, not having OK'd the licence. (And licence first, of course. No point them identifying if they don't agree.)

This is worth exploring, but not before the upcoming release.

hoffie

Looks good to me, thanks! :)

Co-authored-by: Christian Hoffmann <christian@hoffie.info>

Only send audio to clients that have been identified.

81f2ea2

Fixes jamulussoftware#1243.

softins requested a review from hoffie April 30, 2021 21:57

hoffie reviewed Apr 30, 2021

View reviewed changes

hoffie requested changes Apr 30, 2021

View reviewed changes

hoffie added this to the Release 3.8.0 milestone Apr 30, 2021

Clear bIsIdentified when resetting channel info.

908d356

softins added 3 commits May 1, 2021 12:22

Added explanatory comments.

5d4ebc0

Coding style.

5284e9c

Wait until channel is identified before fading in.

d3d689c

Add comment specifying version compatibility.

a2be3ac

hoffie approved these changes May 1, 2021

View reviewed changes

Comment thread src/channel.cpp Outdated

hoffie requested a review from pljones May 1, 2021 14:55

Improve fade-in test

cffc2a5

Co-authored-by: Christian Hoffmann <christian@hoffie.info>

pljones reviewed May 1, 2021

View reviewed changes

Comment thread src/channel.h

pljones reviewed May 1, 2021

View reviewed changes

Comment thread src/channel.cpp

pljones reviewed May 1, 2021

View reviewed changes

Comment thread src/channel.cpp

hoffie mentioned this pull request May 1, 2021

Server: Sync channel list right after connect #1582

Closed

softins requested a review from pljones May 2, 2021 11:38

pljones approved these changes May 2, 2021

View reviewed changes

hoffie merged commit bc13361 into jamulussoftware:master May 2, 2021

Conversation

softins commented Apr 30, 2021

Uh oh!

softins commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoffie left a comment

Choose a reason for hiding this comment

Uh oh!

hoffie Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

softins May 1, 2021

Choose a reason for hiding this comment

Uh oh!

hoffie left a comment

Choose a reason for hiding this comment

Uh oh!

hoffie commented Apr 30, 2021

Uh oh!

softins commented May 1, 2021

Uh oh!

softins commented May 1, 2021

Uh oh!

pljones commented May 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pljones commented May 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

softins commented May 1, 2021

Uh oh!

pljones commented May 1, 2021

Uh oh!

softins commented May 1, 2021

Uh oh!

softins commented May 1, 2021

Uh oh!

softins commented May 1, 2021

Uh oh!

pljones commented May 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

softins commented May 1, 2021

Uh oh!

hoffie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

softins commented Apr 30, 2021 •

edited

Loading

pljones commented May 1, 2021 •

edited

Loading

pljones commented May 1, 2021 •

edited

Loading

pljones commented May 1, 2021 •

edited

Loading