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 767507 - audiovisualizer: Timestamp adjustment calculations wrong for >1 channel
audiovisualizer: Timestamp adjustment calculations wrong for >1 channel
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-06-10 18:42 UTC by Thomas Jones
Modified: 2016-06-11 17:53 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
Fix (1.06 KB, patch)
2016-06-11 02:51 UTC, Thomas Jones 		committed Details | Review

Description Thomas Jones 2016-06-10 18:42:05 UTC 
Currently it does this:

ts = gst_adapter_prev_pts (scope->priv->adapter, &dist);
if (GST_CLOCK_TIME_IS_VALID (ts)) {
  dist /= bps;
  ts += gst_util_uint64_scale_int (dist, GST_SECOND, rate);
}

where bps is GST_AUDIO_INFO_BPS (&scope->ainfo) which I *think* is bytes per sample, but that means this calculation is ignoring the number of channels, and if I'm reading the docs for gst_util_uint64_scale_int(). 

So the calculation probably ought to be:

ts = gst_adapter_prev_pts (scope->priv->adapter, &dist);
if (GST_CLOCK_TIME_IS_VALID (ts)) {
  dist /= bps;
  ts += gst_util_uint64_scale_int (dist, GST_SECOND, 
               rate * GST_AUDIO_INFO_BPF (&scope->ainfo));
}

because that will ACTUALLY give us bytes per second.
Comment 1 Sebastian Dröge (slomo) 2016-06-10 19:05:04 UTC 
Seems correct. Can you provide a patch (in "git format-patch" format)? :)
Comment 2 Thomas Jones 2016-06-11 02:51:58 UTC 
Created attachment 329603 [details] [review]
Fix

Actually I just noticed that the my "ought to be" is still wrong it really ought to be this:

ts = gst_adapter_prev_pts (scope->priv->adapter, &dist);
if (GST_CLOCK_TIME_IS_VALID (ts)) {
  ts += gst_util_uint64_scale_int (dist, GST_SECOND, 
               rate * GST_AUDIO_INFO_BPF (&scope->ainfo));
}

Attached a patch
Comment 3 Sebastian Dröge (slomo) 2016-06-11 08:47:54 UTC 
commit 721e415fd203807a2c1b51435b08574bd7b963fc
Author: Thomas Jones <thomas.jones@utoronto.ca>
Date:   Fri Jun 10 22:50:41 2016 -0400

    audiovisualizer: fix timestamp calculation for audio channels > 1
    
    We have to use bps*channels instead of just bps, which is exactly what bpf is for.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767507
Comment 4 Sebastian Dröge (slomo) 2016-06-11 08:50:32 UTC 
Please try to make sure your patches compile :) I fixed up the one for bug #767505 and the one for bug #767507 to compile. bps and channels was unused now, which caused compiler warnings, and a failure because -Werror.
Comment 5 Thomas Jones 2016-06-11 17:53:51 UTC 
(In reply to Sebastian Dröge (slomo) from comment #4)
> Please try to make sure your patches compile :) I fixed up the one for bug
> #767505 and the one for bug #767507 to compile. bps and channels was unused
> now, which caused compiler warnings, and a failure because -Werror.

Sorry, I was copying out of a version copied into my own source code because my system has 1.6 on it and I didn't have things set up for building against an uninstalled master and I don't build with -werror and I didn't see the warnings.

I have got things set up so I can build against master now though so hopefully any future patches will compile :P