EricssonResearch / scream

SCReAM - Mobile optimised congestion control algorithm
BSD 2-Clause "Simplified" License
174 stars 53 forks source link

Unexpected estimations increases under congestion #4

Closed ggarber closed 7 years ago

ggarber commented 7 years ago

I'm running some tests in a constrained network (uplink 200kbps) and the bandwidth estimation is usually correct (˜150 kbps) but from time to time there is a sudden increase to 500-700kpbs that creates a lot of congestion.

After debugging it I found that the offending code is the one below. In my case some times delta value is -2.X and in that case the target bitrate is increased X3 instead of decreasing like it is supposed to do according to the comment and my expectations.

                float delta = (kGainDown * offTarget * bytesNewlyAcked * mss / cwnd);
                cwnd += (int)(delta);

                /*
                 * Especially when running over low bitrate bottlenecks, it is
                 *  beneficial to reduce the target bitrate a little, it limits
                 *  possible RTP queue delay spikes when congestion window is reduced
                 */
                float rateTotal = 0.0f;
                for (int n = 0; n < nStreams; n++)
                    rateTotal += streams[n]->getMaxRate();
                if (rateTotal < 1.0e5f) {
                    delta = -delta / cwnd;
                    float rateAdjustFactor = (1.0f - delta);
                    for (int n = 0; n < nStreams; n++) {
                        Stream *tmp = streams[n];
                        tmp->targetBitrate = std::max(tmp->minBitrate,
                                                      std::min(tmp->maxBitrate,
                                                               tmp->targetBitrate*rateAdjustFactor));
                    }
                }

In my case these were the values of some relevant variables one of the times I could reproduce the issue:

OffTarget = -7.2
cwnd = -2920
Delta = -2.13

For the time being I will change that code to do nothing if rateAdjustFactor > 1 but I'm not sure of the implications.

This is a graph with encoded and sent bitrate, bandwidth estimation and rtt where you can see how the bwe suddenly increased for no reason in the middle of the session:

image

IngJohEricsson commented 7 years ago

Hi You are absolutely right, this is a bug. Thanks for finding it.

An extra line is needed after the line

                delta = -delta / cwnd;

namely

                delta = std::min(1.0f,delta);

Need to investigate however if 1.0 is the appropriate limit, will have a closer look at this. It should be sufficient with something like

                delta = std::min(0.25f,delta);

/Ingemar

/Ingemar

