[Spice-devel] [codec 2/spice] Revise the spice server to use the new snd_codec functions in spice-common.

[Jeremy White]

Thanks for the review; it is much appreciated.

On 10/16/2013 06:05 AM, Christophe Fergeau wrote:
> The subject is misleading, it's not just changing spice-server, but both
> server and client.

Fair enough, I'll amend that.

> Can you fix the playbacke typo while at it?

Sure.

> This does not seem right, snd_codec_create() hardcodes the numbers of
> channels to SND_CODEC_CELT_PLAYBACK_CHAN, so we totally ignore whatever was
> in 'start', but here we use start->channels to compute _frame_bytes.

Fair enough.  It shouldn't have the appearance of dynamic computation if
it's not, really.

With that said, I think one of the issues with the sound code in general
is that the number of channels and the format are essentially hard
coded.  There are sporadic asserts to protect that, and sporadic
appearances of dynamic behavior, but anyone sending a mono track over a
spice channel is going to be in for a rude shock <grin>.

My next series makes the frequency dynamic, but leaves the other two
points hard coded.

This gave me some heart burn.

The value of the code being less intrusively changed seemed to outweigh
rewriting the code to create the appearance of full configuration of
channels and format.

I'm willing to be persuaded otherwise.

>> +AM_CONDITIONAL([HAVE_CELT051], [test "x$have_celt051" = "xyes"])
>> +AM_COND_IF([HAVE_CELT051], AC_DEFINE([HAVE_CELT051], 1, [Define if
we have celt051 codec]))
>
> Is it needed at all now that you moved the celt logic to spice-common?

It is needed in the current code (the size of SndCodec depends on the
config.h matching at each level).  But perhaps that suggests that
SndCodec should be allocated and freed by spice-common so it's entirely
opaque.

>> -#define SND_RECEIVE_BUF_SIZE (16 * 1024 * 2)
>> -
>> -#define FRAME_SIZE 256
>> -#define PLAYBACK_BUF_SIZE (FRAME_SIZE * 4)
>> -
>> -#define CELT_BIT_RATE (64 * 1024)
>> -#define CELT_COMPRESSED_FRAME_BYTES (FRAME_SIZE * CELT_BIT_RATE / SPICE_INTERFACE_PLAYBACK_FREQ / 8)
>> -
>> +#define SND_RECEIVE_BUF_SIZE     ((16 * 1024 * 2) >> 2)
>>  #define RECORD_SAMPLES_SIZE (SND_RECEIVE_BUF_SIZE >> 2)
> 
> Is it intentional that you made the buffer to receive sound data 4 times
> smaller than it was?

No; nicely caught.

>> +     } else {
>> +        int decode_size;
>> +        decode_size = sizeof(record_channel->decode_buf);
>> +        if (snd_codec_decode(&record_channel->codec, packet->data, packet->data_size,
>> +                    record_channel->decode_buf, &decode_size) != SND_CODEC_OK)
>> +            return FALSE;
>> +        data = (uint32_t *) record_channel->decode_buf;
>> +        size = decode_size >> 2;
> 
> I assume this is to go from bytes to samples? I still feel uncomfortable
> that these parameters are supposed to be hidden by SndCodec, but we still
> have some hardcoding about it here and there. With that said, I can
> probably live with that one.

(Similar issue as above)

>> -void snd_set_playback_compression(int on)
>> +void snd_set_playback_compression(int comp)
>>  {
>>      SndWorker *now = workers;
>>  
>> -    playback_compression = on ? SPICE_AUDIO_DATA_MODE_CELT_0_5_1 : SPICE_AUDIO_DATA_MODE_RAW;
>> +    playback_compression = comp;
>> +
>> +    int desired_mode = snd_desired_audio_mode(TRUE);
>> +
>>      for (; now; now = now->next) {
>>          if (now->base_channel->type == SPICE_CHANNEL_PLAYBACK && now->connection) {
>> -            SndChannel* sndchannel = now->connection;
>>              PlaybackChannel* playback = (PlaybackChannel*)now->connection;
>> -            if (!red_channel_client_test_remote_cap(sndchannel->channel_client,
>> -                                                    SPICE_PLAYBACK_CAP_CELT_0_5_1)) {
> 
> Shouldn't we keep the cap test when desired_mode != SPICE_AUDIO_DATA_MODE_RAW ?

Yes; my next patch actually does this differently.  This is an error on
my part caused by my failure to stage the two commits in logically clean
ways.

While we're on the subject, though - is there any good way to test this
code path?

Cheers,

Jeremy