[jitsi-dev] [libjitsi] Adds another method for getting the current sending bitrate for a Med… (#95)


#1

…iaStream.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Adds another method for getting the current sending bitrate for a MediaStream.

-- File Changes --

    M src/org/jitsi/impl/neomedia/MediaStreamStatsImpl.java (47)
    M src/org/jitsi/impl/neomedia/RTPConnectorOutputStream.java (50)
    M src/org/jitsi/impl/neomedia/rtp/remotebitrateestimator/RateStatistics.java (12)
    M src/org/jitsi/service/neomedia/MediaStreamStats.java (10)

-- Patch Links --

https://github.com/jitsi/libjitsi/pull/95.patch
https://github.com/jitsi/libjitsi/pull/95.diff

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/95


#2

Merged #95.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/95#event-581869642


#3

@@ -1548,4 +1548,51 @@ public void addRembListener(REMBListener listener)
             }
         }
     }
+
+ /**
+ * {@inheritDoc}
+ *
+ * This method is different from {@link #getUploadRateKiloBitPerSec()} in

I'll be commenting on the code inline. I consider these thinking-out-loud pieces of thoughts, not issues to be addressed.

I find the method name getUploadRateKiloBitPerSec... unappealing. Is it correct to have bit in there instead of bits. On top of that, its javadoc talks about download.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/95/files#r55376044


#4

+ @Override
+ public long getSendingBitrate()
+ {
+ long sbr = -1;
+ AbstractRTPConnector rtpConnector = mediaStreamImpl.getRTPConnector();
+
+ if (rtpConnector != null)
+ {
+ try
+ {
+ RTPConnectorOutputStream rtpStream
+ = rtpConnector.getDataOutputStream(false);
+ RTPConnectorOutputStream rtcpStream
+ = rtpConnector.getControlOutputStream(false);
+
+ if (rtpStream != null && rtcpStream != null)

I didn't get it why both RTP and RTCP streams are a must. I guess it could be a requirement of RateStatistics but didn't really search for the cause of the requirement.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/95/files#r55377101


#5

@@ -123,6 +140,12 @@

         PACKET_QUEUE_CAPACITY
             = packetQueueCapacity >= 0 ? packetQueueCapacity : 256;
+
+ logger.debug("Initialized configuration. "

We usually do the logging of constructed strings of logger.isDebugEnabled(). I suppose it doesn't matter too much anyway.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/95/files#r55377378


#6

@@ -215,6 +238,12 @@ static boolean logPacket(long numOfPacket)
     private boolean closed = false;

     /**
+ * The instance used to calculate the sending bitrate of this output stream.
+ */
+ private RateStatistics rateStatistics
+ = new RateStatistics(AVERAGE_BITRATE_WINDOW_MS);

I usually go with final here. Don't know whether that makes any difference. I appear to remember reading that final is overused and fields are probably the most useful places to use it... or something like that. I may be wrong.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/95/files#r55377999


#7

@@ -645,6 +675,7 @@ private boolean write(RawPacket[] pkts)
             {
                 if (success)
                 {
+ rateStatistics.update(pkt.getLength(), now);
                     if (!send(pkt))

I didn't get why rateStatistics.update() is before send(). I trust you know why you have it that way so I went and committed it.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/95/files#r55378310


#8

@@ -663,6 +694,23 @@ private boolean write(RawPacket[] pkts)
         return success;
     }

+ /**
+ * @return the current output bitrate in bits per second.
+ */
+ public long getOutputBitrate()

OutputBitrate of OutputStream... I think the Output on Bitrate is obsolete but I suppose getBitrate could be viewed as too sparse, vague, etc.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/95/files#r55379031


#9

@@ -663,6 +694,23 @@ private boolean write(RawPacket[] pkts)
         return success;
     }

+ /**
+ * @return the current output bitrate in bits per second.
+ */
+ public long getOutputBitrate()

I think it makes it clearer. It could be measuring the bitrate of the stuff that is written to it, before transformations, possible drops of packets, and before it's actually sent (I am moving the call to update() that you mentioned).

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/95/files#r55382231