[gstreamer-bugs] [Bug 356692] wavparse drops final sample in most files

[GStreamer (bugzilla.gnome.org)]

Do not reply to this via email (we are currently unable to handle email
responses and they get discarded).  You can add comments to this bug at
http://bugzilla.gnome.org/show_bug.cgi?id=356692

  GStreamer | gst-plugins-good | Ver: HEAD CVS





------- Comment #3 from René Stadler  2006-10-13 17:56 UTC -------
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)


-- 
Configure bugmail: http://bugzilla.gnome.org/userprefs.cgi?tab=email