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

[Christophe Fergeau]

On Sat, Nov 09, 2013 at 04:09:35PM -0600, Jeremy White wrote:
> 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.

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.

> >>@@ -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.

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.


> >
> >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.

Fwiw, I noticed this while testing with opus spice-server and client, but with an
older qemu build (which only knows to handle 44100).

> >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 :(

> 
> 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 :-/


> >>+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. 

Yeah, scratch that comment, the reason I said that is because I was under
the impression that calling spice_server_set_record_rate() would overwrite
what was set using spice_server_set_playback_rate(). No clue why I was
under that impression, but it all looks fine today ;)

> And, to be honest,
> I also just used the function names you exposed in the patch set you sent me <evil grin>.

In these patches, I never looked at recording much, so I can turn that into
an excuse if needed! :p

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20131112/0c5ae7b5/attachment-0001.pgp>