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

[Jeremy White]

>> 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.
>
> Yeah, I did not want to burden you with an additional patch, especially as
> it does not have to do with what you are trying to achieve. Feel free to
> ignore that remark.

Ah, it's an easy patch, and the typos bug me as well.

>> 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.
>
> Yup, would be nice.
> Looking at the code in qemu/audio/spiceaudio.c:line_out_init() and
> spice/server/snd_worker.c:snd_attach_playback() , I'm wondering if we could
> delay
> if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_OPUS))
>      red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_OPUS);
> until spice_server_set_playback_rate(&out->sin, settings.freq); is called.
> This way we only advertise OPUS support if we have a rate we can handle.

Yeah, that's what I've been working on.  The case of old qemu + new spice
server and client is a good test case, and my code didn't handle it well.

I'm hoping to have a set of patches today that better address those cases.

>>> 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.
>
> Yeah, reason I mentioned this is that at the moment it's mostly unused
> code, which can be surprising to someone reading the code. Maybe it's worth
> a comment saying that the way qemu uses this, the client caps checks won't
> be done? Though this comment is bound to bitrot :(

I'll see about adding a comment as to what the null parameter means.

>
>>
>> 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.
>
> Ah, that sounds bad (I've only tested playback until now), and I'm afraid
> we'll need to fix it sooner or later :-/

Yeah, I'll see if I can articulate the exact corner case; it's one of the clients
that had made a hard coded frequency assumption on the record channel, afair.

I'm hoping to have take 3 ready to send today.

Cheers,

Jeremy