GNOME Bugzilla – Bug 325504
[flacdec] gst_flac_dec_convert_src [mis]uses g_assert
Last modified: 2006-01-02 19:40:51 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:
Created attachment 56662 [details] [review] fix
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.
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.
Created attachment 56675 [details] [review] add explicit checks
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), (gst_flac_dec_change_state): 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.