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
Attachment:
pgpm2Xav6r7Vz.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel