[Spice-devel] make celt to be optional

Submitted by Michael Tokarev on June 2, 2012, 11:46 a.m.

Details

Message ID 1338637615-25967-1-git-send-email-mjt@msgid.tls.msk.ru
State New
Headers show

Not browsing as part of any series.

Commit Message

Michael Tokarev June 2, 2012, 11:46 a.m.
With this patch applied, celt051 library isn't required
anymore.  It is still required by default, but there's
a new configure option, --disable-celt051, which makes
the configure code to omit checking/finding the celt
library and makes resulting spice library to not use
celt codec at all.

The changes in the code - there are relatively many -
are located in 3 source files (see diffstat):

 client/playback_channel.cpp
 client/record_channel.cpp
 server/snd_worker.c

and are local/private to the library (client and server),
so no external ABI/API is done.

I found and marked hopefully all places where celt
codec is being touched/referenced.  The patch may
help future development too, indicating all places
where codec-specific code is used (just grep source
for HAVE_CELT051 to see these).

I plan to use this patch in the upcoming Debian
release, codename wheezy, to get rid of celt
codec library there, since we decided celt051 is
not going to be included, but it is obviously not
a good idea to drop spice entirely.

I did some interoperability tests and the thing
appears to work -- unpatched client to patched
server, patched client to unpatched server and
patched client to patched server.  I didn't check
old clients and servers, however.  In all cases,
raw audio stream is being choosen and used.  But
since I don't really know how spice works internally,
maybe I didn't perform correct testing.

Please consider applying.

Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>
Cc: Ron Lee <ron@debian.org>
Cc: Liang Guo <bluestonechina@gmail.com>
---
 client/audio_channels.h     |    8 +++++
 client/playback_channel.cpp |   25 ++++++++++---
 client/record_channel.cpp   |   21 +++++++++--
 configure.ac                |   16 ++++++---
 server/snd_worker.c         |   82 ++++++++++++++++++++++++++++++++++---------
 5 files changed, 122 insertions(+), 30 deletions(-)

Patch hide | download patch | download mbox

diff --git a/client/audio_channels.h b/client/audio_channels.h
index d38a79e..8f8a186 100644
--- a/client/audio_channels.h
+++ b/client/audio_channels.h
@@ -18,7 +18,9 @@ 
 #ifndef _H_AUDIO_CHANNELS
 #define _H_AUDIO_CHANNELS
 
+#if HAVE_CELT051
 #include <celt051/celt.h>
+#endif
 
 #include "red_channel.h"
 #include "debug.h"
@@ -45,7 +47,9 @@  private:
     void handle_start(RedPeer::InMessage* message);
     void handle_stop(RedPeer::InMessage* message);
     void handle_raw_data(RedPeer::InMessage* message);
+#if HAVE_CELT051
     void handle_celt_data(RedPeer::InMessage* message);
+#endif
     void null_handler(RedPeer::InMessage* message);
     void disable();
 
@@ -57,8 +61,10 @@  private:
     WavePlaybackAbstract* _wave_player;
     uint32_t _mode;
     uint32_t _frame_bytes;
+#if HAVE_CELT051
     CELTMode *_celt_mode;
     CELTDecoder *_celt_decoder;
+#endif
     bool _playing;
     uint32_t _frame_count;
 };
@@ -96,8 +102,10 @@  private:
     Mutex _messages_lock;
     std::list<RecordSamplesMessage *> _messages;
     int _mode;
+#if HAVE_CELT051
     CELTMode *_celt_mode;
     CELTEncoder *_celt_encoder;
+#endif
     uint32_t _frame_bytes;
 
     static int data_mode;
diff --git a/client/playback_channel.cpp b/client/playback_channel.cpp
index 802a4d3..caf6424 100644
--- a/client/playback_channel.cpp
+++ b/client/playback_channel.cpp
@@ -151,8 +151,10 @@  PlaybackChannel::PlaybackChannel(RedClient& client, uint32_t id)
                  Platform::PRIORITY_HIGH)
     , _wave_player (NULL)
     , _mode (SPICE_AUDIO_DATA_MODE_INVALID)
+#if HAVE_CELT051
     , _celt_mode (NULL)
     , _celt_decoder (NULL)
+#endif
     , _playing (false)
 {
 #ifdef WAVE_CAPTURE
@@ -169,7 +171,9 @@  PlaybackChannel::PlaybackChannel(RedClient& client, uint32_t id)
 
     handler->set_handler(SPICE_MSG_PLAYBACK_MODE, &PlaybackChannel::handle_mode);
 
+#if HAVE_CELT051
     set_capability(SPICE_PLAYBACK_CAP_CELT_0_5_1);
+#endif
 }
 
 void PlaybackChannel::clear()
@@ -182,6 +186,7 @@  void PlaybackChannel::clear()
     }
     _mode = SPICE_AUDIO_DATA_MODE_INVALID;
 
+#if HAVE_CELT051
     if (_celt_decoder) {
         celt051_decoder_destroy(_celt_decoder);
         _celt_decoder = NULL;
@@ -191,6 +196,7 @@  void PlaybackChannel::clear()
         celt051_mode_destroy(_celt_mode);
         _celt_mode = NULL;
     }
+#endif
 }
 
 void PlaybackChannel::on_disconnect()
@@ -214,8 +220,10 @@  void PlaybackChannel::set_data_handler()
 
     if (_mode == SPICE_AUDIO_DATA_MODE_RAW) {
         handler->set_handler(SPICE_MSG_PLAYBACK_DATA, &PlaybackChannel::handle_raw_data);
+#if HAVE_CELT051
     } else if (_mode == SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
         handler->set_handler(SPICE_MSG_PLAYBACK_DATA, &PlaybackChannel::handle_celt_data);
+#endif
     } else {
         THROW("invalid mode");
     }
@@ -224,8 +232,11 @@  void PlaybackChannel::set_data_handler()
 void PlaybackChannel::handle_mode(RedPeer::InMessage* message)
 {
     SpiceMsgPlaybackMode* playbacke_mode = (SpiceMsgPlaybackMode*)message->data();
-    if (playbacke_mode->mode != SPICE_AUDIO_DATA_MODE_RAW &&
-        playbacke_mode->mode != SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
+    if (playbacke_mode->mode != SPICE_AUDIO_DATA_MODE_RAW
+#if HAVE_CELT051
+        && playbacke_mode->mode != SPICE_AUDIO_DATA_MODE_CELT_0_5_1
+#endif
+       ) {
         THROW("invalid mode");
     }
 
@@ -265,9 +276,6 @@  void PlaybackChannel::handle_start(RedPeer::InMessage* message)
     start_wave();
 #endif
     if (!_wave_player) {
-        // for now support only one setting
-        int celt_mode_err;
-
         if (start->format != SPICE_AUDIO_FMT_S16) {
             THROW("unexpected format");
         }
@@ -284,6 +292,10 @@  void PlaybackChannel::handle_start(RedPeer::InMessage* message)
             return;
         }
 
+#if HAVE_CELT051
+        // for now support only one setting
+        int celt_mode_err;
+
         if (!(_celt_mode = celt051_mode_create(start->frequency, start->channels,
                                                frame_size, &celt_mode_err))) {
             THROW("create celt mode failed %d", celt_mode_err);
@@ -292,6 +304,7 @@  void PlaybackChannel::handle_start(RedPeer::InMessage* message)
         if (!(_celt_decoder = celt051_decoder_create(_celt_mode))) {
             THROW("create celt decoder");
         }
+#endif
     }
     _playing = true;
     _frame_count = 0;
@@ -333,6 +346,7 @@  void PlaybackChannel::handle_raw_data(RedPeer::InMessage* message)
     _wave_player->write(data);
 }
 
+#if HAVE_CELT051
 void PlaybackChannel::handle_celt_data(RedPeer::InMessage* message)
 {
     SpiceMsgPlaybackPacket* packet = (SpiceMsgPlaybackPacket*)message->data();
@@ -352,6 +366,7 @@  void PlaybackChannel::handle_celt_data(RedPeer::InMessage* message)
     }
     _wave_player->write((uint8_t *)pcm);
 }
+#endif
 
 class PlaybackFactory: public ChannelFactory {
 public:
diff --git a/client/record_channel.cpp b/client/record_channel.cpp
index d9332c6..dbf8344 100644
--- a/client/record_channel.cpp
+++ b/client/record_channel.cpp
@@ -72,8 +72,10 @@  RecordChannel::RecordChannel(RedClient& client, uint32_t id)
     : RedChannel(client, SPICE_CHANNEL_RECORD, id, new RecordHandler(*this))
     , _wave_recorder (NULL)
     , _mode (SPICE_AUDIO_DATA_MODE_INVALID)
+#if HAVE_CELT051
     , _celt_mode (NULL)
     , _celt_encoder (NULL)
+#endif
 {
     for (int i = 0; i < NUM_SAMPLES_MESSAGES; i++) {
         _messages.push_front(new RecordSamplesMessage(*this));
@@ -142,7 +144,11 @@  void RecordChannel::handle_start(RedPeer::InMessage* message)
 
     handler->set_handler(SPICE_MSG_RECORD_START, NULL);
     handler->set_handler(SPICE_MSG_RECORD_STOP, &RecordChannel::handle_stop);
+#if HAVE_CELT051
     ASSERT(!_wave_recorder && !_celt_mode && !_celt_encoder);
+#else
+    ASSERT(!_wave_recorder);
+#endif
 
     // for now support only one setting
     if (start->format != SPICE_AUDIO_FMT_S16) {
@@ -160,8 +166,9 @@  void RecordChannel::handle_start(RedPeer::InMessage* message)
     }
 
     int frame_size = 256;
-    int celt_mode_err;
     _frame_bytes = frame_size * bits_per_sample * start->channels / 8;
+#if HAVE_CELT051
+    int celt_mode_err;
     if (!(_celt_mode = celt051_mode_create(start->frequency, start->channels, frame_size,
                                            &celt_mode_err))) {
         THROW("create celt mode failed %d", celt_mode_err);
@@ -170,6 +177,7 @@  void RecordChannel::handle_start(RedPeer::InMessage* message)
     if (!(_celt_encoder = celt051_encoder_create(_celt_mode))) {
         THROW("create celt encoder failed");
     }
+#endif
 
     send_start_mark();
     _wave_recorder->start();
@@ -182,6 +190,7 @@  void RecordChannel::clear()
         delete _wave_recorder;
         _wave_recorder = NULL;
     }
+#if HAVE_CELT051
     if (_celt_encoder) {
         celt051_encoder_destroy(_celt_encoder);
         _celt_encoder = NULL;
@@ -190,6 +199,7 @@  void RecordChannel::clear()
         celt051_mode_destroy(_celt_mode);
         _celt_mode = NULL;
     }
+#endif
 }
 
 void RecordChannel::handle_stop(RedPeer::InMessage* message)
@@ -200,7 +210,9 @@  void RecordChannel::handle_stop(RedPeer::InMessage* message)
     if (!_wave_recorder) {
         return;
     }
+#if HAVE_CELT051
     ASSERT(_celt_mode && _celt_encoder);
+#endif
     clear();
 }
 
@@ -254,8 +266,9 @@  void RecordChannel::push_frame(uint8_t *frame)
         DBG(0, "blocked");
         return;
     }
-    uint8_t celt_buf[CELT_COMPRESSED_FRAME_BYTES];
     int n;
+#if HAVE_CELT051
+    uint8_t celt_buf[CELT_COMPRESSED_FRAME_BYTES];
 
     if (_mode == SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
         n = celt051_encode(_celt_encoder, (celt_int16_t *)frame, NULL, celt_buf,
@@ -264,7 +277,9 @@  void RecordChannel::push_frame(uint8_t *frame)
             THROW("celt encode failed");
         }
         frame = celt_buf;
-    } else {
+    } else
+#endif
+    {
         n = _frame_bytes;
     }
     RedPeer::OutMessage& peer_message = message->peer_message();
diff --git a/configure.ac b/configure.ac
index 66f9d12..21bd326 100644
--- a/configure.ac
+++ b/configure.ac
@@ -125,6 +125,9 @@  AM_CONDITIONAL(SUPPORT_SMARTCARD, test "x$enable_smartcard" != "xno")
 if test "x$enable_smartcard" = "xyes"; then
    AC_DEFINE([USE_SMARTCARD], [1], [Define if supporting smartcard proxying])
 fi
