After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 420079 - [audioconvert] Uses biased rounding which results in distortions
[audioconvert] Uses biased rounding which results in distortions
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 0.10.13
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2007-03-19 11:28 UTC by Sebastian Dröge (slomo)
Modified: 2007-03-27 12:46 UTC
See Also:
GNOME target: ---
GNOME version: ---

audioconvert-rounding.patch (1.44 KB, patch)
2007-03-19 11:29 UTC, Sebastian Dröge (slomo)
none Details | Review
audioconvert-rounding.patch (2.55 KB, patch)
2007-03-19 20:23 UTC, Sebastian Dröge (slomo)
none Details | Review
audioconvert-rounding.patch (2.55 KB, patch)
2007-03-19 20:26 UTC, Sebastian Dröge (slomo)
none Details | Review
audioconvert-rounding.patch (8.90 KB, patch)
2007-03-25 10:10 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Sebastian Dröge (slomo) 2007-03-19 11:28:58 UTC
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? :)

Comment 1 Sebastian Dröge (slomo) 2007-03-19 11:29:33 UTC
Created attachment 84873 [details] [review]
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2007-03-19 13:57:41 UTC
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.
Comment 3 Jan Schmidt 2007-03-19 17:25:30 UTC
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.
Comment 4 Sebastian Dröge (slomo) 2007-03-19 19:25:44 UTC
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...
Comment 5 Sebastian Dröge (slomo) 2007-03-19 20:23:37 UTC
Created attachment 84909 [details] [review]

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...
Comment 6 Sebastian Dröge (slomo) 2007-03-19 20:26:41 UTC
Created attachment 84910 [details] [review]

Removed the >> scale for the scale == 0 case
Comment 7 Sebastian Dröge (slomo) 2007-03-22 20:55:45 UTC
Stefan, Jan? Does anyone of you see a problem with this?
Comment 8 Jan Schmidt 2007-03-23 11:22:44 UTC
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.
Comment 9 Sebastian Dröge (slomo) 2007-03-23 12:26:24 UTC
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...
Comment 10 Sebastian Dröge (slomo) 2007-03-25 10:10:54 UTC
Created attachment 85252 [details] [review]

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?
Comment 11 Jan Schmidt 2007-03-26 13:39:18 UTC
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.

Comment 12 Sebastian Dröge (slomo) 2007-03-26 21:03:47 UTC
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
Comment 13 Sebastian Dröge (slomo) 2007-03-27 12:46:20 UTC
Ok, committed...

2007-03-27  Sebastian Dröge  <>

        * 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.