>> #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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel