[Spice-devel] [PATCH spice-server] red_worker: don't get bit_rate from main_channel_client, if it wasn't initialized

[Yonit Halperin]

Hi,
On 05/09/2013 07:44 AM, Hans de Goede wrote:
> Hi,
>
> This patch removes the updating of dcc->streams_max_bit_rate when
> the new bit_rate is larger then it. I believe this is unintentional,
> and thus should be fixed.
>
> If it is intentional, it should be documented in the commit msg.
It was intentional, because it doesn't really matter if we update the 
streams_max_bit_rate to the bit rate the main_channel holds, since we 
compare them each time we set the initial bit rate for a stream. I will 
document it. Can I push it (after improving the commit msg)?

Thanks,
Yonit.
>
> Regards,
>
> Hans
>
>
> On 05/08/2013 07:27 PM, Yonit Halperin wrote:
>> When setting an initial video stream bit rate, if the bit rate
>> wasn't calculated by main_channel_client, and we don't have
>> estimation from previos streams, use some default values.
>> ---
>>   server/red_worker.c | 42 ++++++++++++++++++++++++++++++------------
>>   1 file changed, 30 insertions(+), 12 deletions(-)
>>
>> diff --git a/server/red_worker.c b/server/red_worker.c
>> index f12d8f8..fb736b5 100644
>> --- a/server/red_worker.c
>> +++ b/server/red_worker.c
>> @@ -121,6 +121,8 @@
>>   /* the client's stream report frequency is the minimum of the 2
>> values below */
>>   #define RED_STREAM_CLIENT_REPORT_WINDOW 5 // #frames
>>   #define RED_STREAM_CLIENT_REPORT_TIMEOUT 1000 // milliseconds
>> +#define RED_STREAM_DEFAULT_HIGH_START_BIT_RATE (10 * 1024 * 1024) //
>> 10Mbps
>> +#define RED_STREAM_DEFAULT_LOW_START_BIT_RATE (2.5 * 1024 * 1024) //
>> 2.5Mbps
>>
>>   #define FPS_TEST_INTERVAL 1
>>   #define MAX_FPS 30
>> @@ -2929,11 +2931,9 @@ static inline Stream
>> *red_alloc_stream(RedWorker *worker)
>>   static uint64_t red_stream_get_initial_bit_rate(DisplayChannelClient
>> *dcc,
>>                                                   Stream *stream)
>>   {
>> -    MainChannelClient *mcc;
>>       char *env_bit_rate_str;
>>       uint64_t bit_rate = 0;
>>
>> -    mcc = red_client_get_main(dcc->common.base.client);
>>       env_bit_rate_str = getenv("SPICE_BIT_RATE");
>>       if (env_bit_rate_str != NULL) {
>>           double env_bit_rate;
>> @@ -2948,16 +2948,28 @@ static uint64_t
>> red_stream_get_initial_bit_rate(DisplayChannelClient *dcc,
>>       }
>>
>>       if (!bit_rate) {
>> -        bit_rate = main_channel_client_get_bitrate_per_sec(mcc);
>> -
>> -        if (bit_rate > dcc->streams_max_bit_rate) {
>> -            dcc->streams_max_bit_rate = bit_rate;
>> -        } else {
>> -            bit_rate = dcc->streams_max_bit_rate;
>> -        }
>> -    }
>> -
>> -    spice_debug("base-bit-rate %.2f (Mbps)", bit_rate / 1024.0 /1024.0);
>> +        MainChannelClient *mcc;
>> +        uint64_t net_test_bit_rate;
>> +
>> +        mcc = red_client_get_main(dcc->common.base.client);
>> +        net_test_bit_rate =
>> main_channel_client_is_network_info_initialized(mcc) ?
>> +
>> main_channel_client_get_bitrate_per_sec(mcc) :
>> +                                0;
>> +        bit_rate = MAX(dcc->streams_max_bit_rate, net_test_bit_rate);
>> +        if (bit_rate == 0) {
>> +            /*
>> +             * In case we are after a spice session migration,
>> +             * the low_bandwidth flag is retrieved from migration data.
>> +             * If the network info is not initialized due to another
>> reason,
>> +             * the low_bandwidth flag is FALSE.
>> +             */
>> +            bit_rate = dcc->common.is_low_bandwidth ?
>> +                RED_STREAM_DEFAULT_LOW_START_BIT_RATE :
>> +                RED_STREAM_DEFAULT_HIGH_START_BIT_RATE;
>> +        }
>> +    }
>> +
>> +    spice_debug("base-bit-rate %.2f (Mbps)", bit_rate / 1024.0 /
>> 1024.0);
>>       /* dividing the available bandwidth among the active streams,
>> and saving
>>        * (1-RED_STREAM_CHANNEL_CAPACITY) of it for other messages */
>>       return (RED_STREAM_CHANNEL_CAPACITY * bit_rate *
>> @@ -2973,6 +2985,12 @@ static uint32_t
>> red_stream_mjpeg_encoder_get_roundtrip(void *opaque)
>>       roundtrip =
>> red_channel_client_get_roundtrip_ms(&agent->dcc->common.base);
>>       if (roundtrip < 0) {
>>           MainChannelClient *mcc =
>> red_client_get_main(agent->dcc->common.base.client);
>> +
>> +        /*
>> +         * the main channel client roundtrip might not have been
>> +         * calculated (e.g., after migration). In such case,
>> +         * main_channel_client_get_roundtrip_ms returns 0.
>> +         */
>>           roundtrip = main_channel_client_get_roundtrip_ms(mcc);
>>       }
>>
>>