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 767505 - audiovisualizer: produces wrong timestamps with non-16 bit audio formats
audiovisualizer: produces wrong timestamps with non-16 bit audio formats
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 17:48 UTC by Thomas Jones
Modified: 2016-06-11 08:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch that should fix the problem (2.40 KB, patch)
2016-06-10 17:48 UTC, Thomas Jones
needs-work Details | Review
Fixed patch (2.40 KB, patch)
2016-06-11 02:22 UTC, Thomas Jones
needs-work Details | Review
Change the variable name (2.40 KB, patch)
2016-06-11 02:25 UTC, Thomas Jones
needs-work Details | Review
REALLY change the variable name (2.35 KB, patch)
2016-06-11 02:37 UTC, Thomas Jones
committed Details | Review

Description Thomas Jones 2016-06-10 17:48:55 UTC
Created attachment 329581 [details] [review]
Patch that should fix the problem

gstaudiovisualizer computes a number of bytes from a number of samples like this:

sbpf = scope->req_spf * channels * sizeof (gint16);

This fails for obvious reasons if the actual audio data isn't 2 bytes per sample, fortunately GstAudioInfo has exactly the information we need to do this calculation correctly.

sbpf = scope->req_spf * GST_AUDIO_INFO_BPF (&scope->ainfo);

This issue is present in all the 1.6 visualizer's too but it didn't matter because the concrete classes that used it all only took 16 bit audio.

Patch attached, but because my machine doesn't have GStreamer 1.8 and I don't really want to go through setting up a sandbox for this I haven't tested it.
Comment 1 Sebastian Dröge (slomo) 2016-06-10 19:08:12 UTC
Review of attachment 329581 [details] [review]:

Seems mostly correct, but can you update the patch so it actually compiles? Thanks :)

::: gst-libs/gst/pbutils/gstaudiovisualizer.c
@@ +1060,3 @@
   rate = GST_AUDIO_INFO_RATE (&scope->ainfo);
   bps = GST_AUDIO_INFO_BPS (&scope->ainfo);
+  bytes_per_samp = GST_AUDIO_INFO_BPF (&scope->ainfo);

Let's call it bpf maybe? :)

@@ +1073,2 @@
   /* this is what we want */
+  sbpf = scope->req_spf bytes_per_samp;

This doesn't compile, you're missing the *
Comment 2 Thomas Jones 2016-06-11 02:22:34 UTC
Created attachment 329600 [details] [review]
Fixed patch

OK, this one should work
Comment 3 Thomas Jones 2016-06-11 02:25:09 UTC
Created attachment 329601 [details] [review]
Change the variable name

Missed your comment on the variable name before, I skimmed a bit. Here's a version with the suggested name
Comment 4 Thomas Jones 2016-06-11 02:37:57 UTC
Created attachment 329602 [details] [review]
REALLY change the variable name

Ooops forgot to commit before running git format-patch, here's one that' actually right
Comment 5 Sebastian Dröge (slomo) 2016-06-11 08:48:29 UTC
commit d423406e0ac87b365f120e6ebe08801bbb1cfe58
Author: Thomas Jones <thomas.jones@utoronto.ca>
Date:   Fri Jun 10 22:36:32 2016 -0400

    audiovisualizer: Fix calculations for bytes<->samples conversions
    
    Use bpf instead of channels * sizeof(gint16).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767505
Comment 6 Sebastian Dröge (slomo) 2016-06-11 08:50:26 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.