<p>Joshua Colp <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/11193">View Change</a></p><div style="white-space:pre-wrap">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
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">bridge_softmix: use a float type to store the internal REMB bitrate<br><br>REMB's exponent is 6-bits (0..63) and has a mantissa of 18-bits. We were using<br>an unsigned integer to represent the bitrate. However, that type is not large<br>enough to hold all potential bitrate values. If the bitrate is large enough<br>bits were being shifted off the "front" of the mantissa, which caused the<br>wrong value to be sent to the browser.<br><br>This patch makes it so it now uses a float type to hold the bitrate. Using a<br>float allows for all bitrate values to be correctly represented.<br><br>ASTERISK-28255<br><br>Change-Id: Ice00fdd16693b16b41230664be5d9f0e465b239e<br>---<br>M bridges/bridge_softmix.c<br>M res/res_remb_modifier.c<br>2 files changed, 103 insertions(+), 24 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c</span><br><span>index 290ea2b..f9f8520 100644</span><br><span>--- a/bridges/bridge_softmix.c</span><br><span>+++ b/bridges/bridge_softmix.c</span><br><span>@@ -33,6 +33,8 @@</span><br><span> </span><br><span> #include "asterisk.h"</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+#include <math.h></span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> #include "asterisk/stream.h"</span><br><span> #include "asterisk/test.h"</span><br><span> #include "asterisk/vector.h"</span><br><span>@@ -75,8 +77,8 @@</span><br><span> struct ast_frame frame;</span><br><span> /*! The REMB to send to the source which is collecting REMB reports */</span><br><span> struct ast_rtp_rtcp_feedback feedback;</span><br><span style="color: hsl(0, 100%, 40%);">- /*! The maximum bitrate */</span><br><span style="color: hsl(0, 100%, 40%);">- unsigned int bitrate;</span><br><span style="color: hsl(120, 100%, 40%);">+ /*! The maximum bitrate (A single precision floating point is big enough) */</span><br><span style="color: hsl(120, 100%, 40%);">+ float bitrate;</span><br><span> };</span><br><span> </span><br><span> struct softmix_stats {</span><br><span>@@ -1334,7 +1336,7 @@</span><br><span> struct softmix_bridge_data *softmix_data, struct softmix_channel *sc)</span><br><span> {</span><br><span> int i;</span><br><span style="color: hsl(0, 100%, 40%);">- unsigned int bitrate;</span><br><span style="color: hsl(120, 100%, 40%);">+ float bitrate;</span><br><span> </span><br><span> /* If there are no video sources that we are a receiver of then we have noone to</span><br><span> * report REMB to.</span><br><span>@@ -1391,6 +1393,7 @@</span><br><span> static void remb_send_report(struct ast_bridge_channel *bridge_channel, struct softmix_channel *sc)</span><br><span> {</span><br><span> int i;</span><br><span style="color: hsl(120, 100%, 40%);">+ int exp;</span><br><span> </span><br><span> if (!sc->remb_collector) {</span><br><span> return;</span><br><span>@@ -1398,17 +1401,52 @@</span><br><span> </span><br><span> /* We always do this calculation as even when the bitrate is zero the browser</span><br><span> * still prefers it to be accurate instead of lying.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * The mantissa only has 18 bits available, so make sure it fits. Adjust the</span><br><span style="color: hsl(120, 100%, 40%);">+ * value and exponent for those values that don't.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * For example given the following:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * bitrate = 123456789.0</span><br><span style="color: hsl(120, 100%, 40%);">+ * frexp(bitrate, &exp);</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * 'exp' should now equal 27 (number of bits needed to represent the value). Since</span><br><span style="color: hsl(120, 100%, 40%);">+ * the mantissa must fit into an 18-bit unsigned integer, and the given bitrate is</span><br><span style="color: hsl(120, 100%, 40%);">+ * too large to fit, we must subtract 18 from the exponent in order to get the</span><br><span style="color: hsl(120, 100%, 40%);">+ * number of times the bitrate will fit into that size integer.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * exp -= 18;</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * 'exp' is now equal to 9. Now we can get the mantissa that fits into an 18-bit</span><br><span style="color: hsl(120, 100%, 40%);">+ * unsigned integer by dividing the bitrate by 2^exp:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * mantissa = 123456789.0 / 2^9</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * This makes the final mantissa equal to 241126 (implicitly cast), which is less</span><br><span style="color: hsl(120, 100%, 40%);">+ * than 262143 (the max value that can be put into an unsigned 18-bit integer).</span><br><span style="color: hsl(120, 100%, 40%);">+ * So now we have the following:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * exp = 9;</span><br><span style="color: hsl(120, 100%, 40%);">+ * mantissa = 241126;</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * If we multiply that back we should come up with something close to the original</span><br><span style="color: hsl(120, 100%, 40%);">+ * bit rate:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * 241126 * 2^9 = 123456512</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Precision is lost due to the nature of floating point values. Easier to why from</span><br><span style="color: hsl(120, 100%, 40%);">+ * the binary:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * 241126 * 2^9 = 241126 << 9 = 111010110111100110 << 9 = 111010110111100110000000000</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Precision on the "lower" end is lost due to zeros being shifted in. This loss is</span><br><span style="color: hsl(120, 100%, 40%);">+ * both expected and acceptable.</span><br><span> */</span><br><span style="color: hsl(0, 100%, 40%);">- sc->remb_collector->feedback.remb.br_mantissa = sc->remb_collector->bitrate;</span><br><span style="color: hsl(0, 100%, 40%);">- sc->remb_collector->feedback.remb.br_exp = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ frexp(sc->remb_collector->bitrate, &exp);</span><br><span style="color: hsl(120, 100%, 40%);">+ exp = exp > 18 ? exp - 18 : 0;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* The mantissa only has 18 bits available, so while it exceeds them we bump</span><br><span style="color: hsl(0, 100%, 40%);">- * up the exp.</span><br><span style="color: hsl(0, 100%, 40%);">- */</span><br><span style="color: hsl(0, 100%, 40%);">- while (sc->remb_collector->feedback.remb.br_mantissa > 0x3ffff) {</span><br><span style="color: hsl(0, 100%, 40%);">- sc->remb_collector->feedback.remb.br_mantissa = sc->remb_collector->feedback.remb.br_mantissa >> 1;</span><br><span style="color: hsl(0, 100%, 40%);">- sc->remb_collector->feedback.remb.br_exp++;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(120, 100%, 40%);">+ sc->remb_collector->feedback.remb.br_mantissa = sc->remb_collector->bitrate / (1 << exp);</span><br><span style="color: hsl(120, 100%, 40%);">+ sc->remb_collector->feedback.remb.br_exp = exp;</span><br><span> </span><br><span> for (i = 0; i < AST_VECTOR_SIZE(&bridge_channel->stream_map.to_bridge); ++i) {</span><br><span> int bridge_num = AST_VECTOR_GET(&bridge_channel->stream_map.to_bridge, i);</span><br><span>diff --git a/res/res_remb_modifier.c b/res/res_remb_modifier.c</span><br><span>index 1e79b83..a4a83bc 100644</span><br><span>--- a/res/res_remb_modifier.c</span><br><span>+++ b/res/res_remb_modifier.c</span><br><span>@@ -31,6 +31,8 @@</span><br><span> </span><br><span> #include "asterisk.h"</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+#include <math.h></span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> #include "asterisk/module.h"</span><br><span> #include "asterisk/cli.h"</span><br><span> #include "asterisk/channel.h"</span><br><span>@@ -39,9 +41,9 @@</span><br><span> </span><br><span> struct remb_values {</span><br><span> /*! \brief The amount of bitrate to use for REMB received from the channel */</span><br><span style="color: hsl(0, 100%, 40%);">- unsigned int receive_bitrate;</span><br><span style="color: hsl(120, 100%, 40%);">+ float receive_bitrate;</span><br><span> /*! \brief The amount of bitrate to use for REMB sent to the channel */</span><br><span style="color: hsl(0, 100%, 40%);">- unsigned int send_bitrate;</span><br><span style="color: hsl(120, 100%, 40%);">+ float send_bitrate;</span><br><span> };</span><br><span> </span><br><span> static void remb_values_free(void *data)</span><br><span>@@ -59,6 +61,8 @@</span><br><span> struct ast_rtp_rtcp_feedback *feedback;</span><br><span> struct ast_datastore *remb_store;</span><br><span> struct remb_values *remb_values;</span><br><span style="color: hsl(120, 100%, 40%);">+ int exp;</span><br><span style="color: hsl(120, 100%, 40%);">+ float bitrate;</span><br><span> </span><br><span> if (!frame) {</span><br><span> return NULL;</span><br><span>@@ -91,20 +95,57 @@</span><br><span> </span><br><span> /* If a bitrate override has been set apply it to the REMB Frame */</span><br><span> if (event == AST_FRAMEHOOK_EVENT_READ && remb_values->receive_bitrate) {</span><br><span style="color: hsl(0, 100%, 40%);">- feedback->remb.br_mantissa = remb_values->receive_bitrate;</span><br><span style="color: hsl(0, 100%, 40%);">- feedback->remb.br_exp = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ bitrate = remb_values->receive_bitrate;</span><br><span> } else if (event == AST_FRAMEHOOK_EVENT_WRITE && remb_values->send_bitrate) {</span><br><span style="color: hsl(0, 100%, 40%);">- feedback->remb.br_mantissa = remb_values->send_bitrate;</span><br><span style="color: hsl(0, 100%, 40%);">- feedback->remb.br_exp = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ bitrate = remb_values->send_bitrate;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* The mantissa only has 18 bits available, so while it exceeds them we bump</span><br><span style="color: hsl(0, 100%, 40%);">- * up the exp.</span><br><span style="color: hsl(120, 100%, 40%);">+ /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * The mantissa only has 18 bits available, so make sure it fits. Adjust the</span><br><span style="color: hsl(120, 100%, 40%);">+ * value and exponent for those values that don't.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * For example given the following:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * bitrate = 123456789.0</span><br><span style="color: hsl(120, 100%, 40%);">+ * frexp(bitrate, &exp);</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * 'exp' should now equal 27 (number of bits needed to represent the value). Since</span><br><span style="color: hsl(120, 100%, 40%);">+ * the mantissa must fit into an 18-bit unsigned integer, and the given bitrate is</span><br><span style="color: hsl(120, 100%, 40%);">+ * too large to fit, we must subtract 18 from the exponent in order to get the</span><br><span style="color: hsl(120, 100%, 40%);">+ * number of times the bitrate will fit into that size integer.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * exp -= 18;</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * 'exp' is now equal to 9. Now we can get the mantissa that fits into an 18-bit</span><br><span style="color: hsl(120, 100%, 40%);">+ * unsigned integer by dividing the bitrate by 2^exp:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * mantissa = 123456789.0 / 2^9</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * This makes the final mantissa equal to 241126 (implicitly cast), which is less</span><br><span style="color: hsl(120, 100%, 40%);">+ * than 262143 (the max value that can be put into an unsigned 18-bit integer).</span><br><span style="color: hsl(120, 100%, 40%);">+ * So now we have the following:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * exp = 9;</span><br><span style="color: hsl(120, 100%, 40%);">+ * mantissa = 241126;</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * If we multiply that back we should come up with something close to the original</span><br><span style="color: hsl(120, 100%, 40%);">+ * bit rate:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * 241126 * 2^9 = 123456512</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Precision is lost due to the nature of floating point values. Easier to why from</span><br><span style="color: hsl(120, 100%, 40%);">+ * the binary:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * 241126 * 2^9 = 241126 << 9 = 111010110111100110 << 9 = 111010110111100110000000000</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Precision on the "lower" end is lost due to zeros being shifted in. This loss is</span><br><span style="color: hsl(120, 100%, 40%);">+ * both expected and acceptable.</span><br><span> */</span><br><span style="color: hsl(0, 100%, 40%);">- while (feedback->remb.br_mantissa > 0x3ffff) {</span><br><span style="color: hsl(0, 100%, 40%);">- feedback->remb.br_mantissa = feedback->remb.br_mantissa >> 1;</span><br><span style="color: hsl(0, 100%, 40%);">- feedback->remb.br_exp++;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(120, 100%, 40%);">+ frexp(bitrate, &exp);</span><br><span style="color: hsl(120, 100%, 40%);">+ exp = exp > 18 ? exp - 18 : 0;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ feedback->remb.br_mantissa = bitrate / (1 << exp);</span><br><span style="color: hsl(120, 100%, 40%);">+ feedback->remb.br_exp = exp;</span><br><span> </span><br><span> return frame;</span><br><span> }</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/11193">change 11193</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/11193"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: Ice00fdd16693b16b41230664be5d9f0e465b239e </div>
<div style="display:none"> Gerrit-Change-Number: 11193 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Christian Savinovich <csavinovich@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>