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

[Jeremy White]

>> #if HAVE_CELT051 if (c && c->mode ==
>> SPICE_AUDIO_DATA_MODE_CELT_0_5_1) +    { +        /* The output
>> buffer size in celt determines the compression, +            and
>> so is essentially mandatory to use a certain value (47) */ +
>> if (*out_size > SND_CODEC_CELT_COMPRESSED_FRAME_BYTES) +
>> *out_size = SND_CODEC_CELT_COMPRESSED_FRAME_BYTES;
> 
> I would have put this in the previous series I think. Shouldn't it
> be made unconditionnally?

I agree that it seems logically connected to the previous series, but
the code as written previously would never trigger that logic.  I
generally hesitate to add code that will never fire in a patch, hence
my decision to put it here.


>> /* Spice uses a very fixed protocol when transmitting CELT
>> audio; audio must be transmitted in frames of 256, and we must
>> compress data down to a fairly specific size (47, computation
>> below). @@ -33,14 +37,19 @@ #define SND_CODEC_CELT_FRAME_SIZE
>> 256 #define SND_CODEC_CELT_BIT_RATE         (64 * 1024) #define
>> SND_CODEC_CELT_PLAYBACK_FREQ    44100 -#define
>> SND_CODEC_CELT_PLAYBACK_CHAN    2 #define
>> SND_CODEC_CELT_COMPRESSED_FRAME_BYTES (SND_CODEC_CELT_FRAME_SIZE
>> * SND_CODEC_CELT_BIT_RATE / \ SND_CODEC_CELT_PLAYBACK_FREQ / 8)
>> 
>> +#define SND_CODEC_OPUS_FRAME_SIZE       480 +#define
>> SND_CODEC_OPUS_PLAYBACK_FREQ    48000 +#define
>> SND_CODEC_OPUS_PLAYBACK_CHAN    2
> 
> Why add this and rename  SND_CODEC_CELT_PLAYBACK_CHAN to something
> generic at the same time?

Good point; I should remove the use of SND_CODEC_OPUS_PLAYBACK_CHAN.

[Define if we have celt051 codec]))
>> 
>> +PKG_CHECK_MODULES([OPUS], [opus >= 0.9.14], have_opus=yes,
>> have_opus=no)
> 
> I'd require 1.0.0

Debian wheezy only has 0.9.14, and 0.9.14 is ABI compatible with 1.0.0.

> 
>> + +AM_CONDITIONAL([HAVE_OPUS], [test "x$have_opus" = "xyes"]) +if
>> test "x$have_opus" = "xyes" ; then +  AC_DEFINE([HAVE_OPUS], [1],
>> [Define if we have OPUS]) +  SPICE_REQUIRES+=" opus >= 0.9.14" +
>> opus_version=`pkg-config --modversion opus`
> 
> This is not used, is it?

The AC_DEFINE is used, but the SPICE_REQUIRES and opus_version are not.

I was imprecise in my use of SPICE_REQUIRES in this patch series; I
should fix that.

Presuming you accept my responses, I'll spin up a take 3 on the first
three, adding back the configure check / SPICE_REQUIRES to the top
level.  I'll wait for more feedback on the rest of the Opus patches
before respinning those.

Cheers,

Jeremy