GNOME Bugzilla – Bug 356692
wavparse drops final sample in most files
Last modified: 2007-03-02 13:31:28 UTC
When reading a standard PCM file, wavparse usually drops the final sample. It appears to be a rounding error when going from samples -> time -> samples through a convoluted codepath involving segment and seek handling.
Apparently rather the last frame (number-of-channels samples).
Just identified this as a rounding problem (or rather lack thereof). More detailed explanation and maybe a solution follows later.
In gst_wavparse_stream_headers (line 1245): duration = gst_util_uint64_scale_int (wav->datasize, GST_SECOND, wav->bps); In gst_wavparse_perform_seek (line 856): wav->end_offset = gst_util_uint64_scale_int (stop, wav->bps, GST_SECOND); The first one turns the data size into the duration and the second one is supposed to invert that. Of course this does not always work, consider a simple test file with 1024*10 samples in 16-bit integer, 44100 Hz mono format. Then wav->bps will be 88200 and the data size 20480. The complete file size is 20524 (20480 + 44) bytes. Reading such a file with wavparse and then reencoding with wavenc turns this into 20522 bytes. This is the bug, and here is the cause: gst_util_uint64_scale_int (val, num, denom) is just val * num / denom, optionally performing this in 96 bits if that is needed. This means that the combination of the two lines of code cited above computes the equivalent of this expression: 20480 * 10^9 / 88200 * 88200 / 10^9 (which evaluates to 20479!) ^^^^^^^^^^^^^^^^^^^^ \- duration, passed to line 856 as "stop" Two lines later (858), 1 is subtracted from that to make sure we point to the start of a frame, leaving us with 20478. Add 44 to that and you have the broken file size. Solutions: One way to fix this is to modify the mentioned line 858 wav->end_offset -= (wav->end_offset % wav->bytes_per_sample); This currently corrects the offset towards 0 if it does not point to the start of a frame (wav->bytes_per_sample should rather be called wav->bytes_per_frame). Making this add the number of bytes missing to complete the unfinished frame seems to fix the bug (just after that, in line 872, is a safeguard to prevent exceeding the upstream data size even). However, I think it makes more sense to tweak the offset calculation instead. Floor division seems to be inappropriate here, the problem can also be fixed by making either (or both) uses of gst_util_uint64_scale_int a ceil division. This would need new gst utililty functions to get this right (I remember Michael mentioning the lack of these on IRC once). For now, I have a little patch for wavparse that uses this (which is obviously not perfect): #define uint64_scale_int_round_up(val, nom, denom) \ gst_util_uint64_scale_int (val + (denom - 1) / nom, nom, denom)
Created attachment 74648 [details] [review] Ceiling division in duration and offset calculation
Created attachment 76710 [details] [review] Ceiling division in duration and offset calculation (2) The first possible solution I mention in comment #3 is wrong, it would not work for 8-bit mono pcm files. Attached is an improved patch. It does the same thing as the old one, but is more correct. Instead of adding denominator - 1 before dividing (which is awkward to do in conjuction with overflow prevention), this uses the modulo operation (in an overflow safe manner) to find out if the division has a remainder.
2007-03-02 Stefan Kost <ensonic@users.sf.net> Patch by: René Stadler <mail@renestadler.de> * gst/wavparse/gstwavparse.c: (uint64_ceiling_scale_int), (gst_wavparse_perform_seek), (gst_wavparse_stream_headers), (gst_wavparse_stream_data): Handle rounding better to not drop last sample frame. Fixes #356692