GNOME Bugzilla – Bug 794717
Debug levels applied in wrong order
Last modified: 2018-04-05 18:44:35 UTC
For categories created post-init, the level name patterns are applied in the wrong order. Test pipeline: GST_DEBUG="GST_PLUGIN_LOADING:INFO,*:FIXME,videotestsrc:DEBUG" gst-launch-1.0 videotestsrc num-buffers=1 ! fakesink The output should include DEBUG lines from videotestsrc, but doesn't. Neither does it (and should it) include INFO lines from GST_PLUGIN_LOADING, so the order isn't reversed for all categories. This got broken in 67e9d139: the new code doesn't stop checking patterns once a match has been found. AFAICT this re-application of filters is superfluous anyway, as gst_debug_reset_threshold is run on the new category and has already applied the patterns.
Created attachment 370178 [details] [review] gstinfo: Reduce code duplication around level pattern matching Move the match, logging and set_threshold to a new function. The log levels are different, so choose the higher one (LOG). Having two equivalent messages at two different levels seems like a bad idea anyway.
Created attachment 370179 [details] [review] gstinfo: Simplify gst_debug_reset_threshold Replace the while+goto with a for+break and check walk to determine whether we had a match. Move up the unlock to keep the locked section as small as possible.
Created attachment 370180 [details] [review] gstinfo: Remove unneeded reapplication of patterns Besides being superfluous (gst_debug_reset_threshold already applies patterns) it was also wrong and didn't stop checking patterns after the first match (broken in 67e9d139).
This is a regression in 1.14
Comment on attachment 370180 [details] [review] gstinfo: Remove unneeded reapplication of patterns This breaks the unit test however: Running suite(s): GstInfo 90%: Checks: 11, Failures: 1, Errors: 0 gst/gstinfo.c:476:F:info:info_post_gst_init_category_registration:0: 'gst_debug_category_get_threshold (cats[0xa1b])' (2) is not equal to 'GST_LEVEL_LOG' (6) Is the (newly-added along with that commit) unit test broken as well? :)
Actually yes, the unit test is wrong and it triggers correctly. On the upside, this also provides a test case for the ordering issue, so I don't have to write a new one ;)
commit 6436437d836badb1f01f71cf039fa65e4a3a2023 (HEAD -> master, origin/master, origin/HEAD) Author: Jan Alexander Steffens (heftig) <jan.steffens@gmail.com> Date: Tue Mar 27 10:25:46 2018 +0200 gstinfo: fix debug levels being applied in the wrong order Remove unneeded reapplication of patterns. Besides being superfluous (gst_debug_reset_threshold already applies patterns) it was also wrong and didn't stop checking patterns after the first match (broken in 67e9d139). Also fix up unit test which checked for the wrong order. https://bugzilla.gnome.org/show_bug.cgi?id=794717 commit c4ff06986442fe50f424161220ccb31db758b797 Author: Jan Alexander Steffens (heftig) <jan.steffens@gmail.com> Date: Tue Mar 27 10:15:46 2018 +0200 gstinfo: Simplify gst_debug_reset_threshold() implementation Replace the while+goto with a for+break and check walk to determine whether we had a match. Move up the unlock to keep the locked section as small as possible. https://bugzilla.gnome.org/show_bug.cgi?id=794717 commit b12df466f2075bb85b684cd8872127e4022e58ab Author: Jan Alexander Steffens (heftig) <jan.steffens@gmail.com> Date: Tue Mar 27 10:14:27 2018 +0200 gstinfo: Reduce code duplication around level pattern matching Move the match, logging and set_threshold to a new function. The log levels are different, so choose the higher one (LOG). Having two equivalent messages at two different levels seems like a bad idea anyway. https://bugzilla.gnome.org/show_bug.cgi?id=794717
Comment on attachment 370180 [details] [review] gstinfo: Remove unneeded reapplication of patterns Changed commit message summary, since it doesn't describe the issue that was fixed.