[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 /

From: malc
Subject: Re: [Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal
Date: Mon, 14 Apr 2008 20:49:31 +0400 (MSD)

On Mon, 14 Apr 2008, Jan Kiszka wrote:

malc wrote:
On Sun, 13 Apr 2008, Jan Kiszka wrote:


+static void sound_fill_mixer_buffer(mv88w8618_sound_state *s,
unsigned int length)
+    unsigned int pos;
+    double val;
+    if (s->mute) {
+        memset(s->mixer_buffer, 0, length);

Zero is not correct "mute" value for signed formats.

Hmm, maybe it's still too early for me - but what else??

Well.. 0x80 for S8 and 0x8000 for S16 (audio_pcm_info_clear_buf in

+        return;
+    }
+    if (s->playback_mode & 1)
+        for (pos = 0; pos < length; pos += 2) {
+            val = *(int16_t *)(s->target_buffer + s->play_pos + pos);
+            val = val * pow(10.0, s->volume/20.0);
+            *(int16_t *)(s->mixer_buffer + pos) = val;
+        }

On the surface it's sounds like endianess problems are ahead.

Oh, yes.

+    else
+        for (pos = 0; pos < length; pos += 1) {
+            val = *(int8_t *)(s->target_buffer + s->play_pos + pos);
+            val = val * pow(10.0, s->volume/20.0);
+            *(int8_t *)(s->mixer_buffer + pos) = val;
+        }

There's actually a generic framework for volume manipulation in QEMUs
audio, unfortunatelly it's unconditionally disabled (define NOVOL in
audio/mixeng.c) for the reasons i can't recall now.

Yeah, looked around a bit before hacking those loops but indeed found
nothing ready to use.

It's just a question of adding(and using) AUD_set_volume[_in/out],
which will set vol field of respective soft voice and removing
uncoditional #define NOVOL from mixeng.c.

Also adlib emulation is only conditionally available due to
usage of floating point arithmetics, maybe rules have changed now
though, but "fixing" this particular issue in this particular case
seems easy enough.

Sorry, didn't get what you mean here.

Adlib is not built by default, requires explicit --enable-adlib switch
to configure, because fmopl.c uses FPU and Fabrice didn't like the idea.
Fixing it here is just a matter of switching to fixed point.

Then again if volume manipulation in main audio proper are brought up
to date this all can be scrapped and it will also "magically" solve the
endianness issue.

+static void sound_callback(void *opaque, int free)
+    mv88w8618_sound_state *s = opaque;
+    uint32_t old_status = s->status;
+    int64_t now = qemu_get_clock(rt_clock);
+    unsigned int n;
+    if (now - s->last_callback < (1000 * s->threshold/2) / (44100*2))
+        return;
+    s->last_callback = now;

Here two clocks are being used for things - the audio one which influences
`free' paramter and rt_clock, even if there are reasons why free could not
be used choice of rt_clock over vm_clock seems wrong.

Yes, using the monotonic vm_clock is better. Will change.

Why is it needed at all? `free' already supplies you with an audio clock


Thanks a lot for your review!

You are welcome.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]