From: Gustavo Garcia [mailto:notifications@github.com] Sent: den 27 mars 2017 18:07 To: EricssonResearch/scream scream@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [EricssonResearch/scream] Unexpected estimations increases under congestion (#4)

I'm running some tests in a constrained network (uplink 200kbps) and the bandwidth estimation is usually correct (˜150 kbps) but from time to time there is a sudden increase to 500-700kpbs that creates a lot of congestion.

After debugging it I found that the offending code is the one below. In my case some times delta value is -2.X and in that case the target bitrate is increased X3 instead of decreasing like it is supposed to do according to the comment and my expectations.

            float delta = (kGainDown * offTarget * bytesNewlyAcked * mss / cwnd);

            cwnd += (int)(delta);

            /*

             * Especially when running over low bitrate bottlenecks, it is

             *  beneficial to reduce the target bitrate a little, it limits

             *  possible RTP queue delay spikes when congestion window is reduced

             */

            float rateTotal = 0.0f;

            for (int n = 0; n < nStreams; n++)

                rateTotal += streams[n]->getMaxRate();

            if (rateTotal < 1.0e5f) {

                delta = -delta / cwnd;

                float rateAdjustFactor = (1.0f - delta);

                for (int n = 0; n < nStreams; n++) {

                    Stream *tmp = streams[n];

                    tmp->targetBitrate = std::max(tmp->minBitrate,

                                                  std::min(tmp->maxBitrate,

                                                           tmp->targetBitrate*rateAdjustFactor));

                }

            }

In my case these were the values of some relevant variables one of the times I could reproduce the issue:

OffTarget = -7.2

cwnd = -2920

Delta = -2.13

For the time being I will change that code to do nothing if rateAdjustFactor > 1 but I'm not sure of the implications.

This is a graph with encoded and sent bitrate, bandwidth estimation and rtt where you can see how the bwe suddenly increased for no reason in the middle of the session:

[image]https://cloud.githubusercontent.com/assets/512252/24365463/45beb340-1316-11e7-9c55-4a51c0d4be51.png

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/EricssonResearch/scream/issues/4, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AKOdGBrAoG0MW1ScTTElyQHjxsNc-CjYks5rp96UgaJpZM4Mqe16.

IngJohEricsson commented 7 years ago

Hi Just updated ScreamTx.cpp with a solution to this problem. In the process I set a limit to how much the congestion window can be reduced in circumstances where data is queued up due to e.g. retransmission in lower protocol layers.

Many thanks for the bug report!

/Ingemar

From: Ingemar Johansson S Sent: den 27 mars 2017 20:17 To: 'EricssonResearch/scream' reply@reply.github.com; EricssonResearch/scream scream@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: RE: [EricssonResearch/scream] Unexpected estimations increases under congestion (#4)

Hi You are absolutely right, this is a bug. Thanks for finding it.

An extra line is needed after the line

                delta = -delta / cwnd;

namely

                delta = std::min(1.0f,delta);

Need to investigate however if 1.0 is the appropriate limit, will have a closer look at this. It should be sufficient with something like

                delta = std::min(0.25f,delta);

/Ingemar

/Ingemar

From: Gustavo Garcia [mailto:notifications@github.com] Sent: den 27 mars 2017 18:07 To: EricssonResearch/scream scream@noreply.github.com<mailto:scream@noreply.github.com> Cc: Subscribed subscribed@noreply.github.com<mailto:subscribed@noreply.github.com> Subject: [EricssonResearch/scream] Unexpected estimations increases under congestion (#4)

I'm running some tests in a constrained network (uplink 200kbps) and the bandwidth estimation is usually correct (˜150 kbps) but from time to time there is a sudden increase to 500-700kpbs that creates a lot of congestion.

After debugging it I found that the offending code is the one below. In my case some times delta value is -2.X and in that case the target bitrate is increased X3 instead of decreasing like it is supposed to do according to the comment and my expectations.

            float delta = (kGainDown * offTarget * bytesNewlyAcked * mss / cwnd);

            cwnd += (int)(delta);

            /*

             * Especially when running over low bitrate bottlenecks, it is

             *  beneficial to reduce the target bitrate a little, it limits

             *  possible RTP queue delay spikes when congestion window is reduced

             */

            float rateTotal = 0.0f;

            for (int n = 0; n < nStreams; n++)

                rateTotal += streams[n]->getMaxRate();

            if (rateTotal < 1.0e5f) {

                delta = -delta / cwnd;

                float rateAdjustFactor = (1.0f - delta);

                for (int n = 0; n < nStreams; n++) {

                    Stream *tmp = streams[n];

                    tmp->targetBitrate = std::max(tmp->minBitrate,

                                                  std::min(tmp->maxBitrate,

                                                           tmp->targetBitrate*rateAdjustFactor));

                }

            }

In my case these were the values of some relevant variables one of the times I could reproduce the issue:

OffTarget = -7.2

cwnd = -2920

Delta = -2.13

For the time being I will change that code to do nothing if rateAdjustFactor > 1 but I'm not sure of the implications.

This is a graph with encoded and sent bitrate, bandwidth estimation and rtt where you can see how the bwe suddenly increased for no reason in the middle of the session:

[image]https://cloud.githubusercontent.com/assets/512252/24365463/45beb340-1316-11e7-9c55-4a51c0d4be51.png

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/EricssonResearch/scream/issues/4, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AKOdGBrAoG0MW1ScTTElyQHjxsNc-CjYks5rp96UgaJpZM4Mqe16.

ggarber commented 7 years ago

That's great! Thank you very much for your fast response Ingemar.