Re: audio2
At Wed, 29 May 2019 10:20:38 +0300,
Andreas Gustafsson wrote:
> > Now, you omit the most critical bit: what would you name the thing? :)
> > From "the proper way to do audio scaling" I infer that AUDIO_SCALEDOWN
> > (or however that was spelled) should be ok, shouldn't it?
>
> I'm not fussy about the naming, just about the math :) AUDIO_SCALEDOWN
> works for me, but I'm open to alternatives.
Thank you, riastradh@ and gson@!
AUDIO_SCALEDOWN is self explaining name and good for me, too.
I also think floor() is good for this purpose and truncate() is
also acceptable for alternate.
How about this one?
--- sys/dev/audio/audio.c
+++ sys/dev/audio/audio.c
@@ -458,6 +458,28 @@ audio_track_bufstat(audio_track_t *track, struct
audio_track_debugbuf *buf)
#define SPECIFIED(x) ((x) != ~0)
#define SPECIFIED_CH(x)((x) != (u_char)~0)
+/*
+ * AUDIO_SCALEDOWN()
+ * This macro should be used for audio wave data only.
+ *
+ * The arithmetic shift right (ASR) (in other words, floor()) is good for
+ * this purpose, and will be faster than division on the most platform.
+ * The division (in other words, truncate()) is not so bad alternate for
+ * this purpose, and will be fast enough.
+ * (Using ASR is 1.9 times faster than division on my amd64, and 1.3 times
+ * faster on my m68k. -- isaki 201801.)
+ *
+ * However, the right shift operator ('>>') for negative integer is
+ * "implementation defined" behavior in C (note that it's not "undefined"
+ * behavior). So only if implementation defines '>>' as ASR, we use it.
+ */
+#if defined(__GNUC__)
+/* gcc defines '>>' as ASR. */
+#define AUDIO_SCALEDOWN(value, bits) ((value) >> (bits))
+#else
+#define AUDIO_SCALEDOWN(value, bits) ((value) / (1 << (bits)))
+#endif
+
/* Device timeout in msec */
#define AUDIO_TIMEOUT (3000)
@@ -3194,11 +3216,7 @@ audio_track_chvol(audio_filter_arg_t *arg)
for (ch = 0; ch < channels; ch++) {
aint2_t val;
val = *s++;
-#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && defined(__GNUC__)
- val = val * ch_volume[ch] >> 8;
-#else
- val = val * ch_volume[ch] / 256;
-#endif
+ val = AUDIO_SCALEDOWN(val * ch_volume[ch], 8);
*d++ = (aint_t)val;
}
}
@@ -3220,11 +3238,7 @@ audio_track_chmix_mixLR(audio_filter_arg_t *arg)
d = arg->dst;
for (i = 0; i < arg->count; i++) {
-#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && defined(__GNUC__)
- *d++ = (s[0] >> 1) + (s[1] >> 1);
-#else
- *d++ = (s[0] / 2) + (s[1] / 2);
-#endif
+ *d++ = AUDIO_SCALEDOWN(s[0], 1) + AUDIO_SCALEDOWN(s[1], 1);
s += arg->srcfmt->channels;
}
}
@@ -5021,11 +5035,7 @@ audio_pmixer_process(struct audio_softc *sc)
if (vol != 256) {
m = mixer->mixsample;
for (i = 0; i < sample_count; i++) {
-#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && defined(__GNUC__)
- *m = *m * vol >> 8;
-#else
- *m = *m * vol / 256;
-#endif
+ *m = AUDIO_SCALEDOWN(*m * vol, 8);
m++;
}
}
@@ -5113,11 +5123,9 @@ audio_pmixer_mix_track(audio_trackmixer_t *mixer,
audio_track_t *track,
#if defined(AUDIO_SUPPORT_TRACK_VOLUME)
if (track->volume != 256) {
for (i = 0; i < sample_count; i++) {
-#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && defined(__GNUC__)
- *d++ = ((aint2_t)*s++) * track->volume >> 8;
-#else
- *d++ = ((aint2_t)*s++) * track->volume / 256;
-#endif
+ aint2_t v;
+ v = *s++;
+ *d++ = AUDIO_SCALEDOWN(v * track->volume, 8)
}
} else
#endif
@@ -5131,11 +5139,9 @@ audio_pmixer_mix_track(audio_trackmixer_t *mixer,
audio_track_t *track,
#if defined(AUDIO_SUPPORT_TRACK_VOLUME)
if (track->volume != 256) {
for (i = 0; i < sample_count; i++) {
-#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && defined(__GNUC__)
- *d++ += ((aint2_t)*s++) * track->volume >> 8;
-#else
- *d++ += ((aint2_t)*s++) * track->volume / 256;
-#endif
+ aint2_t v;
+ v = *s++;
+ *d++ += AUDIO_SCALEDOWN(v * track->volume, 8);
}
} else
#endif
--- sys/dev/audio/audiodef.h
+++ sys/dev/audio/audiodef.h
@@ -63,12 +63,6 @@
*/
/* #define AUDIO_SUPPORT_TRACK_VOLUME */
-/*
- * Whether use C language's "implementation defined" behavior (note that
-
Re: audio2
Valery Ushakov wrote: > Now, you omit the most critical bit: what would you name the thing? :) > From "the proper way to do audio scaling" I infer that AUDIO_SCALEDOWN > (or however that was spelled) should be ok, shouldn't it? I'm not fussy about the naming, just about the math :) AUDIO_SCALEDOWN works for me, but I'm open to alternatives. -- Andreas Gustafsson, g...@gson.org
Re: audio2
On Tue, May 28, 2019 at 20:25:33 +0300, Andreas Gustafsson wrote: > On May 23, Valery Ushakov wrote: > > This feels inverted. The mathematically correct operation required > > here is symmetric division (that rounds towards zero). > > Rounding towards zero is absolutely the wrong thing to do for audio, > because it introduces a discontinuity, which in turn causes > unnecessary distortion of the audio. > > For example, if your original signal is a linear ramp > > ... -5, -4, -3, -2, -1, 0, 1, 2, 3, 4, 5 ... > > and you divide it by two rounding towards zero, you get > > ... -2, -2, -1, -1, 0, 0, 0, 1, 1, 2, 2 ... > > where every value is repeated twice, except that the value zero occurs > three times, so the ramp is no longer linear - it now has a visible > and audible kink in the middle. And this kink is in the worst > possible place of the amplitude scale, at zero where it will affect > even the weakest signals, distorting every zero crossing. > > The proper way to do audio scaling is using dither, but failing that, > at least the values should be consistently rounded either up or down, > which will at worst introduce a small DC offset which you can't hear, > but no crossover distortion. Thanks a lot for the explanation! Now, you omit the most critical bit: what would you name the thing? :) >From "the proper way to do audio scaling" I infer that AUDIO_SCALEDOWN (or however that was spelled) should be ok, shouldn't it? -uwe
Re: audio2
> Date: Mon, 27 May 2019 22:28:37 +0900 > From: Tetsuya Isaki > > At Sat, 25 May 2019 18:01:11 +0300, > Valery Ushakov wrote: > > My point is exactly that you are confusing implementation detail that > > uses right shift as a good enough implementation (which, I reiterate, > > I don't object to here) and what is the meaning of the operation that > > you are implementing/approximating (see my first paragraph above). > > I'm sorry, I don't understand (I can't parse) this paragraph > due to my poor English skill. Would you write it again? > > And anyway I don't understand your point well. Can you show me > your proposal patch? Hi, Isaki-san! As I understand it, uwe is just asking you to use a different name, because `AUDIO_ASR' suggests only one particular approximation, and it's not even the one that the code always uses. Perhaps `AUDIO_SCALEDOWN' would be clearer? In more detail: - Mathematically, the function is used to scale an amplitude x down by 2^n: f_n(x) = x/2^n. This function has the property that f_n(-x) = -f_n(x), i.e. f_n is an odd function. - In digital audio, we approximate this real-valued function f_n(x) with integer arithmetic. We have a couple choices for how to do this: . truncate(x/2^n), which is also an odd function. . floor(x/2^n), which is _not_ an odd function, and so is maybe not as good an approximation. On platforms where floor(x/2^n) can be computed more efficiently, by doing x >> n, than truncate(x/2^n), i.e. x/(1 << n), then that's OK: being off by one in this approximation, under negation, is not very bad. - Whichever approximation we choose, we should _name_ the function for the mathematical operation it approximates, which is scaling an amplitude down, not for one of the approximations it _might_ use. Calling it AUDIO_ASR seems wrong because: . the main purpose is to use _some_ approximation to scale an amplitude down -- it is an implementation detail that it sometimes approximates the mathematical function by a right shift; . the function doesn't necessarily always compute floor(x/2^n) -- that is, the function you have named AUDIO_ASR doesn't necessarily shift right. Maybe we could call it AUDIO_SCALEDOWN or something instead?
Re: audio2
At Sat, 25 May 2019 18:01:11 +0300,
Valery Ushakov wrote:
> Speaking in the abstract: The operation that happens here is
> rescaling, as far as I understand. Yes, the two-complement ranges are
> lopsided with an extra value on the negative side, but that is far
> less important, I think, than commuting with negation. If you negate
> all samples, you have basically the same audio
> https://manual.audacityteam.org/man/invert.html ("invert" is a bit
> unfortuante as a name b/c of the confusion with the bitwise
> operation), so it's nice to have:
>
> -scaleN(sample) = scaleN(-sample)
Partially, yes. If I make an userland offline waveform editing
software on modern arch, and the output wave can be used as another
input, I may do so.
But this is in-kernel online(realtime) processing including m68k and
the output is final stage that human hear.
> > The correct operation is not exist whenever rounding to integer
> > occurs.
> >
> > And in audio area, we need to understand that both rounding
> > are not correct but are acceptable.
>
> I think you are confusing correctness and precision here.
What is your correctness and precision here?
> My point is exactly that you are confusing implementation detail that
> uses right shift as a good enough implementation (which, I reiterate,
> I don't object to here) and what is the meaning of the operation that
> you are implementing/approximating (see my first paragraph above).
I'm sorry, I don't understand (I can't parse) this paragraph
due to my poor English skill. Would you write it again?
And anyway I don't understand your point well. Can you show me
your proposal patch?
Thanks,
---
Tetsuya Isaki
Re: audio2
On Fri, May 24, 2019 at 21:51:35 +0900, Tetsuya Isaki wrote:
> At Thu, 23 May 2019 17:08:42 +0300,
> Valery Ushakov wrote:
> > > +#if defined(__GNUC__)
> > > +/* gcc defines '>>' as ASR. */
> > > +#define AUDIO_ASR(value, shift) ((value) >> (shift))
> > > +#else
> > > +#define AUDIO_ASR(value, shift) ((value) / (1 << (shift)))
> > > +#endif
> >
> > This feels inverted. The mathematically correct operation required
> > here is symmetric division (that rounds towards zero). Arithmetic
> > shift is flooring division (that rounds towards minus infinity).
>
> Both are not correct but are acceptable.
>
> # I also used to feel that division is the correct operation.
> # But I don't think so now.
>
> For example, let's consider to convert 16bits to 15bits.
> In case of round-to-zero,
> 32767, 32766 => 16383
> :
> 3, 2 => 1
> 1, 0 => 0 < *1
> -1 => 0 < *1
> -2, -3 => -1
> :
> -32766, -32767 => -16383
> -32768 => -16384 < *2
>
> In case of round-to-minus-inf,
> 32767, 32766 => 16383
> :
> 3, 2 => 1
> 1, 0 => 0
> -1, -2 => -1
> :
> -32767, -32768 => -16384
>
> As above, in round-to-zero, there are three inputs that output 0
> (*1) and there are only one input that output -16384 (*2).
> In contrast, in round-to-minus-inf, all outputs correspond to two
> input values.
>
> Which is "correct"?
Speaking in the abstract: The operation that happens here is
rescaling, as far as I understand. Yes, the two-complement ranges are
lopsided with an extra value on the negative side, but that is far
less important, I think, than commuting with negation. If you negate
all samples, you have basically the same audio
https://manual.audacityteam.org/man/invert.html ("invert" is a bit
unfortuante as a name b/c of the confusion with the bitwise
operation), so it's nice to have:
-scaleN(sample) = scaleN(-sample)
> The correct operation is not exist whenever rounding to integer
> occurs.
>
> And in audio area, we need to understand that both rounding
> are not correct but are acceptable.
I think you are confusing correctness and precision here.
> Furthermore, we have to consider that this operation is performed
> in the bottom of loop in realtime mixing. If both rounding are
> acceptable, I'd like to use faster one. Of course, unless it is
> "undefined".
>
> > So it feels confusing and wrong to *name* the operation the
> > opposite of what it really is.
>
> What I want to do here is arithmetic right shift operation and 2nd
> argument in this macro indicates shift count. So I named it ASR.
My point is exactly that you are confusing implementation detail that
uses right shift as a good enough implementation (which, I reiterate,
I don't object to here) and what is the meaning of the operation that
you are implementing/approximating (see my first paragraph above).
-uwe
Re: audio2
At Fri, 24 May 2019 07:01:56 +, m...@netbsd.org wrote: > > we don't have anyone using the non-__GNUC__ case This is C specification matter. It's not important whether such compiler really exist. Thanks, --- Tetsuya Isaki
Re: audio2
At Thu, 23 May 2019 17:08:42 +0300, Valery Ushakov wrote: > > +#if defined(__GNUC__) > > +/* gcc defines '>>' as ASR. */ > > +#define AUDIO_ASR(value, shift)((value) >> (shift)) > > +#else > > +#define AUDIO_ASR(value, shift)((value) / (1 << (shift))) > > +#endif > > This feels inverted. The mathematically correct operation required > here is symmetric division (that rounds towards zero). Arithmetic > shift is flooring division (that rounds towards minus infinity). Both are not correct but are acceptable. # I also used to feel that division is the correct operation. # But I don't think so now. For example, let's consider to convert 16bits to 15bits. In case of round-to-zero, 32767, 32766 => 16383 : 3, 2 => 1 1, 0 => 0 < *1 -1 => 0 < *1 -2, -3 => -1 : -32766, -32767 => -16383 -32768 => -16384 < *2 In case of round-to-minus-inf, 32767, 32766 => 16383 : 3, 2 => 1 1, 0 => 0 -1, -2 => -1 : -32767, -32768 => -16384 As above, in round-to-zero, there are three inputs that output 0 (*1) and there are only one input that output -16384 (*2). In contrast, in round-to-minus-inf, all outputs correspond to two input values. Which is "correct"? The correct operation is not exist whenever rounding to integer occurs. And in audio area, we need to understand that both rounding are not correct but are acceptable. Furthermore, we have to consider that this operation is performed in the bottom of loop in realtime mixing. If both rounding are acceptable, I'd like to use faster one. Of course, unless it is "undefined". > So > it feels confusing and wrong to *name* the operation the opposite of > what it really is. What I want to do here is arithmetic right shift operation and 2nd argument in this macro indicates shift count. So I named it ASR. Thanks, --- Tetsuya Isaki
Re: audio2
we don't have anyone using the non-__GNUC__ case
Re: audio2
On Thu, May 23, 2019 at 21:28:51 +0900, Tetsuya Isaki wrote:
> +/*
> + * AUDIO_ASR() does Arithmetic Shift Right operation.
> + * This macro should be used for audio wave data only.
> + *
> + * Division by power of two is replaced with shift operation in the most
> + * compiler, but even then rounding-to-zero occurs on negative value.
> + * What we handle here is the audio wave data that human hear, so we can
> + * ignore the rounding difference. Therefore we want to use faster
> + * arithmetic shift right operation. But the right shift operator ('>>')
> + * for negative integer is "implementation defined" behavior in C (note
> + * that it's not "undefined" behavior). So if implementation defines '>>'
> + * as ASR, we use it.
> + *
> + * Using ASR is 1.9 times faster than division on my amd64, and 1.3 times
> + * faster on my m68k. -- isaki 201801.
> + */
> +#if defined(__GNUC__)
> +/* gcc defines '>>' as ASR. */
> +#define AUDIO_ASR(value, shift) ((value) >> (shift))
> +#else
> +#define AUDIO_ASR(value, shift) ((value) / (1 << (shift)))
> +#endif
This feels inverted. The mathematically correct operation required
here is symmetric division (that rounds towards zero). Arithmetic
shift is flooring division (that rounds towards minus infinity). So
it feels confusing and wrong to *name* the operation the opposite of
what it really is.
-uwe
Re: audio2
At Wed, 8 May 2019 18:14:28 +,
m...@netbsd.org wrote:
> Is there some scenario where this made a difference?
> it sounds like an optimization that a compiler should already do
>
> 5234:#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) &&
> defined(__GNUC__)
> 5235- *d++ += ((aint2_t)*s++) * track->volume >> 8;
> 5236-#else
> 5237- *d++ += ((aint2_t)*s++) * track->volume / 256;
> 5238-#endif
It's signed integer so these two output different code.
Here is the fragment of amd64 asm of "(x * vol >> 8)"
96a5: 0f af d1imul %ecx,%edx
96a8: c1 fa 08sar$0x8,%edx
96ab: 66 89 14 07 mov%dx,(%rdi,%rax,1)
Here is "(x * vol / 256)"
96a5: 0f af c1imul %ecx,%eax
96a8: 85 c0 test %eax,%eax
96aa: 79 05 jns96b1
96ac: 05 ff 00 00 00 add$0xff,%eax
96b1: c1 f8 08sar$0x8,%eax
96b4: 66 89 04 17 mov%ax,(%rdi,%rdx,1)
I evaluated these (a year ago). The former is 1.9 times faster on
my old amd64 than the later, 1.3 times faster on m68k, at that point.
# Although, above code is from current but evaluation is a year ago.
Let's consider about (-1 / 2). The mathematical answer is -0.5 so
it's 0 for integers. And (-1 >> 1) is -1. These two answers are
different but this value is audio wave that human hear. I took
performance.
However, I understand that my code looks strange.
How about this?
Index: sys/dev/audio/audio.c
===
RCS file: /cvsroot/src/sys/dev/audio/audio.c,v
retrieving revision 1.8
diff -u -r1.8 audio.c
--- sys/dev/audio/audio.c 21 May 2019 12:52:57 - 1.8
+++ sys/dev/audio/audio.c 23 May 2019 03:31:36 -
@@ -465,6 +465,29 @@
#define SPECIFIED(x) ((x) != ~0)
#define SPECIFIED_CH(x)((x) != (u_char)~0)
+/*
+ * AUDIO_ASR() does Arithmetic Shift Right operation.
+ * This macro should be used for audio wave data only.
+ *
+ * Division by power of two is replaced with shift operation in the most
+ * compiler, but even then rounding-to-zero occurs on negative value.
+ * What we handle here is the audio wave data that human hear, so we can
+ * ignore the rounding difference. Therefore we want to use faster
+ * arithmetic shift right operation. But the right shift operator ('>>')
+ * for negative integer is "implementation defined" behavior in C (note
+ * that it's not "undefined" behavior). So if implementation defines '>>'
+ * as ASR, we use it.
+ *
+ * Using ASR is 1.9 times faster than division on my amd64, and 1.3 times
+ * faster on my m68k. -- isaki 201801.
+ */
+#if defined(__GNUC__)
+/* gcc defines '>>' as ASR. */
+#define AUDIO_ASR(value, shift)((value) >> (shift))
+#else
+#define AUDIO_ASR(value, shift)((value) / (1 << (shift)))
+#endif
+
/* Device timeout in msec */
#define AUDIO_TIMEOUT (3000)
@@ -3304,11 +3327,7 @@
for (ch = 0; ch < channels; ch++) {
aint2_t val;
val = *s++;
-#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && defined(__GNUC__)
- val = val * ch_volume[ch] >> 8;
-#else
- val = val * ch_volume[ch] / 256;
-#endif
+ val = AUDIO_ASR(val * ch_volume[ch], 8);
*d++ = (aint_t)val;
}
}
@@ -3330,11 +3349,7 @@
d = arg->dst;
for (i = 0; i < arg->count; i++) {
-#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && defined(__GNUC__)
- *d++ = (s[0] >> 1) + (s[1] >> 1);
-#else
- *d++ = (s[0] / 2) + (s[1] / 2);
-#endif
+ *d++ = AUDIO_ASR(s[0], 1) + AUDIO_ASR(s[1], 1);
s += arg->srcfmt->channels;
}
}
@@ -5131,11 +5146,7 @@
if (vol != 256) {
m = mixer->mixsample;
for (i = 0; i < sample_count; i++) {
-#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && defined(__GNUC__)
- *m = *m * vol >> 8;
-#else
- *m = *m * vol / 256;
-#endif
+ *m = AUDIO_ASR(*m * vol, 8);
m++;
}
}
@@ -5223,11 +5234,9 @@
#if defined(AUDIO_SUPPORT_TRACK_VOLUME)
if (track->volume != 256) {
for (i = 0; i < sample_count; i++) {
-#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && defined(__GNUC__)
- *d++ = ((aint2_t)*s++) * track->volume >> 8;
-#else
- *d++ = ((aint2_t)*s++) * track->volume / 256;
-#endif
+ aint2_t v;
+ v = *s++;
+
Re: audio2
At Sat, 11 May 2019 00:36:42 +, m...@netbsd.org wrote: > - ioctl AUDIO_GETFD/SETFD, AUDIO_GETCHAN/SETCHAN are obsoleted. > ossaudio is still using this, although I don't know if it's critical. > > compat/ossaudio/ossaudio.c:779 > > case OSS_SNDCTL_DSP_SETDUPLEX: > idat = 1; > error = ioctlf(fp, AUDIO_SETFD, ); I think there is no problem. In the past, AUDIO_SETFD used to call hw_if->setfd() in the internal but there was no driver which implemented setfd(). That is, it was available but did nothing. In audio2, AUDIO_SETFD is marked obsolete and does nothing. Thanks, --- Tetsuya Isaki
Re: audio2
You are correct, my mistake! On Tue, 21 May 2019, Tetsuya Isaki wrote: At Thu, 9 May 2019 10:28:19 -0300 (ADT), Jared McNeill wrote: With order of operations here, and track->volume being in range 0...256, I don't think this will work anyway. volume of 255 of less will cause the sample to be 0, and 256 the original value. (A * B >> C) is equivalent to ((A * B) >> C). (A * B / C) is equivalent to ((A * B) / C). And in fact it worked. # I'm sorry if I misread your text. Thanks, --- Tetsuya Isaki 5234:#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && defined(__GNUC__) 5235- *d++ += ((aint2_t)*s++) * track->volume >> 8; 5236-#else 5237- *d++ += ((aint2_t)*s++) * track->volume / 256; 5238-#endif
Re: audio2
At Thu, 9 May 2019 10:28:19 -0300 (ADT), Jared McNeill wrote: > With order of operations here, and track->volume being in range 0...256, I > don't think this will work anyway. volume of 255 of less will cause the > sample to be 0, and 256 the original value. (A * B >> C) is equivalent to ((A * B) >> C). (A * B / C) is equivalent to ((A * B) / C). And in fact it worked. # I'm sorry if I misread your text. Thanks, --- Tetsuya Isaki > > 5234:#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && > > defined(__GNUC__) > > 5235- *d++ += ((aint2_t)*s++) * track->volume > > >> 8; > > 5236-#else > > 5237- *d++ += ((aint2_t)*s++) * track->volume > > / 256; > > 5238-#endif
Re: audio2
With order of operations here, and track->volume being in range 0...256, I don't think this will work anyway. volume of 255 of less will cause the sample to be 0, and 256 the original value. On Wed, 8 May 2019, m...@netbsd.org wrote: Is there some scenario where this made a difference? it sounds like an optimization that a compiler should already do 5234:#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && defined(__GNUC__) 5235- *d++ += ((aint2_t)*s++) * track->volume >> 8; 5236-#else 5237- *d++ += ((aint2_t)*s++) * track->volume / 256; 5238-#endif
Re: audio2
Is there some scenario where this made a difference? it sounds like an optimization that a compiler should already do 5234:#if defined(AUDIO_USE_C_IMPLEMENTATION_DEFINED_BEHAVIOR) && defined(__GNUC__) 5235- *d++ += ((aint2_t)*s++) * track->volume >> 8; 5236-#else 5237- *d++ += ((aint2_t)*s++) * track->volume / 256; 5238-#endif
Re: CVS commit: [isaki-audio2] src/sys/dev/usb
At Thu, 02 May 2019 10:46:14 +1000,
matthew green wrote:
> > Committed By: isaki
> > Date: Wed May 1 13:09:34 UTC 2019
> >
> > Modified Files:
> > src/sys/dev/usb [isaki-audio2]: uaudio.c
> >
> > Log Message:
> > Don't release sc_lock and sc_intr_lock in trigger_{input,output}.
> > In the past, sc_lock was IPL_SCHED and (probably) it had conflicted
> > with usb subroutines. But at some point, sc_lock has changed to use
> > IPL_SOFTUSB so such problems should been gone.
>
> sc_lock was ever IPL_SCHED. only sc_intr_lock has been
> that in uaudio.
Oh, it's typo... it's sc_intr_lock.
> what are you trying to avoid here? the problem this
> solved was not always a problem, but eg:
>
> - drop the audio thread lock when calling into usb when it may sleep,
> avoiding a deadlock between audiowrite and audioioctl. this fixes
> mixerctl -a vs. playing hanging the system
> XXX probably need to check this in a bunch more places.
>
> is the commit related to that change. can you confirm
> that you can be playing and run mixerctl -a at the same
> time with your change in place?
Yes, I can run mixerctl -a or mixerctl -w while playing. But
I think it's not related because my change is about trigger_*
and trigger_* is not called during playback/recording running.
What I tried to solve is rev1.148-1.149 which releases locks
(unnecessarily, I think).
trigger_* is called with sc_lock and sc_intr_lock held, so
it's (basically) strange to release such locks (Of course
I know it's necessary in some cases).
rev1.148-1.149 releases both of sc_lock and sc_intr_lock to
call uaudio_chan_open(), uaudio_chan_alloc_buffers() and
uaudio_chan_ptransfer(). But I've read(traveled) source codes
of these three USB functions and I believe that these can work
with IPL_SOFTUSB/IPL_NONE held (in other words, releasing locks
here is unnecessary).
Thanks,
---
Tetsuya Isaki
re: CVS commit: [isaki-audio2] src/sys/dev/usb
"Tetsuya Isaki" writes:
> Module Name: src
> Committed By: isaki
> Date: Wed May 1 13:09:34 UTC 2019
>
> Modified Files:
> src/sys/dev/usb [isaki-audio2]: uaudio.c
>
> Log Message:
> Don't release sc_lock and sc_intr_lock in trigger_{input,output}.
> In the past, sc_lock was IPL_SCHED and (probably) it had conflicted
> with usb subroutines. But at some point, sc_lock has changed to use
> IPL_SOFTUSB so such problems should been gone.
sc_lock was ever IPL_SCHED. only sc_intr_lock has been
that in uaudio.
what are you trying to avoid here? the problem this
solved was not always a problem, but eg:
- drop the audio thread lock when calling into usb when it may sleep,
avoiding a deadlock between audiowrite and audioioctl. this fixes
mixerctl -a vs. playing hanging the system
XXX probably need to check this in a bunch more places.
is the commit related to that change. can you confirm
that you can be playing and run mixerctl -a at the same
time with your change in place?
thanks.
.mrg.
