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 325504 - [flacdec] gst_flac_dec_convert_src [mis]uses g_assert
[flacdec] gst_flac_dec_convert_src [mis]uses g_assert
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal normal
: 0.10.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2006-01-02 12:40 UTC by Alessandro Decina
Modified: 2006-01-02 19:40 UTC
See Also:
GNOME target: ---
GNOME version: 2.13/2.14

fix (1.02 KB, patch)
2006-01-02 12:43 UTC, Alessandro Decina
needs-work Details | Review
add explicit checks (2.03 KB, patch)
2006-01-02 19:02 UTC, Alessandro Decina
committed Details | Review

Description Alessandro Decina 2006-01-02 12:40:18 UTC
Please describe the problem:
gst_flac_dec_convert_src uses g_assert to assert conditions which can be, and
actually are, better handled by simply returning FALSE.

Steps to reproduce:
Using rhythmbox (cvs) with gstreamer (cvs) to play flac files.
It seems like rhythmbox does the first query_position before the first frame has
been decoded.

Actual results:
Rhythmbox is aborted

Expected results:

Does this happen every time?

Other information:
Comment 1 Alessandro Decina 2006-01-02 12:43:36 UTC
Created attachment 56662 [details] [review]
Comment 2 Tim-Philipp Müller 2006-01-02 13:16:42 UTC
Yes, you are right. Those assertions in _convert_src() shouldn't really be there, and it should just return FALSE instead.

However, your patch doesn't seem to do that, it just removes the assertions there ... :)

The assertion in the chain function that checks that width is a multiple of 8 should probably stay.
Comment 3 Alessandro Decina 2006-01-02 18:49:14 UTC
I didn't add the checks since they are implicitly done afterwards. It'd be better to error out earlier, though.

The assertion which checks width to be a multiple of 8 is pointless, as width is always set to a multiple of 8 in gst_flac_dec_write.
Comment 4 Alessandro Decina 2006-01-02 19:02:57 UTC
Created attachment 56675 [details] [review]
add explicit checks
Comment 5 Tim-Philipp Müller 2006-01-02 19:40:51 UTC
Apologies, you are perfectly right. I didn't actually look at the code, I just had a quick look at the patch. Didn't expect those checks down there :)

Fixed in CVS, thanks for the patch:

2006-01-02  Alessandro Decina  <alessandro at nnva dot org>

        Reviewed by: Tim-Philipp Müller  <tim at centricular dot net>

        * ext/flac/gstflacdec.c: (gst_flac_dec_write),
        (gst_flac_dec_convert_src), (gst_flac_dec_src_query),
          Don't g_assert() where we should just return FALSE; remove
          unnecessary g_assert(); initialize some fields properly in
          state change function (fixes #325504). Also, use
          GST_DEBUG_OBJECT in two more places.