[Asterisk-code-review] bridge_softmix: use a float type to store the internal REMB bitrate (...asterisk[master])
Joshua Colp
asteriskteam at digium.com
Thu Apr 4 08:52:48 CDT 2019
Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/c/asterisk/+/11194 )
Change subject: bridge_softmix: use a float type to store the internal REMB bitrate
......................................................................
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
---
M bridges/bridge_softmix.c
M res/res_remb_modifier.c
2 files changed, 103 insertions(+), 24 deletions(-)
Approvals:
George Joseph: Looks good to me, but someone else must approve
Benjamin Keith Ford: Looks good to me, but someone else must approve
Joshua Colp: Looks good to me, approved; Approved for Submit
diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c
index 290ea2b..f9f8520 100644
--- 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 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 @@
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_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 @@
/* 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 1e79b83..a4a83bc 100644
--- 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 @@
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 @@
/* 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;
}
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11194
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ice00fdd16693b16b41230664be5d9f0e465b239e
Gerrit-Change-Number: 11194
Gerrit-PatchSet: 4
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Christian Savinovich <csavinovich at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190404/e633f7be/attachment-0001.html>
More information about the asterisk-code-review
mailing list