[Spice-devel] [spice opus support 3/6 (take 3)] Add support for the Opus codec

[Jeremy White]

>> +SPICE_GNUC_VISIBLE uint32_t spice_server_get_best_record_rate(SpiceRecordInstance *sin)
>> +{
>> +    int client_can_opus = TRUE;
>> +    if (sin && sin->st->worker.connection)
>> +    {
>> +        SndChannel *channel = sin->st->worker.connection;
>> +        RecordChannel *record_channel = SPICE_CONTAINEROF(channel, RecordChannel, base);
>> +        client_can_opus = red_channel_client_test_remote_cap(record_channel->base.channel_client,
>> +                                          SPICE_RECORD_CAP_OPUS);
>> +    }
>> +
>> +    if (client_can_opus && snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_OPUS, SND_CODEC_ANY_FREQUENCY))
>
> Wouldn't it be better to use sin->st->frequency when sin is non-NULL?

No.  Frequency is always set server side, never from the client.

At this point in the code, sin->st->frequency will be set to 44100 for 
backwards compatibility reasons, so using it would be harmful.

>
>> +    {
>> +        return SND_CODEC_OPUS_PLAYBACK_FREQ;
>> +    }
>
> spice_server_get_best_playback_rate does not have these {}, I'd make them
> consistent

Yah.

In looking at this, though, I also notice that I failed to use K&R 
braces in a number of places.

That's disrespectful of me; I should fix it :-/.

>> @@ -1464,8 +1525,10 @@ void snd_attach_playback(SpicePlaybackInstance *sin)
>>       red_channel_register_client_cbs(channel, &client_cbs);
>>       red_channel_set_data(channel, playback_worker);
>>
>> -    if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1))
>> +    if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1, SND_CODEC_ANY_FREQUENCY))
>>           red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_CELT_0_5_1);
>> +    if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_OPUS, SND_CODEC_ANY_FREQUENCY))
>> +        red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_OPUS);
>
> I think I would have delayed this bit until
> spice_server_set_playback_rate() is called, but this works this way too.

I think you're right.  As written, we'll advertise Opus support, even 
when run with an old qemu.  I think if I shift that logic into the 
spice_server_set_xxx_rate functions, it will be cleaner.

The K&R fixes will require a respin of common/snd_codec.c.  I may also 
replace a certain true/false pair with a flag while I'm at it.

I'll try to put together another (hopefully final) round today or tomorrow.

Cheers,

Jeremy