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

[Jeremy White]

Hey Christophe,

>>       static WaveRecordAbstract* create_recorder(RecordClient& client,
>>                                                  uint32_t sampels_per_sec,
>
> I'd tend to fix the 'sampel' typo while changing these functions (it shows
> up in several places in this patch).

Hmm. I can do that; my instinct is to avoid mixing changes in a patch;
I would tend to do a patch just for the rename, so that the git history has a
fairly clear record.

But it's a minor thing; I'm happy to fix up spelling as part of my job.

>
> With this hunk, we end up with 2 snd_codec_create calls in handle_start()

*blush*

It's amazing how easy it is to stay humble... at least for me :-/.

I'll fix that.

>> @@ -112,7 +114,10 @@ void RecordChannel::on_connect()
>>       Message* message = new Message(SPICE_MSGC_RECORD_MODE);
>>       SpiceMsgcRecordMode mode;
>>       mode.time = get_mm_time();
>> -    if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1) &&
>> +    if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_OPUS) &&
>> +      test_capability(SPICE_RECORD_CAP_OPUS))
>> +          _mode = SPICE_AUDIO_DATA_MODE_OPUS;
>
> I think we also need to check if the frequency is acceptable for Opus as
> the server-side code will silently fallback to celt/raw if the frequency is
> not good (I don't think there's a server -> client message for the
> recording channel to set the compression to use).
> This case can easily be tested with new spice-server/new client, but qemu
> binary built against spice 0.12.4

It's an awkward situation.  If the client requests a combination that
is unsupported, we have no way to reply to the client to say that it
didn't work.

To a large extent, the current logic is that if you report a CAP
(e.g. celt), you're really reporting that you can not only handle
celt, but you can handle it at the expected frequency.

But looking at the code, I think I should change it so that if the frequency
is invalid (which we'll know because codec creation will fail), we
disconnect the channel.  That's an easy change.

>
> Why did you move the recording codec creation from
> on_new_record_channel() to [msgc_record_mode handler] ? You did not do that for the playback
> channel, and more importantly, we lose the call to snd_desired_audio_mode()
> which makes sure we don't try to use Opus when the frequency is 44100.

Recording and playback are not identical; the client is allowed to
instruct the server as to what mode and frequency is being used in
recording, whereas playback appears to be entirely under server control.
So it does, imho, make sense to move the codec creation to the mode message.

However, I think you're right about the loss of the initial frequency
calculation.  That start message should likely go with a 48000 initial rate,
and a quick skim of the code suggests it currently sends 44.1, which
should cause trouble, I think.

I want to take some time and dig into this a bit more carefully; my
testing all worked, but now I'm not sure why.  I want to double check that
so I understand what's going on.

>
>> +SPICE_GNUC_VISIBLE uint32_t spice_server_get_best_playback_rate(SpicePlaybackInstance *sin)
>> +{
>> +    int client_can_opus = TRUE;
>> +    if (sin && sin->st->worker.connection)
>> +    {
>> +        SndChannel *channel = sin->st->worker.connection;
>> +        PlaybackChannel *playback_channel = SPICE_CONTAINEROF(channel, PlaybackChannel, base);
>> +        client_can_opus = red_channel_client_test_remote_cap(playback_channel->base.channel_client,
>> +                                          SPICE_PLAYBACK_CAP_OPUS);
>> +    }
>
> Why are there some caps checks in here? It's called from qemu
> line_out_init() which was called once at startup (when no client are
> connected) in my testing.

Checking the caps of a channel, if we have it, makes it a slightly
more useful implementation.

In the case of Xspice, for example, we can easily delay this creation
until we have a channel.

It also would, in theory, allow us to dynamically change the rate.  I have
tested that usage, and it does seem to work.

This may be useful for the case of an old client connecting to a new qemu image;
the current implementation will end up with a recording frequency mismatch (44.1 vs 48),
so recording doesn't work quite right.  I have a patch that 'fixes' this; I'm not
sure its worth the bother.

>

>> +SPICE_GNUC_VISIBLE void spice_server_set_record_rate(SpiceRecordInstance *sin, uint32_t frequency)
>> +{
>> +    sin->st->frequency = frequency;
>> +}
>
> As we can't change playback/record rate separately, maybe the setter should
> be spice_server_set_audio_rate()?

I don't know; I can see an argument for each approach.  My rationale for choosing
this approach was that it was more future proof.  Also, it is not clear to
me that the recording and playback channels have to use the same frequency;
in practice, they do, but I'm not sure if that's required.  And, to be honest,
I also just used the function names you exposed in the patch set you sent me <evil grin>.

Thanks for the review.

Cheers,

Jeremy