[jitsi-dev] [jitsi/jitsi-videobridge] Do not use WeakReference on Endpoint and Conference (#287)


#1

Both Conference and Endpoint have "expired" properties which can be used to determine if instances are valid, so using WeakReference only adds extra complexity.

I need these changes for another work which will require to store expired Endpoint instances until Conference is not expired and it may break logic which depends on WeakReferences<Endpoint>.
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/jitsi-videobridge/pull/287

-- Commit Summary --

  * No longer use WeakReference on Endpoint
  * No longer use WeakReference on Conference

-- File Changes --

    M src/main/java/org/jitsi/videobridge/Conference.java (26)
    M src/main/java/org/jitsi/videobridge/ConferenceSpeechActivity.java (64)
    M src/main/java/org/jitsi/videobridge/Endpoint.java (95)
    M src/main/java/org/jitsi/videobridge/eventadmin/influxdb/LoggingHandler.java (8)

-- Patch Links --

https://github.com/jitsi/jitsi-videobridge/pull/287.patch
https://github.com/jitsi/jitsi-videobridge/pull/287.diff

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287


#2

The comment at line Conference.java:557 is no longer valid.

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287#issuecomment-240152346


#3

I'm wondering why did we even have weakreferences to endpoints in the first place. There must have been a reason, that's lost by now, and I'm worried that we're not taking that into account.

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287#issuecomment-240153020


#4

I guess we just weren't using the Endpoint.expired field as we should. The functional changes look good to me, I'll merge this after the very few editorial fixes.

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287#issuecomment-240154642


#5

So @lyubomir said that at the time when this code was written there was no "expired" property on Endpoint

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287#issuecomment-240155076


#6

@@ -874,7 +876,7 @@ else if (conferenceEndpoints.contains(endpoint))
                 if ((dominantEndpointIndex != -1)
                         && (dominantEndpointIndex != 0))
                 {
- WeakReference<Endpoint> weakReference
+ Endpoint weakReference

Maybe rename this to something else.

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287/files/29a5dc77e0696edf853a0860a725904dddf036e9#r74968561


#7

@@ -874,7 +876,7 @@ else if (conferenceEndpoints.contains(endpoint))
                 if ((dominantEndpointIndex != -1)
                         && (dominantEndpointIndex != 0))
                 {
- WeakReference<Endpoint> weakReference
+ Endpoint weakReference

Oh yes, sure I have missed that one

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287/files/29a5dc77e0696edf853a0860a725904dddf036e9#r74968678


#8

So @lyubomir said that at the time when this code was written there was no "expired" property on Endpoint

got it, thanks @paweldomas :+1:

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287#issuecomment-240155860


#9

@@ -276,7 +278,7 @@ private void activeSpeakerChanged(long ssrc)
     {
         Conference conference = getConference();

- if ((conference != null) && !conference.isExpired())
+ if (conference != null)

Do we really not want the isExpired check here?

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287/files/29a5dc77e0696edf853a0860a725904dddf036e9#r74969875


#10

@@ -276,7 +278,7 @@ private void activeSpeakerChanged(long ssrc)
     {
         Conference conference = getConference();

- if ((conference != null) && !conference.isExpired())
+ if (conference != null)

I'm sorry, please disregard this, I just noticed that you're taking care of that in the `getConference` method.

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287/files/29a5dc77e0696edf853a0860a725904dddf036e9#r74970204


#11

@@ -276,7 +278,7 @@ private void activeSpeakerChanged(long ssrc)
     {
         Conference conference = getConference();

- if ((conference != null) && !conference.isExpired())
+ if (conference != null)

getConference() returning null equals conference has expired, but if you think it's not readable I will revert that.

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287/files/29a5dc77e0696edf853a0860a725904dddf036e9#r74970269


#12

@@ -276,7 +278,7 @@ private void activeSpeakerChanged(long ssrc)
     {
         Conference conference = getConference();

- if ((conference != null) && !conference.isExpired())
+ if (conference != null)

Ok

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287/files/29a5dc77e0696edf853a0860a725904dddf036e9#r74970334


#13

@paweldomas pushed 2 commits.

a51edb6 doc(Conference): do not mention WeakReference
61aa116 refactor(ConferenceSpeechActivity): simplify indexing logic

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287/files/29a5dc77e0696edf853a0860a725904dddf036e9..61aa116a24a9ec97fd1f6e3395e0ce21964fa36f


#14

@@ -551,8 +555,8 @@ public Endpoint getDominantEndpoint()
             }

Here we can now remove the `if (this.dominantEndpoint == null)` clause.

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287/files/61aa116a24a9ec97fd1f6e3395e0ce21964fa36f#r75015010


#15

@@ -551,8 +555,8 @@ public Endpoint getDominantEndpoint()
             }

Are you sure ? It can be null. Is it ok to remove and add null ?

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287/files/61aa116a24a9ec97fd1f6e3395e0ce21964fa36f#r75015705


#16

@@ -551,8 +555,8 @@ public Endpoint getDominantEndpoint()
             }

Sorry about that, I misread the code. We need that there!

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287/files/61aa116a24a9ec97fd1f6e3395e0ce21964fa36f#r75017242


#17

Merged #287.

···

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/287#event-757660405