+AC_ARG_ENABLE(celt051,
+[  --disable-celt051       Disable celt051 audio codec (enabled by default)],,
+[enable_celt051="yes"])
 
 AC_ARG_ENABLE(client,
 [  --enable-client         Enable spice client],,
@@ -220,11 +223,14 @@  AC_SUBST(PIXMAN_CFLAGS)
 AC_SUBST(PIXMAN_LIBS)
 SPICE_REQUIRES+=" pixman-1 >= 0.17.7"
 
-PKG_CHECK_MODULES(CELT051, celt051 >= 0.5.1.1)
-AC_SUBST(CELT051_CFLAGS)
-AC_SUBST(CELT051_LIBS)
-AC_SUBST(CELT051_LIBDIR)
-SPICE_REQUIRES+=" celt051 >= 0.5.1.1"
+if test "x$enable_celt051" = "xyes"; then
+    PKG_CHECK_MODULES(CELT051, celt051 >= 0.5.1.1)
+    SPICE_REQUIRES+=" celt051 >= 0.5.1.1"
+    AC_DEFINE([HAVE_CELT051], 1, [Define if we have celt051 codec])
+    AC_SUBST(CELT051_CFLAGS)
+    AC_SUBST(CELT051_LIBS)
+    AC_SUBST(CELT051_LIBDIR)
+fi
 
 if test ! -e client/generated_marshallers.cpp; then
 AC_MSG_CHECKING([for pyparsing python module])
diff --git a/server/snd_worker.c b/server/snd_worker.c
index 3599c6f..f0588ad 100644
--- a/server/snd_worker.c
+++ b/server/snd_worker.c
@@ -25,7 +25,9 @@ 
 #include <sys/socket.h>
 #include <netinet/ip.h>
 #include <netinet/tcp.h>
+#if HAVE_CELT051
 #include <celt051/celt.h>
+#endif
 
 #include "common/marshaller.h"
 #include "common/generated_server_marshallers.h"
@@ -136,12 +138,14 @@  typedef struct PlaybackChannel {
     AudioFrame *free_frames;
     AudioFrame *in_progress;
     AudioFrame *pending_frame;
+    uint32_t mode;
+#if HAVE_CELT051
     CELTMode *celt_mode;
     CELTEncoder *celt_encoder;
-    uint32_t mode;
     struct {
         uint8_t celt_buf[CELT_COMPRESSED_FRAME_BYTES];
     } send_data;
+#endif
 } PlaybackChannel;
 
 struct SndWorker {
@@ -187,13 +191,21 @@  typedef struct RecordChannel {
     uint32_t mode;
     uint32_t mode_time;
     uint32_t start_time;
+#if HAVE_CELT051
     CELTDecoder *celt_decoder;
     CELTMode *celt_mode;
     uint32_t celt_buf[FRAME_SIZE];
+#endif
 } RecordChannel;
 
 static SndWorker *workers;
-static uint32_t playback_compression = SPICE_AUDIO_DATA_MODE_CELT_0_5_1;
+static uint32_t playback_compression =
+#if HAVE_CELT051
+    SPICE_AUDIO_DATA_MODE_CELT_0_5_1
+#else
+    SPICE_AUDIO_DATA_MODE_RAW
+#endif
+    ;
 
 static void snd_receive(void* data);
 
@@ -322,6 +334,7 @@  static int snd_record_handle_write(RecordChannel *record_channel, size_t size, v
     packet = (SpiceMsgcRecordPacket *)message;
     size = packet->data_size;
 
+#if HAVE_CELT051
     if (record_channel->mode == SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
         int celt_err = celt051_decode(record_channel->celt_decoder, packet->data, size,
                                       (celt_int16_t *)record_channel->celt_buf);
@@ -331,7 +344,9 @@  static int snd_record_handle_write(RecordChannel *record_channel, size_t size, v
         }
         data = record_channel->celt_buf;
         size = FRAME_SIZE;
-    } else if (record_channel->mode == SPICE_AUDIO_DATA_MODE_RAW) {
+    } else
+#endif
+    if (record_channel->mode == SPICE_AUDIO_DATA_MODE_RAW) {
         data = (uint32_t *)packet->data;
         size = size >> 2;
         size = MIN(size, RECORD_SAMPLES_SIZE);
@@ -386,8 +401,11 @@  static int snd_record_handle_message(SndChannel *channel, size_t size, uint32_t
         SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
         record_channel->mode = mode->mode;
         record_channel->mode_time = mode->time;
-        if (record_channel->mode != SPICE_AUDIO_DATA_MODE_CELT_0_5_1 &&
-                                                  record_channel->mode != SPICE_AUDIO_DATA_MODE_RAW) {
+        if (record_channel->mode != SPICE_AUDIO_DATA_MODE_RAW
+#if HAVE_CELT051
+            && record_channel->mode != SPICE_AUDIO_DATA_MODE_CELT_0_5_1
+#endif
+            ) {
             spice_printerr("unsupported mode");
         }
         break;
@@ -779,6 +797,7 @@  static int snd_playback_send_write(PlaybackChannel *playback_channel)
 
     spice_marshall_msg_playback_data(channel->send_data.marshaller, &msg);
 
+#if HAVE_CELT051
     if (playback_channel->mode == SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
         int n = celt051_encode(playback_channel->celt_encoder, (celt_int16_t *)frame->samples, NULL,
                                playback_channel->send_data.celt_buf, CELT_COMPRESSED_FRAME_BYTES);
@@ -789,7 +808,9 @@  static int snd_playback_send_write(PlaybackChannel *playback_channel)
         }
         spice_marshaller_add_ref(channel->send_data.marshaller,
                                  playback_channel->send_data.celt_buf, n);
-    } else {
+    } else
+#endif
+    {
         spice_marshaller_add_ref(channel->send_data.marshaller,
                                  (uint8_t *)frame->samples, sizeof(frame->samples));
     }
@@ -1157,8 +1178,10 @@  static void snd_playback_cleanup(SndChannel *channel)
         reds_enable_mm_timer();
     }
 
+#if HAVE_CELT051
     celt051_encoder_destroy(playback_channel->celt_encoder);
     celt051_mode_destroy(playback_channel->celt_mode);
+#endif
 }
 
 static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsStream *stream,
@@ -1168,13 +1191,13 @@  static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
     SndWorker *worker = channel->data;
     PlaybackChannel *playback_channel;
     SpicePlaybackState *st = SPICE_CONTAINEROF(worker, SpicePlaybackState, worker);
-    CELTEncoder *celt_encoder;
-    CELTMode *celt_mode;
-    int celt_error;
-    RedChannelClient *rcc;
 
     snd_disconnect_channel(worker->connection);
 
+#if HAVE_CELT051
+    CELTEncoder *celt_encoder;
+    CELTMode *celt_mode;
+    int celt_error;
     if (!(celt_mode = celt051_mode_create(SPICE_INTERFACE_PLAYBACK_FREQ,
                                           SPICE_INTERFACE_PLAYBACK_CHAN,
                                           FRAME_SIZE, &celt_error))) {
@@ -1186,6 +1209,7 @@  static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
         spice_printerr("create celt encoder failed");
         goto error_1;
     }
+#endif
 
     if (!(playback_channel = (PlaybackChannel *)__new_channel(worker,
                                                               sizeof(*playback_channel),
@@ -1202,16 +1226,20 @@  static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
         goto error_2;
     }
     worker->connection = &playback_channel->base;
-    rcc = playback_channel->base.channel_client;
     snd_playback_free_frame(playback_channel, &playback_channel->frames[0]);
     snd_playback_free_frame(playback_channel, &playback_channel->frames[1]);
     snd_playback_free_frame(playback_channel, &playback_channel->frames[2]);
 
+#if HAVE_CELT051
     playback_channel->celt_mode = celt_mode;
     playback_channel->celt_encoder = celt_encoder;
-    playback_channel->mode = red_channel_client_test_remote_cap(rcc,
-                                                                SPICE_PLAYBACK_CAP_CELT_0_5_1) ?
+    playback_channel->mode =
+       red_channel_client_test_remote_cap(playback_channel->base.channel_client,
+                                          SPICE_PLAYBACK_CAP_CELT_0_5_1) ?
         playback_compression : SPICE_AUDIO_DATA_MODE_RAW;
+#else
+    playback_channel->mode = SPICE_AUDIO_DATA_MODE_RAW;
+#endif
 
     on_new_playback_channel(worker);
     if (worker->active) {
@@ -1221,10 +1249,13 @@  static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
     return;
 
 error_2:
+#if HAVE_CELT051
     celt051_encoder_destroy(celt_encoder);
 
 error_1:
     celt051_mode_destroy(celt_mode);
+#endif
+    return;
 }
 
 static void snd_record_migrate_channel_client(RedChannelClient *rcc)
@@ -1365,10 +1396,12 @@  static void on_new_record_channel(SndWorker *worker)
 
 static void snd_record_cleanup(SndChannel *channel)
 {
+#if HAVE_CELT051
     RecordChannel *record_channel = SPICE_CONTAINEROF(channel, RecordChannel, base);
 
     celt051_decoder_destroy(record_channel->celt_decoder);
     celt051_mode_destroy(record_channel->celt_mode);
+#endif
 }
 
 static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStream *stream,
@@ -1378,12 +1411,13 @@  static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
     SndWorker *worker = channel->data;
     RecordChannel *record_channel;
     SpiceRecordState *st = SPICE_CONTAINEROF(worker, SpiceRecordState, worker);
-    CELTDecoder *celt_decoder;
-    CELTMode *celt_mode;
-    int celt_error;
 
     snd_disconnect_channel(worker->connection);
 
+#if HAVE_CELT051
+    CELTDecoder *celt_decoder;
+    CELTMode *celt_mode;
+    int celt_error;
     if (!(celt_mode = celt051_mode_create(SPICE_INTERFACE_RECORD_FREQ,
                                           SPICE_INTERFACE_RECORD_CHAN,
                                           FRAME_SIZE, &celt_error))) {
@@ -1395,6 +1429,7 @@  static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
         spice_printerr("create celt decoder failed");
         goto error_1;
     }
+#endif
 
     if (!(record_channel = (RecordChannel *)__new_channel(worker,
                                                           sizeof(*record_channel),
@@ -1413,8 +1448,10 @@  static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
 
     worker->connection = &record_channel->base;
 
+#if HAVE_CELT051
     record_channel->celt_mode = celt_mode;
     record_channel->celt_decoder = celt_decoder;
+#endif
 
     on_new_record_channel(worker);
     if (worker->active) {
@@ -1424,10 +1461,13 @@  static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
     return;
 
 error_2:
+#if HAVE_CELT051
     celt051_decoder_destroy(celt_decoder);
 
 error_1:
     celt051_mode_destroy(celt_mode);
+#endif
+    return;
 }
 
 static void snd_playback_migrate_channel_client(RedChannelClient *rcc)
@@ -1483,7 +1523,9 @@  void snd_attach_playback(SpicePlaybackInstance *sin)
     client_cbs.migrate = snd_playback_migrate_channel_client;
     red_channel_register_client_cbs(channel, &client_cbs);
     red_channel_set_data(channel, playback_worker);
+#if HAVE_CELT051
     red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_CELT_0_5_1);
+#endif
     red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_VOLUME);
 
     playback_worker->base_channel = channel;
@@ -1510,7 +1552,9 @@  void snd_attach_record(SpiceRecordInstance *sin)
     client_cbs.migrate = snd_record_migrate_channel_client;
     red_channel_register_client_cbs(channel, &client_cbs);
     red_channel_set_data(channel, record_worker);
+#if HAVE_CELT051
     red_channel_set_cap(channel, SPICE_RECORD_CAP_CELT_0_5_1);
+#endif
     red_channel_set_cap(channel, SPICE_RECORD_CAP_VOLUME);
 
     record_worker->base_channel = channel;
@@ -1557,7 +1601,11 @@  void snd_set_playback_compression(int on)
 {
     SndWorker *now = workers;
 
-    playback_compression = on ? SPICE_AUDIO_DATA_MODE_CELT_0_5_1 : SPICE_AUDIO_DATA_MODE_RAW;
+    playback_compression =
+#if HAVE_CELT051
+       on ? SPICE_AUDIO_DATA_MODE_CELT_0_5_1 :
+#endif
+       SPICE_AUDIO_DATA_MODE_RAW;
     for (; now; now = now->next) {
         if (now->base_channel->type == SPICE_CHANNEL_PLAYBACK && now->connection) {
             SndChannel* sndchannel = now->connection;

Comments

Makes sense to me though I've only looked quickly through it. Since it
works for you, I'm in favour of committing it if noone disagrees.

Christophe

On Sat, Jun 02, 2012 at 03:46:55PM +0400, Michael Tokarev wrote:
> With this patch applied, celt051 library isn't required
> anymore.  It is still required by default, but there's
> a new configure option, --disable-celt051, which makes
> the configure code to omit checking/finding the celt
> library and makes resulting spice library to not use
> celt codec at all.
> 
> The changes in the code - there are relatively many -
> are located in 3 source files (see diffstat):
> 
>  client/playback_channel.cpp
>  client/record_channel.cpp
>  server/snd_worker.c
> 
> and are local/private to the library (client and server),
> so no external ABI/API is done.
> 
> I found and marked hopefully all places where celt
> codec is being touched/referenced.  The patch may
> help future development too, indicating all places
> where codec-specific code is used (just grep source
> for HAVE_CELT051 to see these).
> 
> I plan to use this patch in the upcoming Debian
> release, codename wheezy, to get rid of celt
> codec library there, since we decided celt051 is
> not going to be included, but it is obviously not
> a good idea to drop spice entirely.
> 
> I did some interoperability tests and the thing
> appears to work -- unpatched client to patched
> server, patched client to unpatched server and
> patched client to patched server.  I didn't check
> old clients and servers, however.  In all cases,
> raw audio stream is being choosen and used.  But
> since I don't really know how spice works internally,
> maybe I didn't perform correct testing.
> 
> Please consider applying.
> 
> Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>
> Cc: Ron Lee <ron@debian.org>
> Cc: Liang Guo <bluestonechina@gmail.com>
> ---
>  client/audio_channels.h     |    8 +++++
>  client/playback_channel.cpp |   25 ++++++++++---
>  client/record_channel.cpp   |   21 +++++++++--
>  configure.ac                |   16 ++++++---
>  server/snd_worker.c         |   82 ++++++++++++++++++++++++++++++++++---------
>  5 files changed, 122 insertions(+), 30 deletions(-)
> 
> diff --git a/client/audio_channels.h b/client/audio_channels.h
> index d38a79e..8f8a186 100644
> --- a/client/audio_channels.h
> +++ b/client/audio_channels.h
> @@ -18,7 +18,9 @@
>  #ifndef _H_AUDIO_CHANNELS
>  #define _H_AUDIO_CHANNELS
>  
> +#if HAVE_CELT051
>  #include <celt051/celt.h>
> +#endif
>  
>  #include "red_channel.h"
>  #include "debug.h"
> @@ -45,7 +47,9 @@ private:
>      void handle_start(RedPeer::InMessage* message);
>      void handle_stop(RedPeer::InMessage* message);
>      void handle_raw_data(RedPeer::InMessage* message);
> +#if HAVE_CELT051
>      void handle_celt_data(RedPeer::InMessage* message);
> +#endif
>      void null_handler(RedPeer::InMessage* message);
>      void disable();
>  
> @@ -57,8 +61,10 @@ private:
>      WavePlaybackAbstract* _wave_player;
>      uint32_t _mode;
>      uint32_t _frame_bytes;
> +#if HAVE_CELT051
>      CELTMode *_celt_mode;
>      CELTDecoder *_celt_decoder;
> +#endif
>      bool _playing;
>      uint32_t _frame_count;
>  };
> @@ -96,8 +102,10 @@ private:
>      Mutex _messages_lock;
>      std::list<RecordSamplesMessage *> _messages;
>      int _mode;
> +#if HAVE_CELT051
>      CELTMode *_celt_mode;
>      CELTEncoder *_celt_encoder;
> +#endif
>      uint32_t _frame_bytes;
>  
>      static int data_mode;
> diff --git a/client/playback_channel.cpp b/client/playback_channel.cpp
> index 802a4d3..caf6424 100644
> --- a/client/playback_channel.cpp
> +++ b/client/playback_channel.cpp
> @@ -151,8 +151,10 @@ PlaybackChannel::PlaybackChannel(RedClient& client, uint32_t id)
>                   Platform::PRIORITY_HIGH)
>      , _wave_player (NULL)
>      , _mode (SPICE_AUDIO_DATA_MODE_INVALID)
> +#if HAVE_CELT051
>      , _celt_mode (NULL)
>      , _celt_decoder (NULL)
> +#endif
>      , _playing (false)
>  {
>  #ifdef WAVE_CAPTURE
> @@ -169,7 +171,9 @@ PlaybackChannel::PlaybackChannel(RedClient& client, uint32_t id)
>  
>      handler->set_handler(SPICE_MSG_PLAYBACK_MODE, &PlaybackChannel::handle_mode);
>  
> +#if HAVE_CELT051
>      set_capability(SPICE_PLAYBACK_CAP_CELT_0_5_1);
> +#endif
>  }
>  
>  void PlaybackChannel::clear()
> @@ -182,6 +186,7 @@ void PlaybackChannel::clear()
>      }
>      _mode = SPICE_AUDIO_DATA_MODE_INVALID;
>  
> +#if HAVE_CELT051
>      if (_celt_decoder) {
>          celt051_decoder_destroy(_celt_decoder);
>          _celt_decoder = NULL;
> @@ -191,6 +196,7 @@ void PlaybackChannel::clear()
>          celt051_mode_destroy(_celt_mode);
>          _celt_mode = NULL;
>      }
> +#endif
>  }
>  
>  void PlaybackChannel::on_disconnect()
> @@ -214,8 +220,10 @@ void PlaybackChannel::set_data_handler()
>  
>      if (_mode == SPICE_AUDIO_DATA_MODE_RAW) {
>          handler->set_handler(SPICE_MSG_PLAYBACK_DATA, &PlaybackChannel::handle_raw_data);
> +#if HAVE_CELT051
>      } else if (_mode == SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
>          handler->set_handler(SPICE_MSG_PLAYBACK_DATA, &PlaybackChannel::handle_celt_data);
> +#endif
>      } else {
>          THROW("invalid mode");
>      }
> @@ -224,8 +232,11 @@ void PlaybackChannel::set_data_handler()
>  void PlaybackChannel::handle_mode(RedPeer::InMessage* message)
>  {
>      SpiceMsgPlaybackMode* playbacke_mode = (SpiceMsgPlaybackMode*)message->data();
> -    if (playbacke_mode->mode != SPICE_AUDIO_DATA_MODE_RAW &&
> -        playbacke_mode->mode != SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
> +    if (playbacke_mode->mode != SPICE_AUDIO_DATA_MODE_RAW
> +#if HAVE_CELT051
> +        && playbacke_mode->mode != SPICE_AUDIO_DATA_MODE_CELT_0_5_1
> +#endif
> +       ) {
>          THROW("invalid mode");
>      }
>  
> @@ -265,9 +276,6 @@ void PlaybackChannel::handle_start(RedPeer::InMessage* message)
>      start_wave();
>  #endif
>      if (!_wave_player) {
> -        // for now support only one setting
> -        int celt_mode_err;
> -
>          if (start->format != SPICE_AUDIO_FMT_S16) {
>              THROW("unexpected format");
>          }
> @@ -284,6 +292,10 @@ void PlaybackChannel::handle_start(RedPeer::InMessage* message)
>              return;
>          }
>  
> +#if HAVE_CELT051
> +        // for now support only one setting
> +        int celt_mode_err;
> +
>          if (!(_celt_mode = celt051_mode_create(start->frequency, start->channels,
>                                                 frame_size, &celt_mode_err))) {
>              THROW("create celt mode failed %d", celt_mode_err);
> @@ -292,6 +304,7 @@ void PlaybackChannel::handle_start(RedPeer::InMessage* message)
>          if (!(_celt_decoder = celt051_decoder_create(_celt_mode))) {
>              THROW("create celt decoder");
>          }
> +#endif
>      }
>      _playing = true;
>      _frame_count = 0;
> @@ -333,6 +346,7 @@ void PlaybackChannel::handle_raw_data(RedPeer::InMessage* message)
>      _wave_player->write(data);
>  }
>  
> +#if HAVE_CELT051
>  void PlaybackChannel::handle_celt_data(RedPeer::InMessage* message)
>  {
>      SpiceMsgPlaybackPacket* packet = (SpiceMsgPlaybackPacket*)message->data();
> @@ -352,6 +366,7 @@ void PlaybackChannel::handle_celt_data(RedPeer::InMessage* message)
>      }
>      _wave_player->write((uint8_t *)pcm);
>  }
> +#endif
>  
>  class PlaybackFactory: public ChannelFactory {
>  public:
> diff --git a/client/record_channel.cpp b/client/record_channel.cpp
> index d9332c6..dbf8344 100644
> --- a/client/record_channel.cpp
> +++ b/client/record_channel.cpp
> @@ -72,8 +72,10 @@ RecordChannel::RecordChannel(RedClient& client, uint32_t id)
>      : RedChannel(client, SPICE_CHANNEL_RECORD, id, new RecordHandler(*this))
>      , _wave_recorder (NULL)
>      , _mode (SPICE_AUDIO_DATA_MODE_INVALID)
> +#if HAVE_CELT051
>      , _celt_mode (NULL)
>      , _celt_encoder (NULL)
> +#endif
>  {
>      for (int i = 0; i < NUM_SAMPLES_MESSAGES; i++) {
>          _messages.push_front(new RecordSamplesMessage(*this));
> @@ -142,7 +144,11 @@ void RecordChannel::handle_start(RedPeer::InMessage* message)
>  
>      handler->set_handler(SPICE_MSG_RECORD_START, NULL);
>      handler->set_handler(SPICE_MSG_RECORD_STOP, &RecordChannel::handle_stop);
> +#if HAVE_CELT051
>      ASSERT(!_wave_recorder && !_celt_mode && !_celt_encoder);
> +#else
> +    ASSERT(!_wave_recorder);
> +#endif
>  
>      // for now support only one setting
>      if (start->format != SPICE_AUDIO_FMT_S16) {
> @@ -160,8 +166,9 @@ void RecordChannel::handle_start(RedPeer::InMessage* message)
>      }
>  
>      int frame_size = 256;
> -    int celt_mode_err;
>      _frame_bytes = frame_size * bits_per_sample * start->channels / 8;
> +#if HAVE_CELT051
> +    int celt_mode_err;
>      if (!(_celt_mode = celt051_mode_create(start->frequency, start->channels, frame_size,
>                                             &celt_mode_err))) {
>          THROW("create celt mode failed %d", celt_mode_err);
> @@ -170,6 +177,7 @@ void RecordChannel::handle_start(RedPeer::InMessage* message)
>      if (!(_celt_encoder = celt051_encoder_create(_celt_mode))) {
>          THROW("create celt encoder failed");
>      }
> +#endif
>  
>      send_start_mark();
>      _wave_recorder->start();
> @@ -182,6 +190,7 @@ void RecordChannel::clear()
>          delete _wave_recorder;
>          _wave_recorder = NULL;
>      }
> +#if HAVE_CELT051
>      if (_celt_encoder) {
>          celt051_encoder_destroy(_celt_encoder);
>          _celt_encoder = NULL;
> @@ -190,6 +199,7 @@ void RecordChannel::clear()
>          celt051_mode_destroy(_celt_mode);
>          _celt_mode = NULL;
>      }
> +#endif
>  }
>  
>  void RecordChannel::handle_stop(RedPeer::InMessage* message)
> @@ -200,7 +210,9 @@ void RecordChannel::handle_stop(RedPeer::InMessage* message)
>      if (!_wave_recorder) {
>          return;
>      }
> +#if HAVE_CELT051
>      ASSERT(_celt_mode && _celt_encoder);
> +#endif
>      clear();
>  }
>  
> @@ -254,8 +266,9 @@ void RecordChannel::push_frame(uint8_t *frame)
>          DBG(0, "blocked");
>          return;
>      }
> -    uint8_t celt_buf[CELT_COMPRESSED_FRAME_BYTES];
>      int n;
> +#if HAVE_CELT051
> +    uint8_t celt_buf[CELT_COMPRESSED_FRAME_BYTES];
>  
>      if (_mode == SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
>          n = celt051_encode(_celt_encoder, (celt_int16_t *)frame, NULL, celt_buf,
> @@ -264,7 +277,9 @@ void RecordChannel::push_frame(uint8_t *frame)
>              THROW("celt encode failed");
>          }
>          frame = celt_buf;
> -    } else {
> +    } else
> +#endif
> +    {
>          n = _frame_bytes;
>      }
>      RedPeer::OutMessage& peer_message = message->peer_message();
> diff --git a/configure.ac b/configure.ac
> index 66f9d12..21bd326 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -125,6 +125,9 @@ AM_CONDITIONAL(SUPPORT_SMARTCARD, test "x$enable_smartcard" != "xno")
>  if test "x$enable_smartcard" = "xyes"; then
>     AC_DEFINE([USE_SMARTCARD], [1], [Define if supporting smartcard proxying])
>  fi
> +AC_ARG_ENABLE(celt051,
> +[  --disable-celt051       Disable celt051 audio codec (enabled by default)],,
> +[enable_celt051="yes"])
>  
>  AC_ARG_ENABLE(client,
>  [  --enable-client         Enable spice client],,
> @@ -220,11 +223,14 @@ AC_SUBST(PIXMAN_CFLAGS)
>  AC_SUBST(PIXMAN_LIBS)
>  SPICE_REQUIRES+=" pixman-1 >= 0.17.7"
>  
> -PKG_CHECK_MODULES(CELT051, celt051 >= 0.5.1.1)
> -AC_SUBST(CELT051_CFLAGS)
> -AC_SUBST(CELT051_LIBS)
> -AC_SUBST(CELT051_LIBDIR)
> -SPICE_REQUIRES+=" celt051 >= 0.5.1.1"
> +if test "x$enable_celt051" = "xyes"; then
> +    PKG_CHECK_MODULES(CELT051, celt051 >= 0.5.1.1)
> +    SPICE_REQUIRES+=" celt051 >= 0.5.1.1"
> +    AC_DEFINE([HAVE_CELT051], 1, [Define if we have celt051 codec])
> +    AC_SUBST(CELT051_CFLAGS)
> +    AC_SUBST(CELT051_LIBS)
> +    AC_SUBST(CELT051_LIBDIR)
> +fi
>  
>  if test ! -e client/generated_marshallers.cpp; then
>  AC_MSG_CHECKING([for pyparsing python module])
> diff --git a/server/snd_worker.c b/server/snd_worker.c
> index 3599c6f..f0588ad 100644
> --- a/server/snd_worker.c
> +++ b/server/snd_worker.c
> @@ -25,7 +25,9 @@
>  #include <sys/socket.h>
>  #include <netinet/ip.h>
>  #include <netinet/tcp.h>
> +#if HAVE_CELT051
>  #include <celt051/celt.h>
> +#endif
>  
>  #include "common/marshaller.h"
>  #include "common/generated_server_marshallers.h"
> @@ -136,12 +138,14 @@ typedef struct PlaybackChannel {
>      AudioFrame *free_frames;
>      AudioFrame *in_progress;
>      AudioFrame *pending_frame;
> +    uint32_t mode;
> +#if HAVE_CELT051
>      CELTMode *celt_mode;
>      CELTEncoder *celt_encoder;
> -    uint32_t mode;
>      struct {
>          uint8_t celt_buf[CELT_COMPRESSED_FRAME_BYTES];
>      } send_data;
> +#endif
>  } PlaybackChannel;
>  
>  struct SndWorker {
> @@ -187,13 +191,21 @@ typedef struct RecordChannel {
>      uint32_t mode;
>      uint32_t mode_time;
>      uint32_t start_time;
> +#if HAVE_CELT051
>      CELTDecoder *celt_decoder;
>      CELTMode *celt_mode;
>      uint32_t celt_buf[FRAME_SIZE];
> +#endif
>  } RecordChannel;
>  
>  static SndWorker *workers;
> -static uint32_t playback_compression = SPICE_AUDIO_DATA_MODE_CELT_0_5_1;
> +static uint32_t playback_compression =
> +#if HAVE_CELT051
> +    SPICE_AUDIO_DATA_MODE_CELT_0_5_1
> +#else
> +    SPICE_AUDIO_DATA_MODE_RAW
> +#endif
> +    ;
>  
>  static void snd_receive(void* data);
>  
> @@ -322,6 +334,7 @@ static int snd_record_handle_write(RecordChannel *record_channel, size_t size, v
>      packet = (SpiceMsgcRecordPacket *)message;
>      size = packet->data_size;
>  
> +#if HAVE_CELT051
>      if (record_channel->mode == SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
>          int celt_err = celt051_decode(record_channel->celt_decoder, packet->data, size,
>                                        (celt_int16_t *)record_channel->celt_buf);
> @@ -331,7 +344,9 @@ static int snd_record_handle_write(RecordChannel *record_channel, size_t size, v
>          }
>          data = record_channel->celt_buf;
>          size = FRAME_SIZE;
> -    } else if (record_channel->mode == SPICE_AUDIO_DATA_MODE_RAW) {
> +    } else
> +#endif
> +    if (record_channel->mode == SPICE_AUDIO_DATA_MODE_RAW) {
>          data = (uint32_t *)packet->data;
>          size = size >> 2;
>          size = MIN(size, RECORD_SAMPLES_SIZE);
> @@ -386,8 +401,11 @@ static int snd_record_handle_message(SndChannel *channel, size_t size, uint32_t
>          SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
>          record_channel->mode = mode->mode;
>          record_channel->mode_time = mode->time;
> -        if (record_channel->mode != SPICE_AUDIO_DATA_MODE_CELT_0_5_1 &&
> -                                                  record_channel->mode != SPICE_AUDIO_DATA_MODE_RAW) {
> +        if (record_channel->mode != SPICE_AUDIO_DATA_MODE_RAW
> +#if HAVE_CELT051
> +            && record_channel->mode != SPICE_AUDIO_DATA_MODE_CELT_0_5_1
> +#endif
> +            ) {
>              spice_printerr("unsupported mode");
>          }
>          break;
> @@ -779,6 +797,7 @@ static int snd_playback_send_write(PlaybackChannel *playback_channel)
>  
>      spice_marshall_msg_playback_data(channel->send_data.marshaller, &msg);
>  
> +#if HAVE_CELT051
>      if (playback_channel->mode == SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
>          int n = celt051_encode(playback_channel->celt_encoder, (celt_int16_t *)frame->samples, NULL,
>                                 playback_channel->send_data.celt_buf, CELT_COMPRESSED_FRAME_BYTES);
> @@ -789,7 +808,9 @@ static int snd_playback_send_write(PlaybackChannel *playback_channel)
>          }
>          spice_marshaller_add_ref(channel->send_data.marshaller,
>                                   playback_channel->send_data.celt_buf, n);
> -    } else {
> +    } else
> +#endif
> +    {
>          spice_marshaller_add_ref(channel->send_data.marshaller,
>                                   (uint8_t *)frame->samples, sizeof(frame->samples));
>      }
> @@ -1157,8 +1178,10 @@ static void snd_playback_cleanup(SndChannel *channel)
>          reds_enable_mm_timer();
>      }
>  
> +#if HAVE_CELT051
>      celt051_encoder_destroy(playback_channel->celt_encoder);
>      celt051_mode_destroy(playback_channel->celt_mode);
> +#endif
>  }
>  
>  static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsStream *stream,
> @@ -1168,13 +1191,13 @@ static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
>      SndWorker *worker = channel->data;
>      PlaybackChannel *playback_channel;
>      SpicePlaybackState *st = SPICE_CONTAINEROF(worker, SpicePlaybackState, worker);
> -    CELTEncoder *celt_encoder;
> -    CELTMode *celt_mode;
> -    int celt_error;
> -    RedChannelClient *rcc;
>  
>      snd_disconnect_channel(worker->connection);
>  
> +#if HAVE_CELT051
> +    CELTEncoder *celt_encoder;
> +    CELTMode *celt_mode;
> +    int celt_error;
>      if (!(celt_mode = celt051_mode_create(SPICE_INTERFACE_PLAYBACK_FREQ,
>                                            SPICE_INTERFACE_PLAYBACK_CHAN,
>                                            FRAME_SIZE, &celt_error))) {
> @@ -1186,6 +1209,7 @@ static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
>          spice_printerr("create celt encoder failed");
>          goto error_1;
>      }
> +#endif
>  
>      if (!(playback_channel = (PlaybackChannel *)__new_channel(worker,
>                                                                sizeof(*playback_channel),
> @@ -1202,16 +1226,20 @@ static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
>          goto error_2;
>      }
>      worker->connection = &playback_channel->base;
> -    rcc = playback_channel->base.channel_client;
>      snd_playback_free_frame(playback_channel, &playback_channel->frames[0]);
>      snd_playback_free_frame(playback_channel, &playback_channel->frames[1]);
>      snd_playback_free_frame(playback_channel, &playback_channel->frames[2]);
>  
> +#if HAVE_CELT051
>      playback_channel->celt_mode = celt_mode;
>      playback_channel->celt_encoder = celt_encoder;
> -    playback_channel->mode = red_channel_client_test_remote_cap(rcc,
> -                                                                SPICE_PLAYBACK_CAP_CELT_0_5_1) ?
> +    playback_channel->mode =
> +       red_channel_client_test_remote_cap(playback_channel->base.channel_client,
> +                                          SPICE_PLAYBACK_CAP_CELT_0_5_1) ?
>          playback_compression : SPICE_AUDIO_DATA_MODE_RAW;
> +#else
> +    playback_channel->mode = SPICE_AUDIO_DATA_MODE_RAW;
> +#endif
>  
>      on_new_playback_channel(worker);
>      if (worker->active) {
> @@ -1221,10 +1249,13 @@ static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
>      return;
>  
>  error_2:
> +#if HAVE_CELT051
>      celt051_encoder_destroy(celt_encoder);
>  
>  error_1:
>      celt051_mode_destroy(celt_mode);
> +#endif
> +    return;
>  }
>  
>  static void snd_record_migrate_channel_client(RedChannelClient *rcc)
> @@ -1365,10 +1396,12 @@ static void on_new_record_channel(SndWorker *worker)
>  
>  static void snd_record_cleanup(SndChannel *channel)
>  {
> +#if HAVE_CELT051
>      RecordChannel *record_channel = SPICE_CONTAINEROF(channel, RecordChannel, base);
>  
>      celt051_decoder_destroy(record_channel->celt_decoder);
>      celt051_mode_destroy(record_channel->celt_mode);
> +#endif
>  }
>  
>  static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStream *stream,
> @@ -1378,12 +1411,13 @@ static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
>      SndWorker *worker = channel->data;
>      RecordChannel *record_channel;
>      SpiceRecordState *st = SPICE_CONTAINEROF(worker, SpiceRecordState, worker);
> -    CELTDecoder *celt_decoder;
> -    CELTMode *celt_mode;
> -    int celt_error;
>  
>      snd_disconnect_channel(worker->connection);
>  
> +#if HAVE_CELT051
> +    CELTDecoder *celt_decoder;
> +    CELTMode *celt_mode;
> +    int celt_error;
>      if (!(celt_mode = celt051_mode_create(SPICE_INTERFACE_RECORD_FREQ,
>                                            SPICE_INTERFACE_RECORD_CHAN,
>                                            FRAME_SIZE, &celt_error))) {
> @@ -1395,6 +1429,7 @@ static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
>          spice_printerr("create celt decoder failed");
>          goto error_1;
>      }
> +#endif
>  
>      if (!(record_channel = (RecordChannel *)__new_channel(worker,
>                                                            sizeof(*record_channel),
> @@ -1413,8 +1448,10 @@ static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
>  
>      worker->connection = &record_channel->base;
>  
> +#if HAVE_CELT051
>      record_channel->celt_mode = celt_mode;
>      record_channel->celt_decoder = celt_decoder;
> +#endif
>  
>      on_new_record_channel(worker);
>      if (worker->active) {
> @@ -1424,10 +1461,13 @@ static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
>      return;
>  
>  error_2:
> +#if HAVE_CELT051
>      celt051_decoder_destroy(celt_decoder);
>  
>  error_1:
>      celt051_mode_destroy(celt_mode);
> +#endif
> +    return;
>  }
>  
>  static void snd_playback_migrate_channel_client(RedChannelClient *rcc)
> @@ -1483,7 +1523,9 @@ void snd_attach_playback(SpicePlaybackInstance *sin)
>      client_cbs.migrate = snd_playback_migrate_channel_client;
>      red_channel_register_client_cbs(channel, &client_cbs);
>      red_channel_set_data(channel, playback_worker);
> +#if HAVE_CELT051
>      red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_CELT_0_5_1);
> +#endif
>      red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_VOLUME);
>  
>      playback_worker->base_channel = channel;
> @@ -1510,7 +1552,9 @@ void snd_attach_record(SpiceRecordInstance *sin)
>      client_cbs.migrate = snd_record_migrate_channel_client;
>      red_channel_register_client_cbs(channel, &client_cbs);
>      red_channel_set_data(channel, record_worker);
> +#if HAVE_CELT051
>      red_channel_set_cap(channel, SPICE_RECORD_CAP_CELT_0_5_1);
> +#endif
>      red_channel_set_cap(channel, SPICE_RECORD_CAP_VOLUME);
>  
>      record_worker->base_channel = channel;
> @@ -1557,7 +1601,11 @@ void snd_set_playback_compression(int on)
>  {
>      SndWorker *now = workers;
>  
> -    playback_compression = on ? SPICE_AUDIO_DATA_MODE_CELT_0_5_1 : SPICE_AUDIO_DATA_MODE_RAW;
> +    playback_compression =
> +#if HAVE_CELT051
> +       on ? SPICE_AUDIO_DATA_MODE_CELT_0_5_1 :
> +#endif
> +       SPICE_AUDIO_DATA_MODE_RAW;
>      for (; now; now = now->next) {
>          if (now->base_channel->type == SPICE_CHANNEL_PLAYBACK && now->connection) {
>              SndChannel* sndchannel = now->connection;
> -- 
> 1.7.10
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Mon, Jun 04, 2012 at 11:33:22AM +0200, Christophe Fergeau wrote:
> Makes sense to me though I've only looked quickly through it. Since it
> works for you, I'm in favour of committing it if noone disagrees.

I agree. Would you do a review and ack?

> 
> Christophe
> 
> On Sat, Jun 02, 2012 at 03:46:55PM +0400, Michael Tokarev wrote:
> > With this patch applied, celt051 library isn't required
> > anymore.  It is still required by default, but there's
> > a new configure option, --disable-celt051, which makes
> > the configure code to omit checking/finding the celt
> > library and makes resulting spice library to not use
> > celt codec at all.
> > 
> > The changes in the code - there are relatively many -
> > are located in 3 source files (see diffstat):
> > 
> >  client/playback_channel.cpp
> >  client/record_channel.cpp
> >  server/snd_worker.c
> > 
> > and are local/private to the library (client and server),
> > so no external ABI/API is done.
> > 
> > I found and marked hopefully all places where celt
> > codec is being touched/referenced.  The patch may
> > help future development too, indicating all places
> > where codec-specific code is used (just grep source
> > for HAVE_CELT051 to see these).
> > 
> > I plan to use this patch in the upcoming Debian
> > release, codename wheezy, to get rid of celt
> > codec library there, since we decided celt051 is
> > not going to be included, but it is obviously not
> > a good idea to drop spice entirely.
> > 
> > I did some interoperability tests and the thing
> > appears to work -- unpatched client to patched
> > server, patched client to unpatched server and
> > patched client to patched server.  I didn't check
> > old clients and servers, however.  In all cases,
> > raw audio stream is being choosen and used.  But
> > since I don't really know how spice works internally,
> > maybe I didn't perform correct testing.
> > 
> > Please consider applying.
> > 
> > Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>
> > Cc: Ron Lee <ron@debian.org>
> > Cc: Liang Guo <bluestonechina@gmail.com>
> > ---
> >  client/audio_channels.h     |    8 +++++
> >  client/playback_channel.cpp |   25 ++++++++++---
> >  client/record_channel.cpp   |   21 +++++++++--
> >  configure.ac                |   16 ++++++---
> >  server/snd_worker.c         |   82 ++++++++++++++++++++++++++++++++++---------
> >  5 files changed, 122 insertions(+), 30 deletions(-)
> > 
> > diff --git a/client/audio_channels.h b/client/audio_channels.h
> > index d38a79e..8f8a186 100644
> > --- a/client/audio_channels.h
> > +++ b/client/audio_channels.h
> > @@ -18,7 +18,9 @@
> >  #ifndef _H_AUDIO_CHANNELS
> >  #define _H_AUDIO_CHANNELS
> >  
> > +#if HAVE_CELT051
> >  #include <celt051/celt.h>
> > +#endif
> >  
> >  #include "red_channel.h"
> >  #include "debug.h"
> > @@ -45,7 +47,9 @@ private:
> >      void handle_start(RedPeer::InMessage* message);
> >      void handle_stop(RedPeer::InMessage* message);
> >      void handle_raw_data(RedPeer::InMessage* message);
> > +#if HAVE_CELT051
> >      void handle_celt_data(RedPeer::InMessage* message);
> > +#endif
> >      void null_handler(RedPeer::InMessage* message);
> >      void disable();
> >  
> > @@ -57,8 +61,10 @@ private:
> >      WavePlaybackAbstract* _wave_player;
> >      uint32_t _mode;
> >      uint32_t _frame_bytes;
> > +#if HAVE_CELT051
> >      CELTMode *_celt_mode;
> >      CELTDecoder *_celt_decoder;
> > +#endif
> >      bool _playing;
> >      uint32_t _frame_count;
> >  };
> > @@ -96,8 +102,10 @@ private:
> >      Mutex _messages_lock;
> >      std::list<RecordSamplesMessage *> _messages;
> >      int _mode;
> > +#if HAVE_CELT051
> >      CELTMode *_celt_mode;
> >      CELTEncoder *_celt_encoder;
> > +#endif
> >      uint32_t _frame_bytes;
> >  
> >      static int data_mode;
> > diff --git a/client/playback_channel.cpp b/client/playback_channel.cpp
> > index 802a4d3..caf6424 100644
> > --- a/client/playback_channel.cpp
> > +++ b/client/playback_channel.cpp
> > @@ -151,8 +151,10 @@ PlaybackChannel::PlaybackChannel(RedClient& client, uint32_t id)
> >                   Platform::PRIORITY_HIGH)
> >      , _wave_player (NULL)
> >      , _mode (SPICE_AUDIO_DATA_MODE_INVALID)
> > +#if HAVE_CELT051
> >      , _celt_mode (NULL)
> >      , _celt_decoder (NULL)
> > +#endif
> >      , _playing (false)
> >  {
> >  #ifdef WAVE_CAPTURE
> > @@ -169,7 +171,9 @@ PlaybackChannel::PlaybackChannel(RedClient& client, uint32_t id)
> >  
> >      handler->set_handler(SPICE_MSG_PLAYBACK_MODE, &PlaybackChannel::handle_mode);
> >  
> > +#if HAVE_CELT051
> >      set_capability(SPICE_PLAYBACK_CAP_CELT_0_5_1);
> > +#endif
> >  }
> >  
> >  void PlaybackChannel::clear()
> > @@ -182,6 +186,7 @@ void PlaybackChannel::clear()
> >      }
> >      _mode = SPICE_AUDIO_DATA_MODE_INVALID;
> >  
> > +#if HAVE_CELT051
> >      if (_celt_decoder) {
> >          celt051_decoder_destroy(_celt_decoder);
> >          _celt_decoder = NULL;
> > @@ -191,6 +196,7 @@ void PlaybackChannel::clear()
> >          celt051_mode_destroy(_celt_mode);
> >          _celt_mode = NULL;
> >      }
> > +#endif
> >  }
> >  
> >  void PlaybackChannel::on_disconnect()
> > @@ -214,8 +220,10 @@ void PlaybackChannel::set_data_handler()
> >  
> >      if (_mode == SPICE_AUDIO_DATA_MODE_RAW) {
> >          handler->set_handler(SPICE_MSG_PLAYBACK_DATA, &PlaybackChannel::handle_raw_data);
> > +#if HAVE_CELT051
> >      } else if (_mode == SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
> >          handler->set_handler(SPICE_MSG_PLAYBACK_DATA, &PlaybackChannel::handle_celt_data);
> > +#endif
> >      } else {
> >          THROW("invalid mode");
> >      }
> > @@ -224,8 +232,11 @@ void PlaybackChannel::set_data_handler()
> >  void PlaybackChannel::handle_mode(RedPeer::InMessage* message)
> >  {
> >      SpiceMsgPlaybackMode* playbacke_mode = (SpiceMsgPlaybackMode*)message->data();
> > -    if (playbacke_mode->mode != SPICE_AUDIO_DATA_MODE_RAW &&
> > -        playbacke_mode->mode != SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
> > +    if (playbacke_mode->mode != SPICE_AUDIO_DATA_MODE_RAW
> > +#if HAVE_CELT051
> > +        && playbacke_mode->mode != SPICE_AUDIO_DATA_MODE_CELT_0_5_1
> > +#endif
> > +       ) {
> >          THROW("invalid mode");
> >      }
> >  
> > @@ -265,9 +276,6 @@ void PlaybackChannel::handle_start(RedPeer::InMessage* message)
> >      start_wave();
> >  #endif
> >      if (!_wave_player) {
> > -        // for now support only one setting
> > -        int celt_mode_err;
> > -
> >          if (start->format != SPICE_AUDIO_FMT_S16) {
> >              THROW("unexpected format");
> >          }
> > @@ -284,6 +292,10 @@ void PlaybackChannel::handle_start(RedPeer::InMessage* message)
> >              return;
> >          }
> >  
> > +#if HAVE_CELT051
> > +        // for now support only one setting
> > +        int celt_mode_err;
> > +
> >          if (!(_celt_mode = celt051_mode_create(start->frequency, start->channels,
> >                                                 frame_size, &celt_mode_err))) {
> >              THROW("create celt mode failed %d", celt_mode_err);
> > @@ -292,6 +304,7 @@ void PlaybackChannel::handle_start(RedPeer::InMessage* message)
> >          if (!(_celt_decoder = celt051_decoder_create(_celt_mode))) {
> >              THROW("create celt decoder");
> >          }
> > +#endif
> >      }
> >      _playing = true;
> >      _frame_count = 0;
> > @@ -333,6 +346,7 @@ void PlaybackChannel::handle_raw_data(RedPeer::InMessage* message)
> >      _wave_player->write(data);
> >  }
> >  
> > +#if HAVE_CELT051
> >  void PlaybackChannel::handle_celt_data(RedPeer::InMessage* message)
> >  {
> >      SpiceMsgPlaybackPacket* packet = (SpiceMsgPlaybackPacket*)message->data();
> > @@ -352,6 +366,7 @@ void PlaybackChannel::handle_celt_data(RedPeer::InMessage* message)
> >      }
> >      _wave_player->write((uint8_t *)pcm);
> >  }
> > +#endif
> >  
> >  class PlaybackFactory: public ChannelFactory {
> >  public:
> > diff --git a/client/record_channel.cpp b/client/record_channel.cpp
> > index d9332c6..dbf8344 100644
> > --- a/client/record_channel.cpp
> > +++ b/client/record_channel.cpp
> > @@ -72,8 +72,10 @@ RecordChannel::RecordChannel(RedClient& client, uint32_t id)
> >      : RedChannel(client, SPICE_CHANNEL_RECORD, id, new RecordHandler(*this))
> >      , _wave_recorder (NULL)
> >      , _mode (SPICE_AUDIO_DATA_MODE_INVALID)
> > +#if HAVE_CELT051
> >      , _celt_mode (NULL)
> >      , _celt_encoder (NULL)
> > +#endif
> >  {
> >      for (int i = 0; i < NUM_SAMPLES_MESSAGES; i++) {
> >          _messages.push_front(new RecordSamplesMessage(*this));
> > @@ -142,7 +144,11 @@ void RecordChannel::handle_start(RedPeer::InMessage* message)
> >  
> >      handler->set_handler(SPICE_MSG_RECORD_START, NULL);
> >      handler->set_handler(SPICE_MSG_RECORD_STOP, &RecordChannel::handle_stop);
> > +#if HAVE_CELT051
> >      ASSERT(!_wave_recorder && !_celt_mode && !_celt_encoder);
> > +#else
> > +    ASSERT(!_wave_recorder);
> > +#endif
> >  
> >      // for now support only one setting
> >      if (start->format != SPICE_AUDIO_FMT_S16) {
> > @@ -160,8 +166,9 @@ void RecordChannel::handle_start(RedPeer::InMessage* message)
> >      }
> >  
> >      int frame_size = 256;
> > -    int celt_mode_err;
> >      _frame_bytes = frame_size * bits_per_sample * start->channels / 8;
> > +#if HAVE_CELT051
> > +    int celt_mode_err;
> >      if (!(_celt_mode = celt051_mode_create(start->frequency, start->channels, frame_size,
> >                                             &celt_mode_err))) {
> >          THROW("create celt mode failed %d", celt_mode_err);
> > @@ -170,6 +177,7 @@ void RecordChannel::handle_start(RedPeer::InMessage* message)
> >      if (!(_celt_encoder = celt051_encoder_create(_celt_mode))) {
> >          THROW("create celt encoder failed");
> >      }
> > +#endif
> >  
> >      send_start_mark();
> >      _wave_recorder->start();
> > @@ -182,6 +190,7 @@ void RecordChannel::clear()
> >          delete _wave_recorder;
> >          _wave_recorder = NULL;
> >      }
> > +#if HAVE_CELT051
> >      if (_celt_encoder) {
> >          celt051_encoder_destroy(_celt_encoder);
> >          _celt_encoder = NULL;
> > @@ -190,6 +199,7 @@ void RecordChannel::clear()
> >          celt051_mode_destroy(_celt_mode);
> >          _celt_mode = NULL;
> >      }
> > +#endif
> >  }
> >  
> >  void RecordChannel::handle_stop(RedPeer::InMessage* message)
> > @@ -200,7 +210,9 @@ void RecordChannel::handle_stop(RedPeer::InMessage* message)
> >      if (!_wave_recorder) {
> >          return;
> >      }
> > +#if HAVE_CELT051
> >      ASSERT(_celt_mode && _celt_encoder);
> > +#endif
> >      clear();
> >  }
> >  
> > @@ -254,8 +266,9 @@ void RecordChannel::push_frame(uint8_t *frame)
> >          DBG(0, "blocked");
> >          return;
> >      }
> > -    uint8_t celt_buf[CELT_COMPRESSED_FRAME_BYTES];
> >      int n;
> > +#if HAVE_CELT051
> > +    uint8_t celt_buf[CELT_COMPRESSED_FRAME_BYTES];
> >  
> >      if (_mode == SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
> >          n = celt051_encode(_celt_encoder, (celt_int16_t *)frame, NULL, celt_buf,
> > @@ -264,7 +277,9 @@ void RecordChannel::push_frame(uint8_t *frame)
> >              THROW("celt encode failed");
> >          }
> >          frame = celt_buf;
> > -    } else {
> > +    } else
> > +#endif
> > +    {
> >          n = _frame_bytes;
> >      }
> >      RedPeer::OutMessage& peer_message = message->peer_message();
> > diff --git a/configure.ac b/configure.ac
> > index 66f9d12..21bd326 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -125,6 +125,9 @@ AM_CONDITIONAL(SUPPORT_SMARTCARD, test "x$enable_smartcard" != "xno")
> >  if test "x$enable_smartcard" = "xyes"; then
> >     AC_DEFINE([USE_SMARTCARD], [1], [Define if supporting smartcard proxying])
> >  fi
> > +AC_ARG_ENABLE(celt051,
> > +[  --disable-celt051       Disable celt051 audio codec (enabled by default)],,
> > +[enable_celt051="yes"])
> >  
> >  AC_ARG_ENABLE(client,
> >  [  --enable-client         Enable spice client],,
> > @@ -220,11 +223,14 @@ AC_SUBST(PIXMAN_CFLAGS)
> >  AC_SUBST(PIXMAN_LIBS)
> >  SPICE_REQUIRES+=" pixman-1 >= 0.17.7"
> >  
> > -PKG_CHECK_MODULES(CELT051, celt051 >= 0.5.1.1)
> > -AC_SUBST(CELT051_CFLAGS)
> > -AC_SUBST(CELT051_LIBS)
> > -AC_SUBST(CELT051_LIBDIR)
> > -SPICE_REQUIRES+=" celt051 >= 0.5.1.1"
> > +if test "x$enable_celt051" = "xyes"; then
> > +    PKG_CHECK_MODULES(CELT051, celt051 >= 0.5.1.1)
> > +    SPICE_REQUIRES+=" celt051 >= 0.5.1.1"
> > +    AC_DEFINE([HAVE_CELT051], 1, [Define if we have celt051 codec])
> > +    AC_SUBST(CELT051_CFLAGS)
> > +    AC_SUBST(CELT051_LIBS)
> > +    AC_SUBST(CELT051_LIBDIR)
> > +fi
> >  
> >  if test ! -e client/generated_marshallers.cpp; then
> >  AC_MSG_CHECKING([for pyparsing python module])
> > diff --git a/server/snd_worker.c b/server/snd_worker.c
> > index 3599c6f..f0588ad 100644
> > --- a/server/snd_worker.c
> > +++ b/server/snd_worker.c
> > @@ -25,7 +25,9 @@
> >  #include <sys/socket.h>
> >  #include <netinet/ip.h>
> >  #include <netinet/tcp.h>
> > +#if HAVE_CELT051
> >  #include <celt051/celt.h>
> > +#endif
> >  
> >  #include "common/marshaller.h"
> >  #include "common/generated_server_marshallers.h"
> > @@ -136,12 +138,14 @@ typedef struct PlaybackChannel {
> >      AudioFrame *free_frames;
> >      AudioFrame *in_progress;
> >      AudioFrame *pending_frame;
> > +    uint32_t mode;
> > +#if HAVE_CELT051
> >      CELTMode *celt_mode;
> >      CELTEncoder *celt_encoder;
> > -    uint32_t mode;
> >      struct {
> >          uint8_t celt_buf[CELT_COMPRESSED_FRAME_BYTES];
> >      } send_data;
> > +#endif
> >  } PlaybackChannel;
> >  
> >  struct SndWorker {
> > @@ -187,13 +191,21 @@ typedef struct RecordChannel {
> >      uint32_t mode;
> >      uint32_t mode_time;
> >      uint32_t start_time;
> > +#if HAVE_CELT051
> >      CELTDecoder *celt_decoder;
> >      CELTMode *celt_mode;
> >      uint32_t celt_buf[FRAME_SIZE];
> > +#endif
> >  } RecordChannel;
> >  
> >  static SndWorker *workers;
> > -static uint32_t playback_compression = SPICE_AUDIO_DATA_MODE_CELT_0_5_1;
> > +static uint32_t playback_compression =
> > +#if HAVE_CELT051
> > +    SPICE_AUDIO_DATA_MODE_CELT_0_5_1
> > +#else
> > +    SPICE_AUDIO_DATA_MODE_RAW
> > +#endif
> > +    ;
> >  
> >  static void snd_receive(void* data);
> >  
> > @@ -322,6 +334,7 @@ static int snd_record_handle_write(RecordChannel *record_channel, size_t size, v
> >      packet = (SpiceMsgcRecordPacket *)message;
> >      size = packet->data_size;
> >  
> > +#if HAVE_CELT051
> >      if (record_channel->mode == SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
> >          int celt_err = celt051_decode(record_channel->celt_decoder, packet->data, size,
> >                                        (celt_int16_t *)record_channel->celt_buf);
> > @@ -331,7 +344,9 @@ static int snd_record_handle_write(RecordChannel *record_channel, size_t size, v
> >          }
> >          data = record_channel->celt_buf;
> >          size = FRAME_SIZE;
> > -    } else if (record_channel->mode == SPICE_AUDIO_DATA_MODE_RAW) {
> > +    } else
> > +#endif
> > +    if (record_channel->mode == SPICE_AUDIO_DATA_MODE_RAW) {
> >          data = (uint32_t *)packet->data;
> >          size = size >> 2;
> >          size = MIN(size, RECORD_SAMPLES_SIZE);
> > @@ -386,8 +401,11 @@ static int snd_record_handle_message(SndChannel *channel, size_t size, uint32_t
> >          SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
> >          record_channel->mode = mode->mode;
> >          record_channel->mode_time = mode->time;
> > -        if (record_channel->mode != SPICE_AUDIO_DATA_MODE_CELT_0_5_1 &&
> > -                                                  record_channel->mode != SPICE_AUDIO_DATA_MODE_RAW) {
> > +        if (record_channel->mode != SPICE_AUDIO_DATA_MODE_RAW
> > +#if HAVE_CELT051
> > +            && record_channel->mode != SPICE_AUDIO_DATA_MODE_CELT_0_5_1
> > +#endif
> > +            ) {
> >              spice_printerr("unsupported mode");
> >          }
> >          break;
> > @@ -779,6 +797,7 @@ static int snd_playback_send_write(PlaybackChannel *playback_channel)
> >  
> >      spice_marshall_msg_playback_data(channel->send_data.marshaller, &msg);
> >  
> > +#if HAVE_CELT051
> >      if (playback_channel->mode == SPICE_AUDIO_DATA_MODE_CELT_0_5_1) {
> >          int n = celt051_encode(playback_channel->celt_encoder, (celt_int16_t *)frame->samples, NULL,
> >                                 playback_channel->send_data.celt_buf, CELT_COMPRESSED_FRAME_BYTES);
> > @@ -789,7 +808,9 @@ static int snd_playback_send_write(PlaybackChannel *playback_channel)
> >          }
> >          spice_marshaller_add_ref(channel->send_data.marshaller,
> >                                   playback_channel->send_data.celt_buf, n);
> > -    } else {
> > +    } else
> > +#endif
> > +    {
> >          spice_marshaller_add_ref(channel->send_data.marshaller,
> >                                   (uint8_t *)frame->samples, sizeof(frame->samples));
> >      }
> > @@ -1157,8 +1178,10 @@ static void snd_playback_cleanup(SndChannel *channel)
> >          reds_enable_mm_timer();
> >      }
> >  
> > +#if HAVE_CELT051
> >      celt051_encoder_destroy(playback_channel->celt_encoder);
> >      celt051_mode_destroy(playback_channel->celt_mode);
> > +#endif
> >  }
> >  
> >  static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsStream *stream,
> > @@ -1168,13 +1191,13 @@ static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
> >      SndWorker *worker = channel->data;
> >      PlaybackChannel *playback_channel;
> >      SpicePlaybackState *st = SPICE_CONTAINEROF(worker, SpicePlaybackState, worker);
> > -    CELTEncoder *celt_encoder;
> > -    CELTMode *celt_mode;
> > -    int celt_error;
> > -    RedChannelClient *rcc;
> >  
> >      snd_disconnect_channel(worker->connection);
> >  
> > +#if HAVE_CELT051
> > +    CELTEncoder *celt_encoder;
> > +    CELTMode *celt_mode;
> > +    int celt_error;
> >      if (!(celt_mode = celt051_mode_create(SPICE_INTERFACE_PLAYBACK_FREQ,
> >                                            SPICE_INTERFACE_PLAYBACK_CHAN,
> >                                            FRAME_SIZE, &celt_error))) {
> > @@ -1186,6 +1209,7 @@ static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
> >          spice_printerr("create celt encoder failed");
> >          goto error_1;
> >      }
> > +#endif
> >  
> >      if (!(playback_channel = (PlaybackChannel *)__new_channel(worker,
> >                                                                sizeof(*playback_channel),
> > @@ -1202,16 +1226,20 @@ static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
> >          goto error_2;
> >      }
> >      worker->connection = &playback_channel->base;
> > -    rcc = playback_channel->base.channel_client;
> >      snd_playback_free_frame(playback_channel, &playback_channel->frames[0]);
> >      snd_playback_free_frame(playback_channel, &playback_channel->frames[1]);
> >      snd_playback_free_frame(playback_channel, &playback_channel->frames[2]);
> >  
> > +#if HAVE_CELT051
> >      playback_channel->celt_mode = celt_mode;
> >      playback_channel->celt_encoder = celt_encoder;
> > -    playback_channel->mode = red_channel_client_test_remote_cap(rcc,
> > -                                                                SPICE_PLAYBACK_CAP_CELT_0_5_1) ?
> > +    playback_channel->mode =
> > +       red_channel_client_test_remote_cap(playback_channel->base.channel_client,
> > +                                          SPICE_PLAYBACK_CAP_CELT_0_5_1) ?
> >          playback_compression : SPICE_AUDIO_DATA_MODE_RAW;
> > +#else
> > +    playback_channel->mode = SPICE_AUDIO_DATA_MODE_RAW;
> > +#endif
> >  
> >      on_new_playback_channel(worker);
> >      if (worker->active) {
> > @@ -1221,10 +1249,13 @@ static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
> >      return;
> >  
> >  error_2:
> > +#if HAVE_CELT051
> >      celt051_encoder_destroy(celt_encoder);
> >  
> >  error_1:
> >      celt051_mode_destroy(celt_mode);
> > +#endif
> > +    return;
> >  }
> >  
> >  static void snd_record_migrate_channel_client(RedChannelClient *rcc)
> > @@ -1365,10 +1396,12 @@ static void on_new_record_channel(SndWorker *worker)
> >  
> >  static void snd_record_cleanup(SndChannel *channel)
> >  {
> > +#if HAVE_CELT051
> >      RecordChannel *record_channel = SPICE_CONTAINEROF(channel, RecordChannel, base);
> >  
> >      celt051_decoder_destroy(record_channel->celt_decoder);
> >      celt051_mode_destroy(record_channel->celt_mode);
> > +#endif
> >  }
> >  
> >  static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStream *stream,
> > @@ -1378,12 +1411,13 @@ static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
> >      SndWorker *worker = channel->data;
> >      RecordChannel *record_channel;
> >      SpiceRecordState *st = SPICE_CONTAINEROF(worker, SpiceRecordState, worker);
> > -    CELTDecoder *celt_decoder;
> > -    CELTMode *celt_mode;
> > -    int celt_error;
> >  
> >      snd_disconnect_channel(worker->connection);
> >  
> > +#if HAVE_CELT051
> > +    CELTDecoder *celt_decoder;
> > +    CELTMode *celt_mode;
> > +    int celt_error;
> >      if (!(celt_mode = celt051_mode_create(SPICE_INTERFACE_RECORD_FREQ,
> >                                            SPICE_INTERFACE_RECORD_CHAN,
> >                                            FRAME_SIZE, &celt_error))) {
> > @@ -1395,6 +1429,7 @@ static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
> >          spice_printerr("create celt decoder failed");
> >          goto error_1;
> >      }
> > +#endif
> >  
> >      if (!(record_channel = (RecordChannel *)__new_channel(worker,
> >                                                            sizeof(*record_channel),
> > @@ -1413,8 +1448,10 @@ static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
> >  
> >      worker->connection = &record_channel->base;
> >  
> > +#if HAVE_CELT051
> >      record_channel->celt_mode = celt_mode;
> >      record_channel->celt_decoder = celt_decoder;
> > +#endif
> >  
> >      on_new_record_channel(worker);
> >      if (worker->active) {
> > @@ -1424,10 +1461,13 @@ static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
> >      return;
> >  
> >  error_2:
> > +#if HAVE_CELT051
> >      celt051_decoder_destroy(celt_decoder);
> >  
> >  error_1:
> >      celt051_mode_destroy(celt_mode);
> > +#endif
> > +    return;
> >  }
> >  
> >  static void snd_playback_migrate_channel_client(RedChannelClient *rcc)
> > @@ -1483,7 +1523,9 @@ void snd_attach_playback(SpicePlaybackInstance *sin)
> >      client_cbs.migrate = snd_playback_migrate_channel_client;
> >      red_channel_register_client_cbs(channel, &client_cbs);
> >      red_channel_set_data(channel, playback_worker);
> > +#if HAVE_CELT051
> >      red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_CELT_0_5_1);
> > +#endif
> >      red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_VOLUME);
> >  
> >      playback_worker->base_channel = channel;
> > @@ -1510,7 +1552,9 @@ void snd_attach_record(SpiceRecordInstance *sin)
> >      client_cbs.migrate = snd_record_migrate_channel_client;
> >      red_channel_register_client_cbs(channel, &client_cbs);
> >      red_channel_set_data(channel, record_worker);
> > +#if HAVE_CELT051
> >      red_channel_set_cap(channel, SPICE_RECORD_CAP_CELT_0_5_1);
> > +#endif
> >      red_channel_set_cap(channel, SPICE_RECORD_CAP_VOLUME);
> >  
> >      record_worker->base_channel = channel;
> > @@ -1557,7 +1601,11 @@ void snd_set_playback_compression(int on)
> >  {
> >      SndWorker *now = workers;
> >  
> > -    playback_compression = on ? SPICE_AUDIO_DATA_MODE_CELT_0_5_1 : SPICE_AUDIO_DATA_MODE_RAW;
> > +    playback_compression =
> > +#if HAVE_CELT051
> > +       on ? SPICE_AUDIO_DATA_MODE_CELT_0_5_1 :
> > +#endif
> > +       SPICE_AUDIO_DATA_MODE_RAW;
> >      for (; now; now = now->next) {
> >          if (now->base_channel->type == SPICE_CHANNEL_PLAYBACK && now->connection) {
> >              SndChannel* sndchannel = now->connection;
> > -- 
> > 1.7.10
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel



> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On 04.06.2012 16:28, Alon Levy wrote:
> On Mon, Jun 04, 2012 at 11:33:22AM +0200, Christophe Fergeau wrote:
>> Makes sense to me though I've only looked quickly through it. Since it
>> works for you, I'm in favour of committing it if noone disagrees.
> 
> I agree. Would you do a review and ack?

Please don't apply it without at least minimal testing.  I know
right to nothing about spice and celt, the changes I made are
purely mechanical, without understanding what's going on in a
"big picture" (I ofcourse tried to understand the "small picture",
the code I touched).  And I since I don't know how it works, I'm
not sure I can perform any reasonable testing.  For one, I'm not
even sure I actually used spice way to transmit sound, -- it
looked that way, but I'm not sure.  Just a minimal testing is
very welcome.

Thank you!

/mjt
On Sat, 2012-06-02 at 15:46 +0400, Michael Tokarev wrote:

> I plan to use this patch in the upcoming Debian
> release, codename wheezy, to get rid of celt
> codec library there, since we decided celt051 is
> not going to be included, but it is obviously not
> a good idea to drop spice entirely.

Isn't it better to drop spice completely rather than shipping a version
thats essentially protocol incompatible? (Well, it will fall back to no
audio or non-compressed audio, but that is untested and pretty bad for
actual use of spice.) 

Then users that want to use spice can get a working version somewhere
else instead of just thinking that spice sucks because of this change.
On 12.06.2012 12:48, Alexander Larsson wrote:
> On Sat, 2012-06-02 at 15:46 +0400, Michael Tokarev wrote:
> 
>> I plan to use this patch in the upcoming Debian
>> release, codename wheezy, to get rid of celt
>> codec library there, since we decided celt051 is
>> not going to be included, but it is obviously not
>> a good idea to drop spice entirely.
> 
> Isn't it better to drop spice completely rather than shipping a version
> thats essentially protocol incompatible? (Well, it will fall back to no
> audio or non-compressed audio, but that is untested and pretty bad for
> actual use of spice.) 

It isn't incompatible.  It might be incompatible with older releases
of spice where audio codec negotiation/fallback were not properly
implemented (read: was buggy), but it is okay now.  Did you read
the whole my email where I mention testing I performed?

> Then users that want to use spice can get a working version somewhere
> else instead of just thinking that spice sucks because of this change.

Thanks,

/mjt
On Tue, 2012-06-12 at 12:54 +0400, Michael Tokarev wrote:
> On 12.06.2012 12:48, Alexander Larsson wrote:
> > On Sat, 2012-06-02 at 15:46 +0400, Michael Tokarev wrote:
> > 
> >> I plan to use this patch in the upcoming Debian
> >> release, codename wheezy, to get rid of celt
> >> codec library there, since we decided celt051 is
> >> not going to be included, but it is obviously not
> >> a good idea to drop spice entirely.
> > 
> > Isn't it better to drop spice completely rather than shipping a version
> > thats essentially protocol incompatible? (Well, it will fall back to no
> > audio or non-compressed audio, but that is untested and pretty bad for
> > actual use of spice.) 
> 
> It isn't incompatible.  It might be incompatible with older releases
> of spice where audio codec negotiation/fallback were not properly
> implemented (read: was buggy), but it is okay now.  Did you read
> the whole my email where I mention testing I performed?

Its not incompatible, like I i said in my "(Well, .." part above. But
what it means is that it will send audio as PCM instead of compressing
it. This is much larger, and will cause anyone using the debian spice
client to connect to a spice server (even one that supports celt) to use
much more bandwidth due to this. Its not very obvious that the reason
for this is that Debian decided to package Spice that way, but rather
the likely conclusion that any user would draw is that Spice just works
like that. Thats not exactly a good impression for Spice.
On Mon, Jun 4, 2012 at 11:33 AM, Christophe Fergeau <cfergeau@redhat.com> wrote:
> Makes sense to me though I've only looked quickly through it. Since it
> works for you, I'm in favour of committing it if noone disagrees.

I also don't like these changes to end up upstream, for the same
reasons Alex mentioned. It makes me sad that Spice will end up with
debian without Celt. They should disable audio completely if it's the
case. I would be more in favour of a patch --disable-audio instead.
On 12.06.2012 13:15, Alexander Larsson wrote:
[]
> Its not incompatible, like I i said in my "(Well, .." part above. But
> what it means is that it will send audio as PCM instead of compressing
> it. This is much larger, and will cause anyone using the debian spice
> client to connect to a spice server (even one that supports celt) to use
> much more bandwidth due to this. Its not very obvious that the reason

Why do you think it is really that bad?  When used in a LAN, bandwidth
doesn't matter, and it will even work a tiny bit better due to less
cpu usage for encoding/decoding.  It may matter for slow/long Internet
links, but these are much less important for audio.  Also, audio does
not come alone usually (except of a few windows sounds), it usually
comes together with video (ie, movies etc), which has their own
requiriments which are much stronger than even raw audio.

I'm not saying celt or other codec is bad or not needed, something
is definitely worth to have to be able to transmit audio cheaply,
but it is not the first priority thing for sure.

Much more important is to have good visual expirence, and this is what
spice is all about, and this is something what is not changed by this
patch.

Do you not agree?

> for this is that Debian decided to package Spice that way, but rather
> the likely conclusion that any user would draw is that Spice just works
> like that. Thats not exactly a good impression for Spice.

Thanks,

/mjt
On 12.06.2012 14:27, Marc-André Lureau wrote:
> On Mon, Jun 4, 2012 at 11:33 AM, Christophe Fergeau <cfergeau@redhat.com> wrote:
>> Makes sense to me though I've only looked quickly through it. Since it
>> works for you, I'm in favour of committing it if noone disagrees.
> 
> I also don't like these changes to end up upstream, for the same
> reasons Alex mentioned. It makes me sad that Spice will end up with
> debian without Celt.

So let's decide.  Without spice, which means that qemu/kvm, which
is a primary user of spice, does not support spice in debian, and
which means there's no spice clients in debian (which is less of
a problem).  Or without celt.

(Mind you, it wasn't me who decided that celt is somehow bad to
have in Debian, and I myself does not understand the issues 100%.
One of the issues, iirc, is that celt does not compile/work on
anything but x86).

I already replied to email by Christophe, asking why he thinks
raw audio is that bad.  The same question pops again.

For the upstream, and I already mentioned this, the patch
might help to identify all places which should be touched
if a need to add a new codec emerges.

>    They should disable audio completely if it's the
> case. I would be more in favour of a patch --disable-audio instead.

It was one of alternatives.  For the reasons I already mentioned
(raw pcm isn't bad), making celt optional has been choosen, as
it appears to be less restrictive for the user.

Thanks,

/mjt
On Tue, 2012-06-12 at 14:31 +0400, Michael Tokarev wrote:
> On 12.06.2012 13:15, Alexander Larsson wrote:
> []
> > Its not incompatible, like I i said in my "(Well, .." part above. But
> > what it means is that it will send audio as PCM instead of compressing
> > it. This is much larger, and will cause anyone using the debian spice
> > client to connect to a spice server (even one that supports celt) to use
> > much more bandwidth due to this. Its not very obvious that the reason
> 
> Why do you think it is really that bad?  When used in a LAN, bandwidth
> doesn't matter, and it will even work a tiny bit better due to less
> cpu usage for encoding/decoding.  It may matter for slow/long Internet
> links, but these are much less important for audio.  Also, audio does
> not come alone usually (except of a few windows sounds), it usually
> comes together with video (ie, movies etc), which has their own
> requiriments which are much stronger than even raw audio.
> 
> I'm not saying celt or other codec is bad or not needed, something
> is definitely worth to have to be able to transmit audio cheaply,
> but it is not the first priority thing for sure.
> 
> Much more important is to have good visual expirence, and this is what
> spice is all about, and this is something what is not changed by this
> patch.
> 
> Do you not agree?

I don't really agree. Its true that if you have unlimited bandwidth,
then bandwidth doesn't matter. But I don't think in practice this is
true, as bandwidth always has some limits in reality. Its possible that
it works fine in some situations, but its also quite possible that its a
deal-breaker in some others, and we have no data to substantiate the
claim that it doesn't matter (in fact, it seems unlikely since the
developers spent time and energy to implement audio compression).

Spice is very much about more than just pixels, that it does more than
graphics (usb, sound, audio-video sync, clipboard, etc) is an important
part of why Spice is better than other solutions. In fact, our support
of movies by using compressed video and audio is a pretty important
selling point of spice.

The main thing I disagree with this change is that you change how spice
works, making act differently than its creators intended, due to a
packaging technicallity. You decide that spice users are not interested
in low-bandwidh audio, without asking and without telling them. (I'm
sure it'll be mentioned somewhere, but its unlikely that most users will
see it.)
On 12.06.2012 15:35, Alexander Larsson wrote:
> On Tue, 2012-06-12 at 14:31 +0400, Michael Tokarev wrote:
>> On 12.06.2012 13:15, Alexander Larsson wrote:
>> []
>>> Its not incompatible, like I i said in my "(Well, .." part above. But
>>> what it means is that it will send audio as PCM instead of compressing
>>> it. This is much larger, and will cause anyone using the debian spice
>>> client to connect to a spice server (even one that supports celt) to use
>>> much more bandwidth due to this. Its not very obvious that the reason
>>
>> Why do you think it is really that bad?  When used in a LAN, bandwidth
>> doesn't matter, and it will even work a tiny bit better due to less
>> cpu usage for encoding/decoding.  It may matter for slow/long Internet
>> links, but these are much less important for audio.  Also, audio does
>> not come alone usually (except of a few windows sounds), it usually
>> comes together with video (ie, movies etc), which has their own
>> requiriments which are much stronger than even raw audio.
>>
>> I'm not saying celt or other codec is bad or not needed, something
>> is definitely worth to have to be able to transmit audio cheaply,
>> but it is not the first priority thing for sure.
>>
>> Much more important is to have good visual expirence, and this is what
>> spice is all about, and this is something what is not changed by this
>> patch.
>>
>> Do you not agree?
> 
> I don't really agree. Its true that if you have unlimited bandwidth,
> then bandwidth doesn't matter. But I don't think in practice this is
> true, as bandwidth always has some limits in reality. Its possible that

Yes, in practice there's always some limits.  For "standard" CD-quality
audio on a LAN, -- which I mentioned above -- this limit is more in
"unlimited" category (since PCM/RAW stream is small even for 10Mbps
network, but such networks don't exists on LANs anymore).

And yes, when you connect to it over 'net, esp. over long-distance links,
that limit becomes much more limiting.

> it works fine in some situations, but its also quite possible that its a
> deal-breaker in some others, and we have no data to substantiate the
> claim that it doesn't matter (in fact, it seems unlikely since the
> developers spent time and energy to implement audio compression).

Yet again, I didn't say it does not matter, I especially mentioned
this in previous email.

> Spice is very much about more than just pixels, that it does more than
> graphics (usb, sound, audio-video sync, clipboard, etc) is an important
> part of why Spice is better than other solutions. In fact, our support
> of movies by using compressed video and audio is a pretty important
> selling point of spice.
> 
> The main thing I disagree with this change is that you change how spice
> works, making act differently than its creators intended, due to a
> packaging technicallity. You decide that spice users are not interested
> in low-bandwidh audio, without asking and without telling them. (I'm
> sure it'll be mentioned somewhere, but its unlikely that most users will
> see it.)

Well.  Alternative I have is to not provide spice in Debian at all.  This
includes qemu/kvm packages shipping without spice support, so even if you
add spice client built from sources, it wont help.  This includes such
things like xspice.

Does this work better for you?  For users?

Thanks,

/mjt
On Tue, Jun 12, 2012 at 03:47:09PM +0400, Michael Tokarev wrote:
> On 12.06.2012 15:35, Alexander Larsson wrote:
> > Spice is very much about more than just pixels, that it does more than
> > graphics (usb, sound, audio-video sync, clipboard, etc) is an important
> > part of why Spice is better than other solutions. In fact, our support
> > of movies by using compressed video and audio is a pretty important
> > selling point of spice.
> > 
> > The main thing I disagree with this change is that you change how spice
> > works, making act differently than its creators intended, due to a
> > packaging technicallity. You decide that spice users are not interested
> > in low-bandwidh audio, without asking and without telling them. (I'm
> > sure it'll be mentioned somewhere, but its unlikely that most users will
> > see it.)
> 
> Well.  Alternative I have is to not provide spice in Debian at all.  This
> includes qemu/kvm packages shipping without spice support, so even if you
> add spice client built from sources, it wont help.  This includes such
> things like xspice.
> 
> Does this work better for you?  For users?

Neither scenario is good for users. We need to focus on addressing why
Celt051 can't be included in Debian, not arguing over which terrible
alternative is least-bad for users.

Daniel
----- Original Message -----
> From: "Daniel P. Berrange" <berrange@redhat.com>
> To: "Michael Tokarev" <mjt@tls.msk.ru>
> Cc: spice-devel@lists.freedesktop.org, "Ron Lee" <ron@debian.org>
> Sent: Tuesday, June 12, 2012 1:52:48 PM
> Subject: Re: [Spice-devel] [PATCH] make celt to be optional
> 
> On Tue, Jun 12, 2012 at 03:47:09PM +0400, Michael Tokarev wrote:
> > On 12.06.2012 15:35, Alexander Larsson wrote:
> > > Spice is very much about more than just pixels, that it does more
> > > than
> > > graphics (usb, sound, audio-video sync, clipboard, etc) is an
> > > important
> > > part of why Spice is better than other solutions. In fact, our
> > > support
> > > of movies by using compressed video and audio is a pretty
> > > important
> > > selling point of spice.
> > > 
> > > The main thing I disagree with this change is that you change how
> > > spice
> > > works, making act differently than its creators intended, due to
> > > a
> > > packaging technicallity. You decide that spice users are not
> > > interested
> > > in low-bandwidh audio, without asking and without telling them.
> > > (I'm
> > > sure it'll be mentioned somewhere, but its unlikely that most
> > > users will
> > > see it.)
> > 
> > Well.  Alternative I have is to not provide spice in Debian at all.
> >  This
> > includes qemu/kvm packages shipping without spice support, so even
> > if you
> > add spice client built from sources, it wont help.  This includes
> > such
> > things like xspice.
> > 
> > Does this work better for you?  For users?
> 
> Neither scenario is good for users. We need to focus on addressing
> why
> Celt051 can't be included in Debian, not arguing over which terrible
> alternative is least-bad for users.
+1

and with all respect It's been known for some time celt would not make it into debian, Moreover Michael made some effort to get Spice (even though with the limitation) into debian (which benn known for some time as well). Let's be honest video/audio sucks anyway over low-bandwidth lines (and unfortunately we do not even know how much...).
> 
> Daniel
> --
> |: http://berrange.com      -o-
> |   http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-
> |            http://virt-manager.org :|
> |: http://autobuild.org       -o-
> |        http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-
> |      http://live.gnome.org/gtk-vnc :|
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
On Tue, Jun 12, 2012 at 02:44:56PM +0400, Michael Tokarev wrote:
> (Mind you, it wasn't me who decided that celt is somehow bad to
> have in Debian, and I myself does not understand the issues 100%.
> One of the issues, iirc, is that celt does not compile/work on
> anything but x86).

I think the main issue debian has with our spice use is that we require an
old version for compatibility reasons, and debian does not want to ship
this older version just for spice.

> >    They should disable audio completely if it's the
> > case. I would be more in favour of a patch --disable-audio instead.
> 
> It was one of alternatives.  For the reasons I already mentioned
> (raw pcm isn't bad), making celt optional has been choosen, as
> it appears to be less restrictive for the user.

While it's sad that spice won't be able to use celt on debian, it's
probably better than no spice support at all, at least this means things
will work locally/on LAN. This indeed also means people evaluating SPICE
on Debian for WAN use will have worse results than what they could have.
The ideal solution would be to have support for Opus, I think Marc André
and Alon had started to look into that, but that's not done yet :(

Christophe
Hi

----- Mensaje original -----
> The ideal solution would be to have support for Opus, I think Marc
> André
> and Alon had started to look into that, but that's not done yet :(

No, I didn't. Long time ago I proposed we have celt unstable flag, which
would be completely broken in most cases but would also have made debian
folks happy.

As long as the bitstream is not frozen, we can't use opus, or we will
have the same problems as with celt today.

Sadly, I think debian folks aren't looking at what is really celt051.
We could just have renamed it to "spicelt" and they wouldn't even have
said a word I bet.
On Tue, Jun 12, 2012 at 09:11:24AM -0400, Marc-André Lureau wrote:
> As long as the bitstream is not frozen, we can't use opus, or we will
> have the same problems as with celt today.

As I understand it, while the bitstream is not officially frozen yet, it's
very unlikely to change before the real freeze (unless big last minute
issues are fine), so an Opus mode (marked as "no compat guarantees, use at
your own risk, ...") will probably not cause significant compatibilities issues.

> Sadly, I think debian folks aren't looking at what is really celt051.

It's not really an old and unmaintained release of celt ?

Christophe
On 06/12/2012 09:11 AM, Marc-André Lureau wrote:
> Hi
>
> ----- Mensaje original -----
>> The ideal solution would be to have support for Opus, I think Marc
>> André
>> and Alon had started to look into that, but that's not done yet :(
> No, I didn't. Long time ago I proposed we have celt unstable flag, which
> would be completely broken in most cases but would also have made debian
> folks happy.
>
> As long as the bitstream is not frozen, we can't use opus, or we will
> have the same problems as with celt today.
>
> Sadly, I think debian folks aren't looking at what is really celt051.
> We could just have renamed it to "spicelt" and they wouldn't even have
> said a word I bet.
What about a client popup "no audio codecs available.  audio will be 
disabled" if/when the detected bandwidth is below some margin, or just 
completely assuming nothing other than raw PCM is available.  Or a 
warning that the low bandwidth will impact performance and give the user 
the option to disable audio.

A user trying to "fix" their problem would google "debian spice no audio 
codec" and presumably come across this thread ;-) and curse at the 
politics of software development.

Thanks,
David
On Tue, Jun 12, 2012 at 09:11:24AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Mensaje original -----
> > The ideal solution would be to have support for Opus, I think Marc
> > André
> > and Alon had started to look into that, but that's not done yet :(
> 
> No, I didn't. Long time ago I proposed we have celt unstable flag, which
> would be completely broken in most cases but would also have made debian
> folks happy.
> 
> As long as the bitstream is not frozen, we can't use opus, or we will
> have the same problems as with celt today.

I know both Celt & Opus are better codecs, but is it feasible to
perhaps add a Vorbis codec as a further option. It is a widely
available codec which has been bitstream compatible for ages, and
I'd hope it would be better than falling back to raw PCM ? This
would mean we'd not have to rush into Opus or newer Celt while they
are still not declared bitstram stable.

Regards,
Daniel
----- Mensaje original -----
> On Tue, Jun 12, 2012 at 09:11:24AM -0400, Marc-André Lureau wrote:
> > As long as the bitstream is not frozen, we can't use opus, or we
> > will
> > have the same problems as with celt today.
> 
> As I understand it, while the bitstream is not officially frozen yet,
> it's
> very unlikely to change before the real freeze (unless big last
> minute
> issues are fine), so an Opus mode (marked as "no compat guarantees,
> use at
> your own risk, ...") will probably not cause significant
> compatibilities issues.

That's just guesses. It's not about library API, but about bitstream.
There is no guarantee. Sticking to celt051 is still a safer alternative.
Otherwise, how would you identify almost-frozen bitstream to frozen bitstream?
You would have to identify by library version (erk!)
and be compatible with the old and new bitstram (which might be complicated
depending on library design), or be incompatible with the intermediate version,
situation which we better avoid!

> > Sadly, I think debian folks aren't looking at what is really
> > celt051.
> 
> It's not really an old and unmaintained release of celt ?

If something would show up, the spice team would
certainly update it. So I wouldn't say unmaintained.

That's also what I would call a stable release.
On Tue, Jun 12, 2012 at 09:59:39AM -0400, Marc-André Lureau wrote:
> 
> 
> ----- Mensaje original -----
> > On Tue, Jun 12, 2012 at 09:11:24AM -0400, Marc-André Lureau wrote:
> > > As long as the bitstream is not frozen, we can't use opus, or we
> > > will
> > > have the same problems as with celt today.
> >
> > As I understand it, while the bitstream is not officially frozen yet,
> > it's
> > very unlikely to change before the real freeze (unless big last
> > minute
> > issues are fine), so an Opus mode (marked as "no compat guarantees,
> > use at
> > your own risk, ...") will probably not cause significant
> > compatibilities issues.
> 
> That's just guesses. It's not about library API, but about bitstream.
> There is no guarantee.

A guess supported by the slides at http://www.opus-codec.org/presentations/
, by various mailing posts from opus developers, ...

> Sticking to celt051 is still a safer alternative.

Not suggesting dropping celt051 support upstream...

> Otherwise, how would you identify almost-frozen bitstream to frozen bitstream?
> You would have to identify by library version (erk!)
> and be compatible with the old and new bitstram (which might be complicated
> depending on library design), or be incompatible with the intermediate version,
> situation which we better avoid!

We would make no guarantee with interoperability between binaries using
different opus versions until the format is officially frozen. I agree
there's a bit of uncertainty in this move, but I think that at this point
it's a reasonable assumption that things will work, even with different
opus versions.

> 
> > > Sadly, I think debian folks aren't looking at what is really
> > > celt051.
> >
> > It's not really an old and unmaintained release of celt ?
> 
> If something would show up, the spice team would
> certainly update it. So I wouldn't say unmaintained.

Is anyone from the team monitoring celt security issues and whether they
apply to celt 0.5.1?

Christophe
Hi

----- Mensaje original -----
> A guess supported by the slides at
> http://www.opus-codec.org/presentations/
> , by various mailing posts from opus developers, ...
> 

Can you quote the relevant part of the slide?

According to
http://www.opus-codec.org/downloads/

The bit-stream was tentatively frozen several times, and broke from time to time (in 0.9.8 at least).

> > Sticking to celt051 is still a safer alternative.
> 
> Not suggesting dropping celt051 support upstream...

Why do you suggest that?
 
> > Otherwise, how would you identify almost-frozen bitstream to frozen
> > bitstream?
> > You would have to identify by library version (erk!)
> > and be compatible with the old and new bitstram (which might be
> > complicated
> > depending on library design), or be incompatible with the
> > intermediate version,
> > situation which we better avoid!
> 
> We would make no guarantee with interoperability between binaries
> using
> different opus versions until the format is officially frozen. I
> agree
> there's a bit of uncertainty in this move, but I think that at this
> point
> it's a reasonable assumption that things will work, even with
> different
> opus versions.

We don't want to introduce changes that will break in the future, even knowingly, as we want to maintain compatibility. That's the reason why the update celt codec never went upstream. But we can't force people from not forking and do what pleases them. Regarding current Spice upstream, we better stay off those experimental and unstable path.

> > > > Sadly, I think debian folks aren't looking at what is really
> > > > celt051.
> > >
> > > It's not really an old and unmaintained release of celt ?
> > 
> > If something would show up, the spice team would
> > certainly update it. So I wouldn't say unmaintained.
> 
> Is anyone from the team monitoring celt security issues and whether
> they
> apply to celt 0.5.1?

I would be really surprised if nobody is fixing spice celt security issues for RHEL.
On Tue, Jun 12, 2012 at 09:11:24AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Mensaje original -----
> > The ideal solution would be to have support for Opus, I think Marc
> > André
> > and Alon had started to look into that, but that's not done yet :(
> 
> No, I didn't. Long time ago I proposed we have celt unstable flag, which
> would be completely broken in most cases but would also have made debian
> folks happy.
> 
> As long as the bitstream is not frozen, we can't use opus, or we will
> have the same problems as with celt today.

OPUS is frozen. The problem I had when trying to use it was that it uses
a sampling rate of 48000 and we have 44110. That required changes to
qemu/spice interface and I stopped there.

> 
> Sadly, I think debian folks aren't looking at what is really celt051.
> We could just have renamed it to "spicelt" and they wouldn't even have
> said a word I bet.
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Tue, Jun 12, 2012 at 09:11:24AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Mensaje original -----
> > The ideal solution would be to have support for Opus, I think Marc
> > André
> > and Alon had started to look into that, but that's not done yet :(
> 
> No, I didn't. Long time ago I proposed we have celt unstable flag, which
> would be completely broken in most cases but would also have made debian
> folks happy.
> 
> As long as the bitstream is not frozen, we can't use opus, or we will
> have the same problems as with celt today.

Ignore my previous message, it is a draft yet, I remembered wrongly,
sorry for the noise.

> 
> Sadly, I think debian folks aren't looking at what is really celt051.
> We could just have renamed it to "spicelt" and they wouldn't even have
> said a word I bet.
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Tue, Jun 12, 2012 at 10:55:48AM -0400, Marc-André Lureau wrote:
> Can you quote the relevant part of the slide?

See page 20 "Freeze bitstream format" with a check mark to its side
Page 3 too "WGLC (Working Group Last Call) last minor bitstream changes"
There are probably more details about this in the video.

> 
> > > Sticking to celt051 is still a safer alternative.
> > 
> > Not suggesting dropping celt051 support upstream...
> 
> Why do you suggest that?

I probably misinterpreted your "Sticking to celt051..". Of course it's the
best alternative compatibility wise, but the reason for this thread is that
Debian does not want it.

> We don't want to introduce changes that will break in the future, even
> knowingly, as we want to maintain compatibility. That's the reason why
> the update celt codec never went upstream.

As long as we don't drop celt051 support, that Opus can be disabled (it can
be be disabled by default), that our position about
compatibility/interoperability is clear, having support for it would be a
good thing, especially since it would mean a good SPICE in Debian. Maybe
I'm putting too much trust on the Opus people not making regular breaks to
the Opus bitstream format, but I think it's a chance we should take.


> But we can't force people from
> not forking and do what pleases them. Regarding current Spice upstream,
> we better stay off those experimental and unstable path.

Experimental and unstable as in "being standardized by IETF"? Opus is not
final yet, but a this point I hope it's out of the experimental zone and is
stable enough.

> > > If something would show up, the spice team would
> > > certainly update it. So I wouldn't say unmaintained.
> > 
> > Is anyone from the team monitoring celt security issues and whether
> > they
> > apply to celt 0.5.1?
> 
> I would be really surprised if nobody is fixing spice celt security issues for RHEL.

And this would magically trigger a celt 0.5.1 update from the spice team? I
assume you were talking about making new celt .tar.gz available.

Christophe
Hi Marc-André,

On Tue, Jun 12, 2012 at 09:59:39AM -0400, Marc-André Lureau wrote:
> ----- Mensaje original -----
> > On Tue, Jun 12, 2012 at 09:11:24AM -0400, Marc-André Lureau wrote:
> > > As long as the bitstream is not frozen, we can't use opus, or we
> > > will
> > > have the same problems as with celt today.
> >
> > As I understand it, while the bitstream is not officially frozen yet,
> > it's
> > very unlikely to change before the real freeze (unless big last
> > minute
> > issues are fine), so an Opus mode (marked as "no compat guarantees,
> > use at
> > your own risk, ...") will probably not cause significant
> > compatibilities issues.
> 
> That's just guesses. It's not about library API, but about bitstream.
> There is no guarantee. Sticking to celt051 is still a safer alternative.
> Otherwise, how would you identify almost-frozen bitstream to frozen bitstream?
> You would have to identify by library version (erk!)
> and be compatible with the old and new bitstram (which might be complicated
> depending on library design), or be incompatible with the intermediate version,
> situation which we better avoid!

The bitstream has been frozen for just a tad under a year now.
The API is frozen.

The working group and IETF last calls are over.

The IESG telecon has been held and has approved passing this.

The only thing we're effectively waiting on before the RFC is officially
published now is for this to pass through -editors.  And they aren't going
to edit anything that changes the API or bitstream.

Fedora is shipping Opus packages now, as is Debian.

Are there any other guesses that you would like informed answers to ;?


> > > Sadly, I think debian folks aren't looking at what is really
> > > celt051.
> >
> > It's not really an old and unmaintained release of celt ?
> 
> If something would show up, the spice team would
> certainly update it. So I wouldn't say unmaintained.
> 
> That's also what I would call a stable release.

So if say, a later version of celt was found to have issues, which were
fixed in that later version, and which led normally very reliable upstream
developers to suggest that at least some earlier versions may be at risk
of being crashed remotely ...  then what would you do?

Given that nobody is working on celt *at all* anymore, and there are no
simple patches being produced for this that you can Just Apply to it, how
exactly would you plan to "certainly update it"?  Since the only "certain"
update is not bitstream compatible with the celt you currently use.

<hint: this isn't a hypothetical question>

Stable releases have maintainers.  Celt no longer has a maintainer of
any description.  There is an important distinction there.

I'm not suggesting you need to panic and do anything silly.  But the time
to start addressing the real future of this _is_ now.


 Cheers,
 Ron
Hi Alon,

On Tue, Jun 12, 2012 at 06:03:58PM +0300, Alon Levy wrote:
> On Tue, Jun 12, 2012 at 09:11:24AM -0400, Marc-André Lureau wrote:
> > Hi
> > 
> > ----- Mensaje original -----
> > > The ideal solution would be to have support for Opus, I think Marc
> > > André
> > > and Alon had started to look into that, but that's not done yet :(
> > 
> > No, I didn't. Long time ago I proposed we have celt unstable flag, which
> > would be completely broken in most cases but would also have made debian
> > folks happy.
> > 
> > As long as the bitstream is not frozen, we can't use opus, or we will
> > have the same problems as with celt today.
> 
> OPUS is frozen. The problem I had when trying to use it was that it uses
> a sampling rate of 48000 and we have 44110. That required changes to
> qemu/spice interface and I stopped there.

Opus operates in the frequency domain, so it doesn't actually have a
"sampling rate" at all so far as the codec is concerned.  The library
API supports converting that to and from the time domain at 48k or any
integer fraction of that rate down to 8k.  For 'oddball' rates you
simply need to pass the input or output through a resampler, of which
code for many is widely available.   The opusenc and opusdec reference
tools support files at any sampling rate you might dream up or encounter.

Why exactly do you need 44.1?  Many (most?) sound cards don't support
that rate anyway and will require it to be resampled to 48k before
being played on them in any case ...


 Cheers,
 Ron
On Wed, Jun 13, 2012 at 01:22:43AM +0930, Ron wrote:
> 
> Hi Alon,
> 
> On Tue, Jun 12, 2012 at 06:03:58PM +0300, Alon Levy wrote:
> > On Tue, Jun 12, 2012 at 09:11:24AM -0400, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > ----- Mensaje original -----
> > > > The ideal solution would be to have support for Opus, I think Marc
> > > > André
> > > > and Alon had started to look into that, but that's not done yet :(
> > > 
> > > No, I didn't. Long time ago I proposed we have celt unstable flag, which
> > > would be completely broken in most cases but would also have made debian
> > > folks happy.
> > > 
> > > As long as the bitstream is not frozen, we can't use opus, or we will
> > > have the same problems as with celt today.
> > 
> > OPUS is frozen. The problem I had when trying to use it was that it uses
> > a sampling rate of 48000 and we have 44110. That required changes to
> > qemu/spice interface and I stopped there.
> 
> Opus operates in the frequency domain, so it doesn't actually have a
> "sampling rate" at all so far as the codec is concerned.  The library
> API supports converting that to and from the time domain at 48k or any
> integer fraction of that rate down to 8k.  For 'oddball' rates you
> simply need to pass the input or output through a resampler, of which
> code for many is widely available.   The opusenc and opusdec reference
> tools support files at any sampling rate you might dream up or encounter.
> 
> Why exactly do you need 44.1?  Many (most?) sound cards don't support
> that rate anyway and will require it to be resampled to 48k before
> being played on them in any case ...
> 

I wasn't talking about opus internals at all, since I don't understand
them. I was just commenting on where I bumped into a wall when I tried
to update spice to use opus. I'll need to go back and look at my tree,
but I am just speaking of api problems, not about the codec itself,
which is perfectly opaque to me (like celt is right now - spice doesn't
care about the codec).

> 
>  Cheers,
>  Ron
> 
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi

On Tue, Jun 12, 2012 at 5:35 PM, Ron <ron@debian.org> wrote:
> The bitstream has been frozen for just a tad under a year now.

I don't know how you can claim that: http://www.opus-codec.org/downloads/

The bit-stream has not changed, except for some corner cases that are
unlikely to happen in the real world.
[   ] opus-0.9.9.tar.gz                                  18-Feb-2012
16:20  710K

If it is, can you point to an official release note stating it clearly
"bitstream is frozen" or compatible with any further release?

> The API is frozen.
>
> The working group and IETF last calls are over.
>
> The IESG telecon has been held and has approved passing this.
>
> The only thing we're effectively waiting on before the RFC is officially
> published now is for this to pass through -editors.  And they aren't going
> to edit anything that changes the API or bitstream.
>
> Fedora is shipping Opus packages now, as is Debian.

This is not helping, we have implementations not really frozen.

> So if say, a later version of celt was found to have issues, which were
> fixed in that later version, and which led normally very reliable upstream
> developers to suggest that at least some earlier versions may be at risk
> of being crashed remotely ...  then what would you do?
>
> Given that nobody is working on celt *at all* anymore, and there are no
> simple patches being produced for this that you can Just Apply to it, how
> exactly would you plan to "certainly update it"?  Since the only "certain"
> update is not bitstream compatible with the celt you currently use.

We would make a release, just like there has been several 0.5.1 releases:
http://downloads.us.xiph.org/releases/celt/celt-0.5.1.tar.gz vs
http://downloads.us.xiph.org/releases/celt/celt-0.5.1.3.tar.gz

Since no releases happened recently, I assume no security issues are known.
As I said, it would be quite critical for the Spice project to rely on a celt
release with security issues. As long as spice support 0.5.1 (which is likely
for a long time still) there will be new releases or fix when necessary.

> I'm not suggesting you need to panic and do anything silly.  But the time
> to start addressing the real future of this _is_ now.

It will be when there is an official and clear announcement that the opus
library implementation has frozen bit-stream (or any further release will be
bit-stream compatible)
On Tue, Jun 12, 2012 at 06:30:36PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jun 12, 2012 at 5:35 PM, Ron <ron@debian.org> wrote:
> > The bitstream has been frozen for just a tad under a year now.
> 
> I don't know how you can claim that: http://www.opus-codec.org/downloads/
> 
> The bit-stream has not changed, except for some corner cases that are
> unlikely to happen in the real world.
> [   ] opus-0.9.9.tar.gz                                  18-Feb-2012
> 16:20  710K

Which part of "The bit-stream has not changed" is unclear?

The qualifier there I believe is a reference to this commit:

 Fixes the code for optional self-delimited packing to make it fit the draft.
 This has no impact on opus_demo, test vectors, or "normal" codec operation.

Since support for that packing mode is declared to be OPTIONAL in the
standard (their caps not mine :), anything which might use it cannot
expect to actually be interoperable with a compliant decoder anyhow.

If "unlikely to happen in the real world" was not an extremely conservative
estimate of the real impact, then strictly preserving the bitstream would
have won hands down at that point in time, without any doubt at all.

> If it is, can you point to an official release note stating it clearly
> "bitstream is frozen" or compatible with any further release?

More official than the codec working group declaring it to be in its
finally frozen form, or the codec specification being submitted to and
approved by the IESG for publication?

Archives of all those things are public if you don't want to take
my word for it.


> > The API is frozen.
> >
> > The working group and IETF last calls are over.
> >
> > The IESG telecon has been held and has approved passing this.
> >
> > The only thing we're effectively waiting on before the RFC is officially
> > published now is for this to pass through -editors.  And they aren't going
> > to edit anything that changes the API or bitstream.
> >
> > Fedora is shipping Opus packages now, as is Debian.
> 
> This is not helping, we have implementations not really frozen.

Who is "we" here?

I *absolutely* would not have pushed a copy of this to Debian for people
to use if there was anything short of an ASTRONOMICALLY SMALL chance of
there still being a bitstream change.  (my caps this time, I'm serious
about that.  It's not going to happen while the upstream developers
draw breath to stab anybody who might try).

Anything which did change it would no longer be Opus, since the IETF now
has change control over it, and the Last Call for changes is over.


> > So if say, a later version of celt was found to have issues, which were
> > fixed in that later version, and which led normally very reliable upstream
> > developers to suggest that at least some earlier versions may be at risk
> > of being crashed remotely ...  then what would you do?
> >
> > Given that nobody is working on celt *at all* anymore, and there are no
> > simple patches being produced for this that you can Just Apply to it, how
> > exactly would you plan to "certainly update it"?  Since the only "certain"
> > update is not bitstream compatible with the celt you currently use.
> 
> We would make a release, just like there has been several 0.5.1 releases:
> http://downloads.us.xiph.org/releases/celt/celt-0.5.1.tar.gz vs
> http://downloads.us.xiph.org/releases/celt/celt-0.5.1.3.tar.gz

At risk of sounding repetitive, Who is "we" here?

That looks like the download site of the people who are no longer maintaining
celt to me, not evidence of people existing who actually are.

0.5.1.3 was May 2009.  Made just 2 weeks after 0.5.1 to fix some initial
obvious screwups.  It hasn't been touched or looked at by anyone since.

> Since no releases happened recently, I assume no security issues are known.

That's an interesting assumption ...

Here's another, that is actually a little more grounded in what the people
who might actually make those releases have actually been focussed on:

No releases happened because *nobody* is looking at celt 0.5.1 at all, let
alone backporting fixes to it for things found in much later releases.
Issues found in later releases were fixed in later releases, and nobody
spent even a minute looking at backporting those things to 0.5.1.

And now that Opus is finalised, nobody is looking at *any version* of celt
at all outside of what is in Opus.


So if you aren't actively doing that, well, I guess that just leaves the
black hats with motivation to do so ...


> As I said, it would be quite critical for the Spice project to rely on a celt
> release with security issues. As long as spice support 0.5.1 (which is likely
> for a long time still) there will be new releases or fix when necessary.

Well, I just told you there is a reasonable suspicion (from the person who
single-handedly has found more bugs in this code than anyone else in the
world, all put together) ...

What is your plan to confirm or deny 0.5.1 is affected by the problems that
he found and fixed in later versions?

The only thing I know for sure is that nobody else is looking at the 0.5.1
code to assess whether things need backporting.


You could just wait for the news of a live exploit actually doing the rounds,
but I personally wouldn't want to trust my own systems to that plan.


> > I'm not suggesting you need to panic and do anything silly.  But the time
> > to start addressing the real future of this _is_ now.
> 
> It will be when there is an official and clear announcement that the opus
> library implementation has frozen bit-stream (or any further release will be
> bit-stream compatible)

If the IETF closure on this isn't official enough for you, then I don't really
know what to say.

I'm not particularly interested in arguing with someone who has already made
up their mind and for whom facts are just annoying details to handwave away.
But if you're actually interested in the facts, I'm happy to help answer any
questions that you or the rest of the spice team might have.

It's your decision what you do with spice.  But saying "Opus isn't ready yet"
is simply not true.  You may have other reasons not to use it, but that isn't
one of them that stands up to genuine scrutiny.

 Cheers,
 Ron
Hi

On Tue, Jun 12, 2012 at 8:19 PM, Ron <ron@debian.org> wrote:
> On Tue, Jun 12, 2012 at 06:30:36PM +0200, Marc-André Lureau wrote:
>> Hi
>>
>> On Tue, Jun 12, 2012 at 5:35 PM, Ron <ron@debian.org> wrote:
>> > The bitstream has been frozen for just a tad under a year now.
>>
>> I don't know how you can claim that: http://www.opus-codec.org/downloads/
>>
>> The bit-stream has not changed, except for some corner cases that are
>> unlikely to happen in the real world.
>> [   ] opus-0.9.9.tar.gz                                  18-Feb-2012
>> 16:20  710K
>
> Which part of "The bit-stream has not changed" is unclear?
>
> The qualifier there I believe is a reference to this commit:
>
>  Fixes the code for optional self-delimited packing to make it fit the draft.
>  This has no impact on opus_demo, test vectors, or "normal" codec operation.
>
> Since support for that packing mode is declared to be OPTIONAL in the
> standard (their caps not mine :), anything which might use it cannot
> expect to actually be interoperable with a compliant decoder anyhow.
>
> If "unlikely to happen in the real world" was not an extremely conservative
> estimate of the real impact, then strictly preserving the bitstream would
> have won hands down at that point in time, without any doubt at all.
>
>> If it is, can you point to an official release note stating it clearly
>> "bitstream is frozen" or compatible with any further release?
>
> More official than the codec working group declaring it to be in its
> finally frozen form, or the codec specification being submitted to and
> approved by the IESG for publication?

You keep refering to standards, while I am talking about what we can
actually rely on, the implementation.

>> This is not helping, we have implementations not really frozen.
>
> Who is "we" here?

The community, at large. The people who actually use and distribute this code.

> I *absolutely* would not have pushed a copy of this to Debian for people
> to use if there was anything short of an ASTRONOMICALLY SMALL chance of
> there still being a bitstream change.  (my caps this time, I'm serious
> about that.  It's not going to happen while the upstream developers
> draw breath to stab anybody who might try).

We just need an opus implementation to say clearly that it will be
bit-stream compatible with future releases. Not play with chances.

>> Since no releases happened recently, I assume no security issues are known.
>
> That's an interesting assumption ...

Isn't it how security works? It's safe until it's proven it's no
longer, isn't it?

> Here's another, that is actually a little more grounded in what the people
> who might actually make those releases have actually been focussed on:
>
> No releases happened because *nobody* is looking at celt 0.5.1 at all, let

Spice depends on celt 0.5.1, people do care.

> What is your plan to confirm or deny 0.5.1 is affected by the problems that
> he found and fixed in later versions?

I am not saying it's not without problems. But if something is
serious, the Spice project will certainly drop celt 0.5.1 support or
fix it. If Spice should drop celt 0.5.1 support altogether for
security reasons, a bunch of people would be interested to know.

> You could just wait for the news of a live exploit actually doing the rounds,
> but I personally wouldn't want to trust my own systems to that plan.

I am no security expert, but even if we would make a security audit,
there would still be chances an exploit be found.

Rushing into using new libraries isn't going to magically make
security issues disappear.

> I'm not particularly interested in arguing with someone who has already made
> up their mind and for whom facts are just annoying details to handwave away.
> But if you're actually interested in the facts, I'm happy to help answer any
> questions that you or the rest of the spice team might have.

I have not made up my mind, but my points are:
- spice depends on 0.5.1 of celt, and for quite some time (several
years), so security issues should be addressed
- xiph opus implementation doesn't guarantee bit-stream compatibility
yet, according to release log

> It's your decision what you do with spice.  But saying "Opus isn't ready yet"
> is simply not true.  You may have other reasons not to use it, but that isn't
> one of them that stands up to genuine scrutiny.

Personally, I would be glad to have opus support once there is an
implementation the spice server can rely on.

Even when opus support will be added, celt 0.5.1 will continue to be
maintained unless we drop audio support for older releases.

regards
On Tue, Jun 12, 2012 at 09:38:37PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jun 12, 2012 at 8:19 PM, Ron <ron@debian.org> wrote:
> > More official than the codec working group declaring it to be in its
> > finally frozen form, or the codec specification being submitted to and
> > approved by the IESG for publication?
> 
> You keep refering to standards, while I am talking about what we can
> actually rely on, the implementation.

Then you have my apologies for omitting a fact from my original list
that I'd assumed was common knowledge:

 - The implementation you are referring to _is_ the normative part
   of the now finalised standard.  That code defines the bitstream.
   Officially.  Any other text is purely informative.

So any deviation from compatibility with that in later releases, or any
other implementation, will be a very serious bug, and cause it to no
longer be an implementation of Opus.   ie. This simply isn't going to
happen now.


> We just need an opus implementation to say clearly that it will be
> bit-stream compatible with future releases. Not play with chances.

It said that by being published as the normative standard :)

You have more chance of your office being struck by a meterorite
shaped like a rhinoceros, with your name written on it in 40 foot
high gold leaf, whistling ride of the valkyries as it descends into
the atmosphere, than of opus breaking bitstream compatibility now.

Really.  It's somewhere on that sort of order of risk.


You're already playing a game of chance.  Think of Opus as your
insurance policy if your luck doesn't hold out as long as you'd like.

 Ron
Hi,

>> More official than the codec working group declaring it to be in its
>> finally frozen form, or the codec specification being submitted to and
>> approved by the IESG for publication?
> 
> You keep refering to standards, while I am talking about what we can
> actually rely on, the implementation.

I'd be very surprised if the most recent opus release doesn't conform to
the submitted specification.  Guess the 1.0 release just waits for the
formal approval & publication

>>> This is not helping, we have implementations not really frozen.

???

Can't see what your problem is ...

>>> Since no releases happened recently, I assume no security issues are known.
>>
>> That's an interesting assumption ...
> 
> Isn't it how security works? It's safe until it's proven it's no
> longer, isn't it?

For a piece of software not being actively maintained any more by anyone
I wouldn't bet on it.

> Spice depends on celt 0.5.1, people do care.

What kind of care, beyond packaging & using it?

>> It's your decision what you do with spice.  But saying "Opus isn't ready yet"
>> is simply not true.  You may have other reasons not to use it, but that isn't
>> one of them that stands up to genuine scrutiny.
> 
> Personally, I would be glad to have opus support once there is an
> implementation the spice server can rely on.

IMHO we should undust Alons opus patches *now* and get stuff ready.
Given the sample rate issue a bit more work that just switching the
codec, so it will need some time anyway.

cheers,
  Gerd
On Wed, Jun 13, 2012 at 09:25:59AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >> More official than the codec working group declaring it to be in its
> >> finally frozen form, or the codec specification being submitted to and
> >> approved by the IESG for publication?
> > 
> > You keep refering to standards, while I am talking about what we can
> > actually rely on, the implementation.
> 
> I'd be very surprised if the most recent opus release doesn't conform to
> the submitted specification.  Guess the 1.0 release just waits for the
> formal approval & publication
> 
> >>> This is not helping, we have implementations not really frozen.
> 
> ???
> 
> Can't see what your problem is ...
> 
> >>> Since no releases happened recently, I assume no security issues are known.
> >>
> >> That's an interesting assumption ...
> > 
> > Isn't it how security works? It's safe until it's proven it's no
> > longer, isn't it?
> 
> For a piece of software not being actively maintained any more by anyone
> I wouldn't bet on it.
> 
> > Spice depends on celt 0.5.1, people do care.
> 
> What kind of care, beyond packaging & using it?
> 
> >> It's your decision what you do with spice.  But saying "Opus isn't ready yet"
> >> is simply not true.  You may have other reasons not to use it, but that isn't
> >> one of them that stands up to genuine scrutiny.
> > 
> > Personally, I would be glad to have opus support once there is an
> > implementation the spice server can rely on.
> 
> IMHO we should undust Alons opus patches *now* and get stuff ready.
> Given the sample rate issue a bit more work that just switching the
> codec, so it will need some time anyway.

I replied to Ron privately but let me repeat my problems last time
around. They are certainly not unsurmountable, I just didn't have the
right motivation, that Ron provided right now (i.e. saying that we might
be carrying a breakable package):
 We have a fixed sample rate of 44100 in spiceaudio, as defined by spice
 server. I guess I could try to either resample (didn't want to go down
 that path last time since it seemed a waste of cpu for something I
 could just adjust upfront in the guest/client for playback/recording)
 or change it. I can start by seeing if changing the rate breaks
 anything using celt, if not I can change it for both celt and opus. If
 it does break I still have the options of either having a single codec
 enabled, or doing resampling and providing the older sampling rate (and
 then some time in the future when we drop celt we can drop the
 resampling).

Alon
> 
> cheers,
>   Gerd
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

>> IMHO we should undust Alons opus patches *now* and get stuff ready.
>> Given the sample rate issue a bit more work that just switching the
>> codec, so it will need some time anyway.
> 
> I replied to Ron privately but let me repeat my problems last time
> around. They are certainly not unsurmountable, I just didn't have the
> right motivation, that Ron provided right now (i.e. saying that we might
> be carrying a breakable package):
>  We have a fixed sample rate of 44100 in spiceaudio, as defined by spice
>  server. I guess I could try to either resample (didn't want to go down
>  that path last time since it seemed a waste of cpu for something I
>  could just adjust upfront in the guest/client for playback/recording)
>  or change it.

It's a waste indeed, and I think the best long-term option is to go for
48 kHz.  Real hardware does this, virtual qemu hardware does this, and
using it on the complete path from guest to spice client makes perfect
sense.

I suspect there is no way around resampling support though.  We'll need
it for compatibility reasons, so sound keeps working if only one side is
able to operate at 48 kHz.

cheers,
  Gerd
On Wed, Jun 13, 2012 at 01:49:59PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >> IMHO we should undust Alons opus patches *now* and get stuff ready.
> >> Given the sample rate issue a bit more work that just switching the
> >> codec, so it will need some time anyway.
> > 
> > I replied to Ron privately

Yeah, I only took that part off the list to avoid distracting an otherwise
busy thread with what seemed like a 'side issue' at the time, that Alon and
I could clarify and bring back to it again later.  You're welcome to quote
or forward any of that if you like.

> > but let me repeat my problems last time
> > around. They are certainly not unsurmountable, I just didn't have the
> > right motivation, that Ron provided right now (i.e. saying that we might
> > be carrying a breakable package):
> >  We have a fixed sample rate of 44100 in spiceaudio, as defined by spice
> >  server. I guess I could try to either resample (didn't want to go down
> >  that path last time since it seemed a waste of cpu for something I
> >  could just adjust upfront in the guest/client for playback/recording)
> >  or change it.
> 
> It's a waste indeed, and I think the best long-term option is to go for
> 48 kHz.  Real hardware does this, virtual qemu hardware does this, and
> using it on the complete path from guest to spice client makes perfect
> sense.

The 44.1 rate is what CD audio uses (for amusing reasons related to the
frame rate of video cassettes originally used for storing digital audio)
but it's becoming increasingly isolated in that for modern digital audio.

Lots of hardware these days doesn't handle that rate natively, and needs
it to be resampled to/from 48k anyway - so if you don't need 44.1 for
other reasons of your own, then roundtripping through it is a good thing
to avoid.

> I suspect there is no way around resampling support though.  We'll need
> it for compatibility reasons, so sound keeps working if only one side is
> able to operate at 48 kHz.

The ability to resample OTOH probably isn't a silly thing to have, and
can be fairly easily set up as an inline filter that is switched out
if you really don't need it.

Just to be clear about this though, there is actually no reason that
both sides of the link need to use the *same* sample rate, even without
a resampler.

For instance you can perfectly well push an 8kHz stream into opus, and
decode that as a 48kHz stream at the other end, or vice versa, without
resampling, for any integer fraction of 48k down to 8.  Or just because
one end needs to resample from 44.1 to 48, it doesn't prevent the other
side from still natively using 48k etc.

So this isn't something you strictly need to coordinate between endpoints
unless they have their own requirements to do that.  The common thing
they negotiate is "we both speak opus", while the sample rate they see
at their own ends can be entirely up to them to decide, without needing
to agree on a common rate, or to share information about the rate that
they chose.

Once encoded in opus, there is no "sample rate" anymore, except what you
choose to decode it as, at the time you choose to decode it.

Does that make sense, or did I just make this sound even more confusing
than it really is ;?


 Cheers,
 Ron
Hi,

>> I suspect there is no way around resampling support though.  We'll need
>> it for compatibility reasons, so sound keeps working if only one side is
>> able to operate at 48 kHz.
> 
> The ability to resample OTOH probably isn't a silly thing to have, and
> can be fairly easily set up as an inline filter that is switched out
> if you really don't need it.
> 
> Just to be clear about this though, there is actually no reason that
> both sides of the link need to use the *same* sample rate, even without
> a resampler.
> 
> For instance you can perfectly well push an 8kHz stream into opus, and
> decode that as a 48kHz stream at the other end, or vice versa, without
> resampling, for any integer fraction of 48k down to 8.  Or just because
> one end needs to resample from 44.1 to 48, it doesn't prevent the other
> side from still natively using 48k etc.

Ah, ok, so opus is different from mp3 here.  So you could "reample"
using opus?  i.e. have one side feed 44.1 into opus encoder, then send
opus over the wire, have the other side decode 48 kHz from opus?

Is that true for celt 0.5.1 too btw?

> So this isn't something you strictly need to coordinate between endpoints
> unless they have their own requirements to do that.  The common thing
> they negotiate is "we both speak opus", while the sample rate they see
> at their own ends can be entirely up to them to decide, without needing
> to agree on a common rate, or to share information about the rate that
> they chose.
> 
> Once encoded in opus, there is no "sample rate" anymore, except what you
> choose to decode it as, at the time you choose to decode it.
> 
> Does that make sense, or did I just make this sound even more confusing
> than it really is ;?

Makes perfect sense.

thanks,
  Gerd
On Tue, 2012-06-12 at 16:33 +0200, Christophe Fergeau wrote:
> On Tue, Jun 12, 2012 at 09:59:39AM -0400, Marc-André Lureau wrote:
> > 
> > 
> > ----- Mensaje original -----
> > > On Tue, Jun 12, 2012 at 09:11:24AM -0400, Marc-André Lureau wrote:
> > > > As long as the bitstream is not frozen, we can't use opus, or we
> > > > will
> > > > have the same problems as with celt today.
> > >
> > > As I understand it, while the bitstream is not officially frozen yet,
> > > it's
> > > very unlikely to change before the real freeze (unless big last
> > > minute
> > > issues are fine), so an Opus mode (marked as "no compat guarantees,
> > > use at
> > > your own risk, ...") will probably not cause significant
> > > compatibilities issues.
> > 
> > That's just guesses. It's not about library API, but about bitstream.
> > There is no guarantee.
> 
> A guess supported by the slides at http://www.opus-codec.org/presentations/
> , by various mailing posts from opus developers, ...
> 
> > Sticking to celt051 is still a safer alternative.
> 
> Not suggesting dropping celt051 support upstream...
> 
> > Otherwise, how would you identify almost-frozen bitstream to frozen bitstream?
> > You would have to identify by library version (erk!)
> > and be compatible with the old and new bitstram (which might be complicated
> > depending on library design), or be incompatible with the intermediate version,
> > situation which we better avoid!
> 
> We would make no guarantee with interoperability between binaries using
> different opus versions until the format is officially frozen. I agree
> there's a bit of uncertainty in this move, but I think that at this point
> it's a reasonable assumption that things will work, even with different
> opus versions.

It seems like Opus is at a stage where we want to at least start adding
support for it so we can switch to it by default as early as possible.
Its not like this is a new idea, the plan was always to jump to a stable
bitstream format when one appeared.

However, that is imho a different issue than the celt051 support. A new
release of spice client and server supporting opus does not magically
make old servers and client disappear, so it would still be the case
that e.g. debian spice client would get lame audio performance if
connecting to say a RHEV spice client, or if some old client connects to
a server running on debian. In time, it would perhaps make sense to drop
celt051 support, but its seems pretty bad to release a client binary
that doesn't do audio well with all currently existing deployed servers.
On Thu, Jun 14, 2012 at 10:14:36AM +0200, Alexander Larsson wrote:
> However, that is imho a different issue than the celt051 support. A new
> release of spice client and server supporting opus does not magically
> make old servers and client disappear, so it would still be the case
> that e.g. debian spice client would get lame audio performance if
> connecting to say a RHEV spice client, or if some old client connects to
> a server running on debian. In time, it would perhaps make sense to drop
> celt051 support, but its seems pretty bad to release a client binary
> that doesn't do audio well with all currently existing deployed servers.

It all depends if we consider remote SPICE access with limited bandwidth and
with audio needed will be an important use case that must run as good as
possible. In my opinion, sound is most of the time not the most important
thing if what you want is a remote desktop. It also won't be really
noticeable on LAN, or in GNOME Boxes use case, ...

What I gather from this thread is that we don't want anyone to use the
fallback PCM code, which means we should deprecate it if that's really what
we want... Maybe the clients could be patched to stop advertising raw PCM
support? I don't know if no audio at all is more acceptable than not doing
audio well in some cases.

Christophe
On Thu, 2012-06-14 at 12:31 +0200, Christophe Fergeau wrote:
> On Thu, Jun 14, 2012 at 10:14:36AM +0200, Alexander Larsson wrote:
> > However, that is imho a different issue than the celt051 support. A new
> > release of spice client and server supporting opus does not magically
> > make old servers and client disappear, so it would still be the case
> > that e.g. debian spice client would get lame audio performance if
> > connecting to say a RHEV spice client, or if some old client connects to
> > a server running on debian. In time, it would perhaps make sense to drop
> > celt051 support, but its seems pretty bad to release a client binary
> > that doesn't do audio well with all currently existing deployed servers.
> 
> It all depends if we consider remote SPICE access with limited bandwidth and
> with audio needed will be an important use case that must run as good as
> possible. In my opinion, sound is most of the time not the most important
> thing if what you want is a remote desktop. It also won't be really
> noticeable on LAN, or in GNOME Boxes use case, ...
> 
> What I gather from this thread is that we don't want anyone to use the
> fallback PCM code, which means we should deprecate it if that's really what
> we want... Maybe the clients could be patched to stop advertising raw PCM
> support? I don't know if no audio at all is more acceptable than not doing
> audio well in some cases.

I don't know if that is true. If nothing else works then in some cases
PCM is a good approach. However, maybe the client should warn about this
and allow disabling audio?
On Thu, Jun 14, 2012 at 12:50:39PM +0200, Alexander Larsson wrote:
> On Thu, 2012-06-14 at 12:31 +0200, Christophe Fergeau wrote:
> > What I gather from this thread is that we don't want anyone to use the
> > fallback PCM code, which means we should deprecate it if that's really what
> > we want... Maybe the clients could be patched to stop advertising raw PCM
> > support? I don't know if no audio at all is more acceptable than not doing
> > audio well in some cases.
> 
> I don't know if that is true. If nothing else works then in some cases
> PCM is a good approach. However, maybe the client should warn about this
> and allow disabling audio?

If this is good enough for you to avoid bad audio experience when celt
isn't there, then yes this would good too.

Christophe

> 
>
Hi,

> What I gather from this thread is that we don't want anyone to use the
> fallback PCM code, which means we should deprecate it if that's really what
> we want... Maybe the clients could be patched to stop advertising raw PCM
> support?

You can't, PCM is the baseline which isn't negotiated.  You can only
disconnect the sound channel.

> I don't know if no audio at all is more acceptable than not doing
> audio well in some cases.

Of course you can warn if no codec could be negotiated and offer the
option to disable sound.  Not sure it is worth the trouble though, as
far I know there is no data traveling the wire if the guest doesn't play
audio.

cheers,
  Gerd
Christophe Fergeau píše v Čt 14. 06. 2012 v 12:31 +0200:
> On Thu, Jun 14, 2012 at 10:14:36AM +0200, Alexander Larsson wrote:
> > However, that is imho a different issue than the celt051 support. A new
> > release of spice client and server supporting opus does not magically
> > make old servers and client disappear, so it would still be the case
> > that e.g. debian spice client would get lame audio performance if
> > connecting to say a RHEV spice client, or if some old client connects to
> > a server running on debian. In time, it would perhaps make sense to drop
> > celt051 support, but its seems pretty bad to release a client binary
> > that doesn't do audio well with all currently existing deployed servers.
> 
> It all depends if we consider remote SPICE access with limited bandwidth and
> with audio needed will be an important use case that must run as good as
> possible. In my opinion, sound is most of the time not the most important
> thing if what you want is a remote desktop. It also won't be really
> noticeable on LAN, or in GNOME Boxes use case, ...
> 
> What I gather from this thread is that we don't want anyone to use the
> fallback PCM code, which means we should deprecate it if that's really what
> we want... Maybe the clients could be patched to stop advertising raw PCM
> support? I don't know if no audio at all is more acceptable than not doing
> audio well in some cases.

There was an interest in lossless audio for some medical application
some time ago so support for explicit codec choice (and incoproration of
FLAC or some better lossless codec, if any) would be a big improvement
for some spice users.

David

> 
> Christophe
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On 06/14/2012 06:31 AM, Christophe Fergeau wrote:
> On Thu, Jun 14, 2012 at 10:14:36AM +0200, Alexander Larsson wrote:
>> However, that is imho a different issue than the celt051 support. A new
>> release of spice client and server supporting opus does not magically
>> make old servers and client disappear, so it would still be the case
>> that e.g. debian spice client would get lame audio performance if
>> connecting to say a RHEV spice client, or if some old client connects to
>> a server running on debian. In time, it would perhaps make sense to drop
>> celt051 support, but its seems pretty bad to release a client binary
>> that doesn't do audio well with all currently existing deployed servers.
> It all depends if we consider remote SPICE access with limited bandwidth and
> with audio needed will be an important use case that must run as good as
> possible. In my opinion, sound is most of the time not the most important
> thing if what you want is a remote desktop. It also won't be really
> noticeable on LAN, or in GNOME Boxes use case, ...
>
> What I gather from this thread is that we don't want anyone to use the
> fallback PCM code, which means we should deprecate it if that's really what
> we want... Maybe the clients could be patched to stop advertising raw PCM
> support? I don't know if no audio at all is more acceptable than not doing
> audio well in some cases.

As a user, being given the information and the option to disable is the 
best case.  A message saying "No compressed audio codec is available.  
Continuing with audio enabled may degrade performance.  Disable audio 
for this sesison?"

This would inform, and allow a user on a LAN to proceed or not, and give 
them enough info to get a head start googling for the problem.  A debian 
user still could install the necessary libraries his/her self and get 
the desired performance, given the information in the popup.

Silently using PCM would suck.  Silently disabling audio would suck. IMHO.
14.06.2012 18:23, David Mansfield wrote:
[]
> As a user, being given the information and the option to disable is the best case.  A message saying "No compressed audio codec is available.  Continuing with audio enabled may degrade performance.  Disable audio for this sesison?"

I wonder which popup you're talking about.  Is that in client?
Because if you're talking about the server, it is not the most
smart idea to ask someone if he wants to disable audio, during
headless system startup.

If the talk is about the client, the server patch (the one which
this thread is about) can still be applied, how do you think?
This might need a bit more investigation, ie, how the server
part will react if the application (the server, eg qemu) explicitly
enabled audio compression.  But at least no popups there should
be added.

Will you help adding such a dialog (and a proper setting too, to be
able to shut it off) to client software, eg, to spice-gtk client for
which Lian posted a patch making celt optional?  This needs to be
done really fast if there's a chance still to get this stuff into
the next debian release.

> This would inform, and allow a user on a LAN to proceed or not, and give them enough info to get a head start googling for the problem.  A debian user still could install the necessary libraries his/her self and get the desired performance, given the information in the popup.
> 
> Silently using PCM would suck.  Silently disabling audio would suck. IMHO.

Well, requiring answering an annoying question every time you start
an app is even more sucky, in my opinion.  If that'd be the case,
I'd go and fix the code by commenting out this dialog altogther.

My opinion is that the the whole issue isn't worth this and other
similar discussions already, it is not a use case important enough
to receive so much attention.

And I'm about to give up, too, just disabling spice if spice guys
don't want it to be shipped in popular distributions.

Thanks,

/mjt
On Thu, Jun 14, 2012 at 08:55:59AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >> I suspect there is no way around resampling support though.  We'll need
> >> it for compatibility reasons, so sound keeps working if only one side is
> >> able to operate at 48 kHz.
> > 
> > The ability to resample OTOH probably isn't a silly thing to have, and
> > can be fairly easily set up as an inline filter that is switched out
> > if you really don't need it.
> > 
> > Just to be clear about this though, there is actually no reason that
> > both sides of the link need to use the *same* sample rate, even without
> > a resampler.
> > 
> > For instance you can perfectly well push an 8kHz stream into opus, and
> > decode that as a 48kHz stream at the other end, or vice versa, without
> > resampling, for any integer fraction of 48k down to 8.  Or just because
> > one end needs to resample from 44.1 to 48, it doesn't prevent the other
> > side from still natively using 48k etc.
> 
> Ah, ok, so opus is different from mp3 here.  So you could "reample"
> using opus?  i.e. have one side feed 44.1 into opus encoder, then send
> opus over the wire, have the other side decode 48 kHz from opus?

From 44.1 you'd still need to externally resample to one of 8, 12, 16, 24,
or 48k - but you can put any of those rates in, and take any of those out
again without them needing to be the same.  So only endpoints actually
needing an 'oddball' rate would need a resampler to get them, they wouldn't
force the other end to also support and use that rate as well.

> Is that true for celt 0.5.1 too btw?

Unfortunately no.  For celt you compose your own 'mode' and that mode
essentially defines the bitstream.  Different modes make different
bitstreams so you need to decode with the same mode you encoded with.

For opus, the emphasis on "everything should be interoperable" came to
the fore, so from the user visible side there is only one mode, that
supports multiple rates/frame sizes/channel counts.

  Ron
On Thu, Jun 14, 2012 at 01:58:55PM +0200, David Jaša wrote:
> Christophe Fergeau píše v Čt 14. 06. 2012 v 12:31 +0200:
> > On Thu, Jun 14, 2012 at 10:14:36AM +0200, Alexander Larsson wrote:
> > > However, that is imho a different issue than the celt051 support. A new
> > > release of spice client and server supporting opus does not magically
> > > make old servers and client disappear, so it would still be the case
> > > that e.g. debian spice client would get lame audio performance if
> > > connecting to say a RHEV spice client, or if some old client connects to
> > > a server running on debian. In time, it would perhaps make sense to drop
> > > celt051 support, but its seems pretty bad to release a client binary
> > > that doesn't do audio well with all currently existing deployed servers.
> > 
> > It all depends if we consider remote SPICE access with limited bandwidth and
> > with audio needed will be an important use case that must run as good as
> > possible. In my opinion, sound is most of the time not the most important
> > thing if what you want is a remote desktop. It also won't be really
> > noticeable on LAN, or in GNOME Boxes use case, ...
> > 
> > What I gather from this thread is that we don't want anyone to use the
> > fallback PCM code, which means we should deprecate it if that's really what
> > we want... Maybe the clients could be patched to stop advertising raw PCM
> > support? I don't know if no audio at all is more acceptable than not doing
> > audio well in some cases.
> 
> There was an interest in lossless audio for some medical application
> some time ago so support for explicit codec choice (and incoproration of
> FLAC or some better lossless codec, if any) would be a big improvement
> for some spice users.

I guess this kind of depends on what they mean by "lossless".

At 64kb/s, in listening tests of opus, there were a notable number of
samples where the listeners could not pick opus from the reference.

At 128kb/s, even codec developers and practiced listeners have trouble
hearing any difference from the reference samples with high quality
headphones, on all but the most pathologically difficult examples
to code.

And we expect that will improve even further in the months to come.
The decoder and bitstream are frozen, but there is still potential
for a compatible encoder to make notable improvements to quality.

So if what they want is 'transparency', then maybe opus can do this
for them all by itself.  That's not the same as true lossless if they
need to analyse the samples received - but that would be something
I'd expect to have an application specific data stream all of its own
if that was needed, rather than it using the generic audio support
of spice.

 Ron
Ron píše v Pá 15. 06. 2012 v 16:29 +0930:
> On Thu, Jun 14, 2012 at 01:58:55PM +0200, David Jaša wrote:
> > Christophe Fergeau píše v Čt 14. 06. 2012 v 12:31 +0200:
> > > On Thu, Jun 14, 2012 at 10:14:36AM +0200, Alexander Larsson wrote:
> > > > However, that is imho a different issue than the celt051 support. A new
> > > > release of spice client and server supporting opus does not magically
> > > > make old servers and client disappear, so it would still be the case
> > > > that e.g. debian spice client would get lame audio performance if
> > > > connecting to say a RHEV spice client, or if some old client connects to
> > > > a server running on debian. In time, it would perhaps make sense to drop
> > > > celt051 support, but its seems pretty bad to release a client binary
> > > > that doesn't do audio well with all currently existing deployed servers.
> > > 
> > > It all depends if we consider remote SPICE access with limited bandwidth and
> > > with audio needed will be an important use case that must run as good as
> > > possible. In my opinion, sound is most of the time not the most important
> > > thing if what you want is a remote desktop. It also won't be really
> > > noticeable on LAN, or in GNOME Boxes use case, ...
> > > 
> > > What I gather from this thread is that we don't want anyone to use the
> > > fallback PCM code, which means we should deprecate it if that's really what
> > > we want... Maybe the clients could be patched to stop advertising raw PCM
> > > support? I don't know if no audio at all is more acceptable than not doing
> > > audio well in some cases.
> > 
> > There was an interest in lossless audio for some medical application
> > some time ago so support for explicit codec choice (and incoproration of
> > FLAC or some better lossless codec, if any) would be a big improvement
> > for some spice users.
> 
> I guess this kind of depends on what they mean by "lossless".
> 
> At 64kb/s, in listening tests of opus, there were a notable number of
> samples where the listeners could not pick opus from the reference.
> 
> At 128kb/s, even codec developers and practiced listeners have trouble
> hearing any difference from the reference samples with high quality
> headphones, on all but the most pathologically difficult examples
> to code.
> 
> And we expect that will improve even further in the months to come.
> The decoder and bitstream are frozen, but there is still potential
> for a compatible encoder to make notable improvements to quality.
> 
> So if what they want is 'transparency', then maybe opus can do this
> for them all by itself.  

Celt is already good enough in my experience in this regard and I don't
expect Opus to be any worse.

> That's not the same as true lossless if they
> need to analyse the samples received - but that would be something
> I'd expect to have an application specific data stream all of its own
> if that was needed, rather than it using the generic audio support
> of spice.

IIRC this was the case, they wanted to use it to feed outputs from some
tools to a software run in the guest and they liked that the spice audio
support was ready to use for them, apart from not allowing codec choice.

David

> 
>  Ron
> 
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

> From 44.1 you'd still need to externally resample to one of 8, 12, 16, 24,
> or 48k - but you can put any of those rates in, and take any of those out
> again without them needing to be the same.  So only endpoints actually
> needing an 'oddball' rate would need a resampler to get them, they wouldn't
> force the other end to also support and use that rate as well.

Ok, so we must resample 44.1 anyway before encoding.

>> Is that true for celt 0.5.1 too btw?
> 
> Unfortunately no.  For celt you compose your own 'mode' and that mode
> essentially defines the bitstream.  Different modes make different
> bitstreams so you need to decode with the same mode you encoded with.

Ok, so it's easiest to simply not touch celt.

So, how to go forward?  spice-server gets a 44.1 stream from qemu today.
 I think we should switch that to 48k.  Needs changes in the sound
interface and in qemu.  spice-server needs to be able to handle both old
(44.1) and new (48k) qemu.  spice-server also needs to be able to handle
clients with different capabilities (possibly even at the same time if
multiclient is enabled, not sure multiclient support covers the sound
channel though).

So we'll need resampling support for both 44.1 -> 48k and 48k -> 44.1.
In the end we'll have the following cases, at the server side:

  old qemu, no caps [-> 44.1 pcm]
  pass through pcm audio data as-is

  new qemu, no caps [-> 44.1 pcm]
  resample 48k -> 44.1

  old qemu, cap_celt
  encode 44.1 celt

  new qemu, cap_celt
  resample 48k -> 44.1, encode 44.1 celt

  old qemu, cap_opus
  resample 44.1 -> 48k, encode opus

  new qemu, cap_opus
  encode opus

We might want to add:

  new qemu, cap_pcm48k
  pass through pcm audio data as-is

Long-term (new spice-server, new qemu, clients supporting opus) we'll
operate at 48k on the whole chain without resampling needed in the
spice-server.

Client side is easy, we just decode stuff and pass it on to pulseaudio
to deal with it.

cheers,
  Gerd
On Fri, Jun 15, 2012 at 11:17:34AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > From 44.1 you'd still need to externally resample to one of 8, 12, 16, 24,
> > or 48k - but you can put any of those rates in, and take any of those out
> > again without them needing to be the same.  So only endpoints actually
> > needing an 'oddball' rate would need a resampler to get them, they wouldn't
> > force the other end to also support and use that rate as well.
> 
> Ok, so we must resample 44.1 anyway before encoding.
> 
> >> Is that true for celt 0.5.1 too btw?
> > 
> > Unfortunately no.  For celt you compose your own 'mode' and that mode
> > essentially defines the bitstream.  Different modes make different
> > bitstreams so you need to decode with the same mode you encoded with.
> 
> Ok, so it's easiest to simply not touch celt.
> 
> So, how to go forward?  spice-server gets a 44.1 stream from qemu today.
>  I think we should switch that to 48k.  Needs changes in the sound
> interface and in qemu.  spice-server needs to be able to handle both old
> (44.1) and new (48k) qemu.  spice-server also needs to be able to handle
> clients with different capabilities (possibly even at the same time if
> multiclient is enabled, not sure multiclient support covers the sound
> channel though).

No sound channel for multiclient.

> 
> So we'll need resampling support for both 44.1 -> 48k and 48k -> 44.1.
> In the end we'll have the following cases, at the server side:
> 
>   old qemu, no caps [-> 44.1 pcm]
>   pass through pcm audio data as-is
> 
>   new qemu, no caps [-> 44.1 pcm]
>   resample 48k -> 44.1
> 
>   old qemu, cap_celt
>   encode 44.1 celt
> 
>   new qemu, cap_celt
>   resample 48k -> 44.1, encode 44.1 celt
> 
>   old qemu, cap_opus
>   resample 44.1 -> 48k, encode opus
> 
>   new qemu, cap_opus
>   encode opus
> 
> We might want to add:
> 
>   new qemu, cap_pcm48k
>   pass through pcm audio data as-is
> 
> Long-term (new spice-server, new qemu, clients supporting opus) we'll
> operate at 48k on the whole chain without resampling needed in the
> spice-server.
> 
> Client side is easy, we just decode stuff and pass it on to pulseaudio
> to deal with it.
> 

Sounds like a good plan.

> cheers,
>   Gerd
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Thu, Jun 14, 2012 at 06:49:28PM +0400, Michael Tokarev wrote:
> And I'm about to give up, too, just disabling spice if spice guys
> don't want it to be shipped in popular distributions.

Well, at worst you have the option of shipping spice with a distro patch
even if upstream disagrees with it while proper Opus support gets
implemented.

Christophe