[jitsi-dev] [jitsi/libjitsi] Statistics improvements (#190)


#1

Refactors MediaStreamStats. Fixes the loss statistics and implements additional stream statistics.

Note that because the split into commits was done after the code was written the code may not even compile at each commit. The effort to avoid this is, IMO, not worth the benefits.
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/libjitsi/pull/190

-- Commit Summary --

  * refactor: Adds a new utility method to RawPacket.
  * refactor: Removes dead code and suppresses warnings.
  * feat: Adds extended interfaces for MediaStream statistics.
  * docs: Deprecates some of the MediaStreamStats methods.
  * refactor: Removes two methods from MediaStreamStats.
  * feat: Adds implementations for the new statistics interfaces.
  * refactor: Removes obsoletes classes.
  * refactor: Uses the implementation of MediaStreamStats2.
  * refactor: Adapts StatisticsEngine to recent changes.
  * feature: Updates the MediaStream#getMediaStreamStats return type.

-- File Changes --

    D src/org/jitsi/impl/neomedia/AbstractMediaStreamSSRCStats.java (108)
    M src/org/jitsi/impl/neomedia/MediaStreamImpl.java (13)
    D src/org/jitsi/impl/neomedia/MediaStreamReceivedSSRCStats.java (48)
    M src/org/jitsi/impl/neomedia/MediaStreamStatsImpl.java (126)
    M src/org/jitsi/impl/neomedia/RawPacket.java (10)
    M src/org/jitsi/impl/neomedia/recording/RecorderRtpImpl.java (2)
    M src/org/jitsi/impl/neomedia/recording/SynchronizerImpl.java (2)
    A src/org/jitsi/impl/neomedia/stats/AbstractBasicStreamStats.java (220)
    A src/org/jitsi/impl/neomedia/stats/BasicReceiveStreamStatsImpl.java (139)
    A src/org/jitsi/impl/neomedia/stats/BasicSendStreamStatsImpl.java (161)
    A src/org/jitsi/impl/neomedia/stats/MediaStreamStats2Impl.java (405)
    M src/org/jitsi/impl/neomedia/transform/TransformEngineWrapper.java (2)
    M src/org/jitsi/impl/neomedia/transform/rtcp/StatisticsEngine.java (196)
    M src/org/jitsi/service/neomedia/MediaStream.java (3)
    D src/org/jitsi/service/neomedia/MediaStreamSSRCStats.java (65)
    M src/org/jitsi/service/neomedia/MediaStreamStats.java (64)
    R src/org/jitsi/service/neomedia/stats/BasicReceiveStreamStats.java (32)
    A src/org/jitsi/service/neomedia/stats/BasicSendStreamStats.java (31)
    A src/org/jitsi/service/neomedia/stats/BasicStreamStats.java (80)
    A src/org/jitsi/service/neomedia/stats/MediaStreamStats2.java (71)

-- Patch Links --

https://github.com/jitsi/libjitsi/pull/190.patch
https://github.com/jitsi/libjitsi/pull/190.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/libjitsi/pull/190


#2

+ if (highestSeq == -1)
+ {
+ highestSeq = seq;
+ return;
+ }
+
+ // Now check for lost packets.
+ int diff = RTPUtils.sequenceNumberDiff(seq, highestSeq);
+ if (diff <= 0)
+ {
+ // RFC3550 says that all packets should be counted as received.
+ // However, we want to *not* count retransmitted packets as received,
+ // otherwise the calculated loss rate will be close to zero as long
+ // as all missing packets are requested and retransmitted.
+ // Here we differentiate between packets received out of order and
+ // those that were retransmitted.

I think it is worth mentioning that the proper solution would be to enable RTX and detect losses before the de-RTX-fication.

···


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/libjitsi/pull/190/files/06c3fa8c8e8ffc5e8a3e5b4dcadce2b3972c27a2..ac75e5886a70ced027a78c953e29add7e5a57f12#r74991224


#3

Hey @bgrozev, this is a significant amount of work and I'm trying to wrap my head around it. One thing that I find confusing is that many of the new classes have a corresponding removed class. For example:

AbstractMediaStreamSSRCStats-> AbstractBasicStreamStats
MediaStreamSSRCStats->BasicStreamStats
MediaStreamReceivedSSRCStats-> BasicReceiveStreamStatsImpl, BasicReceiveStreamStats
MediaStreamSentSSRCStats->BasicSendStreamStatsImpl, BasicSendStreamStats

Unless I’m missing something, is it impossible to keep the old class names?

···

--
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/libjitsi/pull/190#issuecomment-240210593


#4

Unless I’m missing something, is it impossible to keep the old class names?

Sure, but I prefer to keep the new names because:
1. The implementations are almost entirely new, so this wouldn't improve readability much
2. The API will still be incompatible with the old one
3. I think the new names (and package) are more appropriate

···

--
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/libjitsi/pull/190#issuecomment-240217368


#5

1. The implementations are indeed almost entirely new, but the functionality/semantics isn't. And new implementation shouldn't mean we need to change the name.

2. Is it? https://github.com/jitsi/jitsi-videobridge/pull/284/files makes me think of the opposite. For example, the changes in CallStatsConferenceStatsHandler.java can be completely removed if we keep the existing naming.

3. The appropriateness of the names is debatable; I actually find the new names confusing. "stream" is already an overloaded term. I would rather see something that incorporates the term "track", like SendTrackStats or ReceiveTrackStats because that's what we really care about, i.e. we don't care about statistics on a per SSRC level (RTX or FEC ssrcs are irrelevant for instance). What we care about is statistics at the track level, that, by chance, it can have an associated RTX/FEC ssrc.

···

--
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/libjitsi/pull/190#issuecomment-240222567


#6

The implementations are indeed almost entirely new, but the functionality/semantics isn't. And new implementation shouldn't mean we need to change the name.

My point it that changing the names back won't make your job as a reviewer easier.

Is it?

Yes, unless we also bring back the old inconsistent method names and move the accessor methods back to the MediaStreamStats interface (in which case we would have to move part of the implementation from MediaStreamStats2Impl to MediaStreamStatsImpl).

The appropriateness of the names is debatable

Agreed.

I would rather see something that incorporates the term "track", like SendTrackStats or ReceiveTrackStats because that's what we really care about

Sure, but this is not keeping the old class names.

we don't care about statistics on a per SSRC level

One more reason to not keep the old names :slight_smile:

What we care about is statistics at the track level, that, by chance, it can have an associated RTX/FEC ssrc.

We also use the same interfaces for the aggregated stats. But I like the "track" idea: TrackStats, SendTrackStats, ReceiveTrackStats -- gets rid of the word "basic" which is just superfluous. Do you want me to use these names instead?

···

--
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/libjitsi/pull/190#issuecomment-240228025


#7

Do you want me to use these names instead?

Yes, let's do that. I agree that makes more sense than both the old naming and the new naming.

···

--
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/libjitsi/pull/190#issuecomment-240232650


#8

And what do you think about just Stats, ReceiveStats, SendStats? I think I prefer this to using "track" (which has other meanings, including one in FMJ)

···

--
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/libjitsi/pull/190#issuecomment-240421109


#9

@bgrozev pushed 4 commits.

34890de docs: Adds a comment.
6aba3dc docs: Adds missing javadocs.
f9fefb5 refactor: Renames BasicStreamStats to TrackStats and more.
1b066f8 docs: Adds missing javadocs.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/190/files/f2b17ba075ea756781ef3e7d87e26a20f4af3504..1b066f8db51e9e79e3605ef276bf9b0ea4184ba6


#10

@bgrozev pushed 1 commit.

2072665 chore: Updates ice4j.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/libjitsi/pull/190/files/1b066f8db51e9e79e3605ef276bf9b0ea4184ba6..2072665008e1a382fe6137c849739309c2e21153


#11

jenkins it's ok to test

···

--
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/libjitsi/pull/190#issuecomment-240508794


#12

Merged #190.

···

--
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/libjitsi/pull/190#event-759015310