[Spice-devel] [codec 2/spice] Revise the spice server to use the new snd_codec functions in spice-common.

[Christophe Fergeau]

Hey,

On Wed, Oct 16, 2013 at 08:45:31AM -0500, Jeremy White wrote:
> On 10/16/2013 06:05 AM, Christophe Fergeau wrote:
> > This does not seem right, snd_codec_create() hardcodes the numbers of
> > channels to SND_CODEC_CELT_PLAYBACK_CHAN, so we totally ignore whatever was
> > in 'start', but here we use start->channels to compute _frame_bytes.
> 
> Fair enough.  It shouldn't have the appearance of dynamic computation if
> it's not, really.
> 
> With that said, I think one of the issues with the sound code in general
> is that the number of channels and the format are essentially hard
> coded.  There are sporadic asserts to protect that, and sporadic
> appearances of dynamic behavior, but anyone sending a mono track over a
> spice channel is going to be in for a rude shock <grin>.
> 
> My next series makes the frequency dynamic, but leaves the other two
> points hard coded.
> 
> This gave me some heart burn.
> 
> The value of the code being less intrusively changed seemed to outweigh
> rewriting the code to create the appearance of full configuration of
> channels and format.
> 
> I'm willing to be persuaded otherwise.

As said on IRC, I'm not looking for something fully dynamic, but if more of
the number of channel/sample size could be hidden in SndCodec (while still
being hardcoded), this would look nicer imo. But I agree it only makes
sense if it's straightforward to change this.


> 
> >> +AM_CONDITIONAL([HAVE_CELT051], [test "x$have_celt051" = "xyes"])
> >> +AM_COND_IF([HAVE_CELT051], AC_DEFINE([HAVE_CELT051], 1, [Define if
> we have celt051 codec]))
> >
> > Is it needed at all now that you moved the celt logic to spice-common?
> 
> It is needed in the current code (the size of SndCodec depends on the
> config.h matching at each level).  But perhaps that suggests that
> SndCodec should be allocated and freed by spice-common so it's entirely
> opaque.

Ugh, yeah, needing to have config.h to match everywhere seems very fragile,
I'd favour dynamic allocation of SndCodec (iirc I refrained from saying
something about this during the initial review)

> >> -void snd_set_playback_compression(int on)
> >> +void snd_set_playback_compression(int comp)
> >>  {
> >>      SndWorker *now = workers;
> >>  
> >> -    playback_compression = on ? SPICE_AUDIO_DATA_MODE_CELT_0_5_1 : SPICE_AUDIO_DATA_MODE_RAW;
> >> +    playback_compression = comp;
> >> +
> >> +    int desired_mode = snd_desired_audio_mode(TRUE);
> >> +
> >>      for (; now; now = now->next) {
> >>          if (now->base_channel->type == SPICE_CHANNEL_PLAYBACK && now->connection) {
> >> -            SndChannel* sndchannel = now->connection;
> >>              PlaybackChannel* playback = (PlaybackChannel*)now->connection;
> >> -            if (!red_channel_client_test_remote_cap(sndchannel->channel_client,
> >> -                                                    SPICE_PLAYBACK_CAP_CELT_0_5_1)) {
> > 
> > Shouldn't we keep the cap test when desired_mode != SPICE_AUDIO_DATA_MODE_RAW ?
> 
> Yes; my next patch actually does this differently.  This is an error on
> my part caused by my failure to stage the two commits in logically clean
> ways.
> 
> While we're on the subject, though - is there any good way to test this
> code path?

Hmm, I suspect this is dead code. If you look at the old
Spice_user_manual.odt from the RHEL5 era or so (spice-space.org should
have it but is down for me at the moment), in 4.2 Qemu monitor commands,
you'll see there used to be a command to dynamically change playback
compression at runtime. I guess this is when this codepath run. These days,
I think the command is gone.

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