GNOME Bugzilla – Bug 420079
[audioconvert] Uses biased rounding which results in distortions
Last modified: 2007-03-27 12:46:20 UTC
Hi, audioconvert currently uses a biased rounding function, that will always round to the smaller integer. The attached patch fixes this (with special care for integer overflows) and results on one of my samples in ~3-5 db less noise. Could someone please review the patch for sanity? :) Bye
Created attachment 84873 [details] [review] audioconvert-rounding.patch
for performance reasons I would like to see this refactored (scale can't change in the loop) for (;count; count--) { \ tmp = *src++; \ if (scale > 0 && tmp > 0 && G_MAXINT32 - tmp < (1<<(scale-1))) \ tmp = G_MAXINT32; \ else if (scale > 0) \ tmp += 1<<(scale-1); \ tmp = (tmp ^ (sign)) >> scale; \ WRITE_FUNC (p, tmp); \ p+=stride; \ } \ if(scale>0) { \ for (;count; count--) { \ tmp = *src++; \ if (tmp > 0 && G_MAXINT32 - tmp < (1<<(scale-1))) \ tmp = G_MAXINT32; \ else \ tmp += 1<<(scale-1); \ tmp = (tmp ^ (sign)) >> scale; \ WRITE_FUNC (p, tmp); \ p+=stride; \ } \ } \ else { \ for (;count; count--) { \ tmp = *src++; \ tmp = (tmp ^ (sign)) >> scale; \ WRITE_FUNC (p, tmp); \ p+=stride; \ } \ } maybe also moving (1<<(scale-1)) out of the loop and assigning it to a nicly named variable.
when scale == 0 in the else branch, the ">> scale" can be skipped too... Computing 1 << (scale-1) outside the loop is probably worth it too, as you say. Also, why the check that tmp > 0 in the comparison? it's a guint32, so it can never be negative, and 0 itself doesn't seem harmful in the "G_MAXINT32 - tmp < (1<<(scale-1))" part.
Well, that tmp > 0 comparison is "necessary"... For some reason I thought tmp was a gint32. In that case I have to check if it's greater 0 before doing the "overflow check"... otherwise it will overflow in the overflow check. I have a version locally that your two ideas implemented... but I believe I should add another local variable for that logic that is signed. I have actually no idea why the result was the same as if the variable was signed ;) I'll investigate and attach a new patch later...
Created attachment 84909 [details] [review] audioconvert-rounding.patch Ok, this is how I planned to do it from the beginning with a signed temporary variable. If I do it all on the unsigned tmp how I wanted to do it, it would become hard to understand and I would need another else if in there as I must check if I'm below 0x7fffffff (i.e. G_MAXINT32) and would get over 0x7ffffff (i.e. must set 0x7fffffff) and also must check if I could get over 0xffffffff. Please tell me if I'm missing something here...
Created attachment 84910 [details] [review] audioconvert-rounding.patch Removed the >> scale for the scale == 0 case
Stefan, Jan? Does anyone of you see a problem with this?
Yes, I do - I don't understand the old code well enough to be sure your new code does the same thing. In particular, does it work correctly when handling unsigned output formats? I'm not convinced - I think we need some test cases.
Ok, I'll add some unit tests for this and specifically test unsigned output formats and make sure it works from all to all formats...
Created attachment 85252 [details] [review] audioconvert-rounding.patch Ok, here is a finally correct version with fixed and new unit tests for checking the rounding. First of all, it doesn't matter at all if we have the temporary variable signed or unsigned in the original code, thus I changed from guint32 to gint32 as I need signed values for the rounding and this would otherwise require another new variable. Then the rounding that was implemented before did: -1.2 => -2 1.2 => 1 -1.5 => -2 1.5 => 1 -1.7 => -2 1.7 => 1 The rounding that is implemented now in this patch does: -1.2 => -1 1.2 => 1 -1.5 => -2 1.5 => 2 -1.7 => -2 1.7 => 2 I added two unit tests that check this for signed and unsigned targets, two unit test for 16 bit width/depth <-> 8 bit depth, 16 bit width signed and unsigned and one for testing 16 bit signed <-> unsigned. Then I had to fix one of the unit tests as it relied on the old rounding. Does this look fine for you, Jan?
Yep, this one looks much more obviously correct, and having tests in the testsuite for it is awesome. I don't know if it's worth splitting the code into signed & unsigned versions now that there's a branch based on the sign flag, but that's an optimisation step - I'm happy to see your patch committed as is.
Ok, I'll commit it tomorrow... thanks :) The branch based on the sign flag should be optimized away if sign == 0 by a sane compiler and for sign != 0 the sign check should be removed and only for < 0 the check would be done. But apart from that it's probably a good idea to split signed and unsigned functions... added it somewhere on my todo list
Ok, committed... 2007-03-27 Sebastian Dröge <slomo@circular-chaos.org> * gst/audioconvert/audioconvert.c: Add docs to the integer pack functions and implement proper rounding. Before we had rounding towards negative infinity, i.e. always the smaller number was taken. Now we use natural rounding, i.e. rounding to the nearest integer and to the one with the largest absolute value for X.5. The old rounding introduced some minor distortions. Fixes #420079 * tests/check/elements/audioconvert.c: (GST_START_TEST): Fix one unit test that assumed the old rounding and added unit tests for checking signed/unsigned int16 <-> signed/unsigned int16 with depth 8, one for signed int16 <-> unsigned int16 and one for the new rounding from signed int32 to signed/unsigned int16.