[Spice-devel] [spice opus support 3/6 (take 3)] Add support for the Opus codec
Christophe Fergeau
cfergeau at redhat.com
Thu Nov 21 05:23:06 PST 2013
Hey,
A few minor comments below,
Christophe
On Tue, Nov 12, 2013 at 03:52:53PM -0600, Jeremy White wrote:
> diff --git a/server/snd_worker.c b/server/snd_worker.c
> index 57f30c2..1590abf 100644
> --- a/server/snd_worker.c
> +++ b/server/snd_worker.c
> @@ -156,12 +156,14 @@ struct SpicePlaybackState {
> struct SndWorker worker;
> SpicePlaybackInstance *sin;
> SpiceVolumeState volume;
> + uint32_t frequency;
> };
>
> struct SpiceRecordState {
> struct SndWorker worker;
> SpiceRecordInstance *sin;
> SpiceVolumeState volume;
> + uint32_t frequency;
> };
>
> typedef struct RecordChannel {
> @@ -370,14 +372,28 @@ static int snd_record_handle_message(SndChannel *channel, size_t size, uint32_t
> return snd_record_handle_write((RecordChannel *)channel, size, message);
> case SPICE_MSGC_RECORD_MODE: {
> SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
> - record_channel->mode = mode->mode;
> + SpiceRecordState *st = SPICE_CONTAINEROF(channel->worker, SpiceRecordState, worker);
> record_channel->mode_time = mode->time;
> - if (record_channel->mode != SPICE_AUDIO_DATA_MODE_RAW &&
> - ! snd_codec_is_capable(record_channel->mode)) {
> - spice_printerr("unsupported mode %d", record_channel->mode);
> + if (mode->mode != SPICE_AUDIO_DATA_MODE_RAW)
> + {
> + if (snd_codec_is_capable(mode->mode, st->frequency)) {
> + if (snd_codec_create(&record_channel->codec, mode->mode, st->frequency, FALSE, TRUE) == SND_CODEC_OK) {
> + record_channel->mode = mode->mode;
> + } else {
> + spice_printerr("create decoder failed");
> + return FALSE;
> + }
> + }
> + else {
> + spice_printerr("unsupported mode %d", record_channel->mode);
> + return FALSE;
> + }
> }
> + else
> + record_channel->mode = mode->mode;
> break;
> }
> +
> case SPICE_MSGC_RECORD_START_MARK: {
> SpiceMsgcRecordStartMark *mark = (SpiceMsgcRecordStartMark *)message;
> record_channel->start_time = mark->time;
> @@ -615,6 +631,7 @@ static int snd_playback_send_latency(PlaybackChannel *playback_channel)
> static int snd_playback_send_start(PlaybackChannel *playback_channel)
> {
> SndChannel *channel = (SndChannel *)playback_channel;
> + SpicePlaybackState *st = SPICE_CONTAINEROF(channel->worker, SpicePlaybackState, worker);
> SpiceMsgPlaybackStart start;
>
> if (!snd_reset_send_data(channel, SPICE_MSG_PLAYBACK_START)) {
> @@ -622,7 +639,7 @@ static int snd_playback_send_start(PlaybackChannel *playback_channel)
> }
>
> start.channels = SPICE_INTERFACE_PLAYBACK_CHAN;
> - start.frequency = SPICE_INTERFACE_PLAYBACK_FREQ;
> + start.frequency = st->frequency;
> spice_assert(SPICE_INTERFACE_PLAYBACK_FMT == SPICE_INTERFACE_AUDIO_FMT_S16);
> start.format = SPICE_AUDIO_FMT_S16;
> start.time = reds_get_mm_time();
> @@ -656,6 +673,7 @@ static int snd_playback_send_ctl(PlaybackChannel *playback_channel)
> static int snd_record_send_start(RecordChannel *record_channel)
> {
> SndChannel *channel = (SndChannel *)record_channel;
> + SpiceRecordState *st = SPICE_CONTAINEROF(channel->worker, SpiceRecordState, worker);
> SpiceMsgRecordStart start;
>
> if (!snd_reset_send_data(channel, SPICE_MSG_RECORD_START)) {
> @@ -663,7 +681,7 @@ static int snd_record_send_start(RecordChannel *record_channel)
> }
>
> start.channels = SPICE_INTERFACE_RECORD_CHAN;
> - start.frequency = SPICE_INTERFACE_RECORD_FREQ;
> + start.frequency = st->frequency;
> spice_assert(SPICE_INTERFACE_RECORD_FMT == SPICE_INTERFACE_AUDIO_FMT_S16);
> start.format = SPICE_AUDIO_FMT_S16;
> spice_marshall_msg_record_start(channel->send_data.marshaller, &start);
> @@ -745,11 +763,13 @@ static int snd_playback_send_write(PlaybackChannel *playback_channel)
>
> if (playback_channel->mode == SPICE_AUDIO_DATA_MODE_RAW) {
> spice_marshaller_add_ref(channel->send_data.marshaller,
> - (uint8_t *)frame->samples, sizeof(frame->samples));
> + (uint8_t *)frame->samples,
> + snd_codec_frame_size(playback_channel->codec) * sizeof(frame->samples[0]));
> }
> else {
> int n = sizeof(playback_channel->encode_buf);
> - if (snd_codec_encode(playback_channel->codec, (uint8_t *) frame->samples, sizeof(frame->samples),
> + if (snd_codec_encode(playback_channel->codec, (uint8_t *) frame->samples,
> + snd_codec_frame_size(playback_channel->codec) * sizeof(frame->samples[0]),
> playback_channel->encode_buf, &n) != SND_CODEC_OK) {
> spice_printerr("encode failed");
> snd_disconnect_channel(channel);
> @@ -1126,12 +1146,15 @@ void snd_set_playback_latency(RedClient *client, uint32_t latency)
> }
> }
>
> -static int snd_desired_audio_mode(int client_can_celt)
> +static int snd_desired_audio_mode(int frequency, int client_can_celt, int client_can_opus)
> {
> if (! playback_compression)
> return SPICE_AUDIO_DATA_MODE_RAW;
>
> - if (client_can_celt && snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1))
> + if (client_can_opus && snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_OPUS, frequency))
> + return SPICE_AUDIO_DATA_MODE_OPUS;
> +
> + if (client_can_celt && snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1, frequency))
> return SPICE_AUDIO_DATA_MODE_CELT_0_5_1;
>
> return SPICE_AUDIO_DATA_MODE_RAW;
> @@ -1199,11 +1222,13 @@ static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
>
> int client_can_celt = red_channel_client_test_remote_cap(playback_channel->base.channel_client,
> SPICE_PLAYBACK_CAP_CELT_0_5_1);
> - int desired_mode = snd_desired_audio_mode(client_can_celt);
> + int client_can_opus = red_channel_client_test_remote_cap(playback_channel->base.channel_client,
> + SPICE_PLAYBACK_CAP_OPUS);
> + int desired_mode = snd_desired_audio_mode(st->frequency, client_can_celt, client_can_opus);
> playback_channel->mode = SPICE_AUDIO_DATA_MODE_RAW;
> if (desired_mode != SPICE_AUDIO_DATA_MODE_RAW)
> {
> - if (snd_codec_create(&playback_channel->codec, desired_mode, SPICE_INTERFACE_PLAYBACK_FREQ, TRUE, FALSE) == SND_CODEC_OK) {
> + if (snd_codec_create(&playback_channel->codec, desired_mode, st->frequency, TRUE, FALSE) == SND_CODEC_OK) {
> playback_channel->mode = desired_mode;
> } else {
> spice_printerr("create encoder failed");
> @@ -1341,6 +1366,52 @@ SPICE_GNUC_VISIBLE uint32_t spice_server_record_get_samples(SpiceRecordInstance
> return len;
> }
>
> +SPICE_GNUC_VISIBLE uint32_t spice_server_get_best_playback_rate(SpicePlaybackInstance *sin)
> +{
> + int client_can_opus = TRUE;
> + if (sin && sin->st->worker.connection)
> + {
> + SndChannel *channel = sin->st->worker.connection;
> + PlaybackChannel *playback_channel = SPICE_CONTAINEROF(channel, PlaybackChannel, base);
> + client_can_opus = red_channel_client_test_remote_cap(playback_channel->base.channel_client,
> + SPICE_PLAYBACK_CAP_OPUS);
> + }
> +
> + if (client_can_opus && snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_OPUS, SND_CODEC_ANY_FREQUENCY))
> + return SND_CODEC_OPUS_PLAYBACK_FREQ;
> +
> + return SND_CODEC_CELT_PLAYBACK_FREQ;
> +}
> +
> +SPICE_GNUC_VISIBLE void spice_server_set_playback_rate(SpicePlaybackInstance *sin, uint32_t frequency)
> +{
> + sin->st->frequency = frequency;
> +}
> +
> +SPICE_GNUC_VISIBLE uint32_t spice_server_get_best_record_rate(SpiceRecordInstance *sin)
> +{
> + int client_can_opus = TRUE;
> + if (sin && sin->st->worker.connection)
> + {
> + SndChannel *channel = sin->st->worker.connection;
> + RecordChannel *record_channel = SPICE_CONTAINEROF(channel, RecordChannel, base);
> + client_can_opus = red_channel_client_test_remote_cap(record_channel->base.channel_client,
> + SPICE_RECORD_CAP_OPUS);
> + }
> +
> + if (client_can_opus && snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_OPUS, SND_CODEC_ANY_FREQUENCY))
Wouldn't it be better to use sin->st->frequency when sin is non-NULL?
> + {
> + return SND_CODEC_OPUS_PLAYBACK_FREQ;
> + }
spice_server_get_best_playback_rate does not have these {}, I'd make them
consistent
> +
> + return SND_CODEC_CELT_PLAYBACK_FREQ;
> +}
> +
> +SPICE_GNUC_VISIBLE void spice_server_set_record_rate(SpiceRecordInstance *sin, uint32_t frequency)
> +{
> + sin->st->frequency = frequency;
> +}
> +
> static void on_new_record_channel(SndWorker *worker)
> {
> RecordChannel *record_channel = (RecordChannel *)worker->connection;
> @@ -1387,18 +1458,7 @@ static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
> return;
> }
>
> - int client_can_celt = red_channel_client_test_remote_cap(record_channel->base.channel_client,
> - SPICE_RECORD_CAP_CELT_0_5_1);
> - int desired_mode = snd_desired_audio_mode(client_can_celt);
> record_channel->mode = SPICE_AUDIO_DATA_MODE_RAW;
> - if (desired_mode != SPICE_AUDIO_DATA_MODE_RAW)
> - {
> - if (snd_codec_create(&record_channel->codec, desired_mode, SPICE_INTERFACE_RECORD_FREQ, FALSE, TRUE) == SND_CODEC_OK) {
> - record_channel->mode = desired_mode;
> - } else {
> - spice_printerr("create decoder failed");
> - }
> - }
>
> worker->connection = &record_channel->base;
>
> @@ -1453,6 +1513,7 @@ void snd_attach_playback(SpicePlaybackInstance *sin)
> sin->st = spice_new0(SpicePlaybackState, 1);
> sin->st->sin = sin;
> playback_worker = &sin->st->worker;
> + sin->st->frequency = SND_CODEC_CELT_PLAYBACK_FREQ; /* Default to the legacy rate */
>
> // TODO: Make RedChannel base of worker? instead of assigning it to channel->data
> channel = red_channel_create_dummy(sizeof(RedChannel), SPICE_CHANNEL_PLAYBACK, 0);
> @@ -1464,8 +1525,10 @@ void snd_attach_playback(SpicePlaybackInstance *sin)
> red_channel_register_client_cbs(channel, &client_cbs);
> red_channel_set_data(channel, playback_worker);
>
> - if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1))
> + if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1, SND_CODEC_ANY_FREQUENCY))
> red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_CELT_0_5_1);
> + if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_OPUS, SND_CODEC_ANY_FREQUENCY))
> + red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_OPUS);
I think I would have delayed this bit until
spice_server_set_playback_rate() is called, but this works this way too.
>
> red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_VOLUME);
>
> @@ -1483,6 +1546,7 @@ void snd_attach_record(SpiceRecordInstance *sin)
> sin->st = spice_new0(SpiceRecordState, 1);
> sin->st->sin = sin;
> record_worker = &sin->st->worker;
> + sin->st->frequency = SND_CODEC_CELT_PLAYBACK_FREQ; /* Default to the legacy rate */
>
> // TODO: Make RedChannel base of worker? instead of assigning it to channel->data
> channel = red_channel_create_dummy(sizeof(RedChannel), SPICE_CHANNEL_RECORD, 0);
> @@ -1493,8 +1557,10 @@ 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 (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1))
> + if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1, SND_CODEC_ANY_FREQUENCY))
> red_channel_set_cap(channel, SPICE_RECORD_CAP_CELT_0_5_1);
> + if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_OPUS, SND_CODEC_ANY_FREQUENCY))
> + red_channel_set_cap(channel, SPICE_RECORD_CAP_OPUS);
> red_channel_set_cap(channel, SPICE_RECORD_CAP_VOLUME);
>
> record_worker->base_channel = channel;
> @@ -1546,9 +1612,12 @@ void snd_set_playback_compression(int on)
> for (; now; now = now->next) {
> if (now->base_channel->type == SPICE_CHANNEL_PLAYBACK && now->connection) {
> PlaybackChannel* playback = (PlaybackChannel*)now->connection;
> - int desired_mode = snd_desired_audio_mode(
> - red_channel_client_test_remote_cap(now->connection->channel_client, SPICE_PLAYBACK_CAP_CELT_0_5_1)
> - );
> + SpicePlaybackState *st = SPICE_CONTAINEROF(now, SpicePlaybackState, worker);
> + int client_can_celt = red_channel_client_test_remote_cap(playback->base.channel_client,
> + SPICE_PLAYBACK_CAP_CELT_0_5_1);
> + int client_can_opus = red_channel_client_test_remote_cap(playback->base.channel_client,
> + SPICE_PLAYBACK_CAP_OPUS);
> + int desired_mode = snd_desired_audio_mode(st->frequency, client_can_opus, client_can_celt);
> if (playback->mode != desired_mode) {
> playback->mode = desired_mode;
> snd_set_command(now->connection, SND_PLAYBACK_MODE_MASK);
> diff --git a/server/spice-server.syms b/server/spice-server.syms
> index 4f2dc37..2193811 100644
> --- a/server/spice-server.syms
> +++ b/server/spice-server.syms
> @@ -145,3 +145,11 @@ SPICE_SERVER_0.12.4 {
> global:
> spice_server_set_agent_file_xfer;
> } SPICE_SERVER_0.12.3;
> +
> +SPICE_SERVER_0.12.5 {
> +global:
> + spice_server_get_best_playback_rate;
> + spice_server_set_playback_rate;
> + spice_server_get_best_record_rate;
> + spice_server_set_record_rate;
> +} SPICE_SERVER_0.12.4;
> diff --git a/server/spice.h b/server/spice.h
> index b645112..c648a1d 100644
> --- a/server/spice.h
> +++ b/server/spice.h
> @@ -24,7 +24,7 @@
> #include <spice/vd_agent.h>
> #include <spice/macros.h>
>
> -#define SPICE_SERVER_VERSION 0x000c04 /* release 0.12.4 */
> +#define SPICE_SERVER_VERSION 0x000c05 /* release 0.12.5 */
>
> #ifdef SPICE_SERVER_INTERNAL
> #undef SPICE_GNUC_DEPRECATED
> @@ -333,7 +333,7 @@ struct SpiceTabletInstance {
>
> #define SPICE_INTERFACE_PLAYBACK "playback"
> #define SPICE_INTERFACE_PLAYBACK_MAJOR 1
> -#define SPICE_INTERFACE_PLAYBACK_MINOR 2
> +#define SPICE_INTERFACE_PLAYBACK_MINOR 3
> typedef struct SpicePlaybackInterface SpicePlaybackInterface;
> typedef struct SpicePlaybackInstance SpicePlaybackInstance;
> typedef struct SpicePlaybackState SpicePlaybackState;
> @@ -342,7 +342,7 @@ enum {
> SPICE_INTERFACE_AUDIO_FMT_S16 = 1,
> };
>
> -#define SPICE_INTERFACE_PLAYBACK_FREQ 44100
> +#define SPICE_INTERFACE_PLAYBACK_FREQ 48000
> #define SPICE_INTERFACE_PLAYBACK_CHAN 2
> #define SPICE_INTERFACE_PLAYBACK_FMT SPICE_INTERFACE_AUDIO_FMT_S16
>
> @@ -367,12 +367,12 @@ void spice_server_playback_set_mute(SpicePlaybackInstance *sin, uint8_t mute);
>
> #define SPICE_INTERFACE_RECORD "record"
> #define SPICE_INTERFACE_RECORD_MAJOR 2
> -#define SPICE_INTERFACE_RECORD_MINOR 2
> +#define SPICE_INTERFACE_RECORD_MINOR 3
> typedef struct SpiceRecordInterface SpiceRecordInterface;
> typedef struct SpiceRecordInstance SpiceRecordInstance;
> typedef struct SpiceRecordState SpiceRecordState;
>
> -#define SPICE_INTERFACE_RECORD_FREQ 44100
> +#define SPICE_INTERFACE_RECORD_FREQ 48000
> #define SPICE_INTERFACE_RECORD_CHAN 2
> #define SPICE_INTERFACE_RECORD_FMT SPICE_INTERFACE_AUDIO_FMT_S16
>
> @@ -393,6 +393,11 @@ void spice_server_record_set_volume(SpiceRecordInstance *sin,
> uint8_t nchannels, uint16_t *volume);
> void spice_server_record_set_mute(SpiceRecordInstance *sin, uint8_t mute);
>
> +uint32_t spice_server_get_best_playback_rate(SpicePlaybackInstance *sin);
> +void spice_server_set_playback_rate(SpicePlaybackInstance *sin, uint32_t frequency);
> +uint32_t spice_server_get_best_record_rate(SpiceRecordInstance *sin);
> +void spice_server_set_record_rate(SpiceRecordInstance *sin, uint32_t frequency);
> +
> /* char device interfaces */
>
> #define SPICE_INTERFACE_CHAR_DEVICE "char_device"
> --
> 1.7.10.4
>
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20131121/63475cd0/attachment.pgp>
More information about the Spice-devel
mailing list