bridge_softmix: use a float type to store the internal REMB bitrate

REMB's exponent is 6-bits (0..63) and has a mantissa of 18-bits. We were using
an unsigned integer to represent the bitrate. However, that type is not large
enough to hold all potential bitrate values. If the bitrate is large enough
bits were being shifted off the "front" of the mantissa, which caused the
wrong value to be sent to the browser.

This patch makes it so it now uses a float type to hold the bitrate. Using a
float allows for all bitrate values to be correctly represented.

ASTERISK-28255

Change-Id: Ice00fdd16693b16b41230664be5d9f0e465b239e

diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c
index 290ea2b29221a831490c07de133f725565ebf805..f9f852064a046e3cfef353e738fe0a5c49ffca7c 100644 (file)
--- a/bridges/bridge_softmix.c
+++ b/bridges/bridge_softmix.c
@@ -33,6 +33,8 @@
 
 #include "asterisk.h"
 
+#include <math.h>
+
 #include "asterisk/stream.h"
 #include "asterisk/test.h"
 #include "asterisk/vector.h"
@@ -75,8 +77,8 @@ struct softmix_remb_collector {
        struct ast_frame frame;
        /*! The REMB to send to the source which is collecting REMB reports */
        struct ast_rtp_rtcp_feedback feedback;
-       /*! The maximum bitrate */
-       unsigned int bitrate;
+       /*! The maximum bitrate (A single precision floating point is big enough) */
+       float bitrate;
 };
 
 struct softmix_stats {
@@ -1334,7 +1336,7 @@ static void remb_collect_report(struct ast_bridge *bridge, struct ast_bridge_cha
        struct softmix_bridge_data *softmix_data, struct softmix_channel *sc)
 {
        int i;
-       unsigned int bitrate;
+       float bitrate;
 
        /* If there are no video sources that we are a receiver of then we have noone to
         * report REMB to.
@@ -1391,6 +1393,7 @@ static void remb_collect_report(struct ast_bridge *bridge, struct ast_bridge_cha
 static void remb_send_report(struct ast_bridge_channel *bridge_channel, struct softmix_channel *sc)
 {
        int i;
+       int exp;
 
        if (!sc->remb_collector) {
                return;
@@ -1398,17 +1401,52 @@ static void remb_send_report(struct ast_bridge_channel *bridge_channel, struct s
 
        /* We always do this calculation as even when the bitrate is zero the browser
         * still prefers it to be accurate instead of lying.
+        *
+        * The mantissa only has 18 bits available, so make sure it fits. Adjust the
+        * value and exponent for those values that don't.
+        *
+        * For example given the following:
+        *
+        * bitrate = 123456789.0
+        * frexp(bitrate, &exp);
+        *
+        * 'exp' should now equal 27 (number of bits needed to represent the value). Since
+        * the mantissa must fit into an 18-bit unsigned integer, and the given bitrate is
+        * too large to fit, we must subtract 18 from the exponent in order to get the
+        * number of times the bitrate will fit into that size integer.
+        *
+        * exp -= 18;
+        *
+        * 'exp' is now equal to 9. Now we can get the mantissa that fits into an 18-bit
+        * unsigned integer by dividing the bitrate by 2^exp:
+        *
+        * mantissa = 123456789.0 / 2^9
+        *
+        * This makes the final mantissa equal to 241126 (implicitly cast), which is less
+        * than 262143 (the max value that can be put into an unsigned 18-bit integer).
+        * So now we have the following:
+        *
+        * exp = 9;
+        * mantissa = 241126;
+        *
+        * If we multiply that back we should come up with something close to the original
+        * bit rate:
+        *
+        * 241126 * 2^9 = 123456512
+        *
+        * Precision is lost due to the nature of floating point values. Easier to why from
+        * the binary:
+        *
+        * 241126 * 2^9 = 241126 << 9 = 111010110111100110 << 9 = 111010110111100110000000000
+        *
+        * Precision on the "lower" end is lost due to zeros being shifted in. This loss is
+        * both expected and acceptable.
         */
-       sc->remb_collector->feedback.remb.br_mantissa = sc->remb_collector->bitrate;
-       sc->remb_collector->feedback.remb.br_exp = 0;
+       frexp(sc->remb_collector->bitrate, &exp);
+       exp = exp > 18 ? exp - 18 : 0;
 
-       /* The mantissa only has 18 bits available, so while it exceeds them we bump
-        * up the exp.
-        */
-       while (sc->remb_collector->feedback.remb.br_mantissa > 0x3ffff) {
-               sc->remb_collector->feedback.remb.br_mantissa = sc->remb_collector->feedback.remb.br_mantissa >> 1;
-               sc->remb_collector->feedback.remb.br_exp++;
-       }
+       sc->remb_collector->feedback.remb.br_mantissa = sc->remb_collector->bitrate / (1 << exp);
+       sc->remb_collector->feedback.remb.br_exp = exp;
 
        for (i = 0; i < AST_VECTOR_SIZE(&bridge_channel->stream_map.to_bridge); ++i) {
                int bridge_num = AST_VECTOR_GET(&bridge_channel->stream_map.to_bridge, i);

diff --git a/res/res_remb_modifier.c b/res/res_remb_modifier.c
index 1e79b83d1cce7c0c06521d4fa1e0ecf003e434e5..a4a83bce22f3c0a066f224ba6a64e848d151d617 100644 (file)
--- a/res/res_remb_modifier.c
+++ b/res/res_remb_modifier.c
@@ -31,6 +31,8 @@
 
 #include "asterisk.h"
 
+#include <math.h>
+
 #include "asterisk/module.h"
 #include "asterisk/cli.h"
 #include "asterisk/channel.h"
@@ -39,9 +41,9 @@
 
 struct remb_values {
        /*! \brief The amount of bitrate to use for REMB received from the channel */
-       unsigned int receive_bitrate;
+       float receive_bitrate;
        /*! \brief The amount of bitrate to use for REMB sent to the channel */
-       unsigned int send_bitrate;
+       float send_bitrate;
 };
 
 static void remb_values_free(void *data)
@@ -59,6 +61,8 @@ static struct ast_frame *remb_hook_event_cb(struct ast_channel *chan, struct ast
        struct ast_rtp_rtcp_feedback *feedback;
        struct ast_datastore *remb_store;
        struct remb_values *remb_values;
+       int exp;
+       float bitrate;
 
        if (!frame) {
                return NULL;
@@ -91,20 +95,57 @@ static struct ast_frame *remb_hook_event_cb(struct ast_channel *chan, struct ast
 
        /* If a bitrate override has been set apply it to the REMB Frame */
        if (event == AST_FRAMEHOOK_EVENT_READ && remb_values->receive_bitrate) {
-               feedback->remb.br_mantissa = remb_values->receive_bitrate;
-               feedback->remb.br_exp = 0;
+               bitrate = remb_values->receive_bitrate;
        } else if (event == AST_FRAMEHOOK_EVENT_WRITE && remb_values->send_bitrate) {
-               feedback->remb.br_mantissa = remb_values->send_bitrate;
-               feedback->remb.br_exp = 0;
+               bitrate = remb_values->send_bitrate;
        }
 
-       /* The mantissa only has 18 bits available, so while it exceeds them we bump
-        * up the exp.
+       /*
+        * The mantissa only has 18 bits available, so make sure it fits. Adjust the
+        * value and exponent for those values that don't.
+        *
+        * For example given the following:
+        *
+        * bitrate = 123456789.0
+        * frexp(bitrate, &exp);
+        *
+        * 'exp' should now equal 27 (number of bits needed to represent the value). Since
+        * the mantissa must fit into an 18-bit unsigned integer, and the given bitrate is
+        * too large to fit, we must subtract 18 from the exponent in order to get the
+        * number of times the bitrate will fit into that size integer.
+        *
+        * exp -= 18;
+        *
+        * 'exp' is now equal to 9. Now we can get the mantissa that fits into an 18-bit
+        * unsigned integer by dividing the bitrate by 2^exp:
+        *
+        * mantissa = 123456789.0 / 2^9
+        *
+        * This makes the final mantissa equal to 241126 (implicitly cast), which is less
+        * than 262143 (the max value that can be put into an unsigned 18-bit integer).
+        * So now we have the following:
+        *
+        * exp = 9;
+        * mantissa = 241126;
+        *
+        * If we multiply that back we should come up with something close to the original
+        * bit rate:
+        *
+        * 241126 * 2^9 = 123456512
+        *
+        * Precision is lost due to the nature of floating point values. Easier to why from
+        * the binary:
+        *
+        * 241126 * 2^9 = 241126 << 9 = 111010110111100110 << 9 = 111010110111100110000000000
+        *
+        * Precision on the "lower" end is lost due to zeros being shifted in. This loss is
+        * both expected and acceptable.
         */
-       while (feedback->remb.br_mantissa > 0x3ffff) {
-               feedback->remb.br_mantissa = feedback->remb.br_mantissa >> 1;
-               feedback->remb.br_exp++;
-       }
+       frexp(bitrate, &exp);
+       exp = exp > 18 ? exp - 18 : 0;
+
+       feedback->remb.br_mantissa = bitrate / (1 << exp);
+       feedback->remb.br_exp = exp;
 
        return frame;
 }