GNOME Bugzilla – Bug 771210
tests: new queue2: Add higher-resolution low/high-watermark properties unit test is flaky
Last modified: 2016-09-30 07:05:11 UTC
Running suite(s): queue2 88%: Checks: 9, Failures: 1, Errors: 0 elements/queue2.c:270:F:general:test_watermark_and_fill_level:0: Got incorrect percentage: 0% expected: 50% Can be reproduced fairly quickly with: $ GST_CHECKS=test_watermark_and_fill_level make elements/queue2.forever This needs to be investigated and fixed, or disabled. Or stuff needs to be backed out. +++ This bug was initially created as a clone of Bug #769449 +++ Add new low/high-watermark properties, which are of type double, and given in range 0.0-1.0. This makes it possible to set low/high watermarks with greater resolution, which is useful with large queue2 max sizes and watermarks like 0.5% for example. low/high-percent properties remain for backwards compatibility, and are marked as "deprecated".
Created attachment 335304 [details] [review] Patch for updating buffering state when changing low/high watermarks Prerequisite to the fixed test. If buffering was enabled, and the low/high watermarks were changed, the buffering wasn't being updated. As a result, no new buffering message was produced, even if the relative buffer level did change.
Created attachment 335305 [details] [review] Fix watermark test This fixes test_watermark_and_fill_level . Essentially, the same test from the multiqueue test code is adapted for queue2.
Comment on attachment 335304 [details] [review] Patch for updating buffering state when changing low/high watermarks This looks like it needs a mutex around that. set_property() can be called from any thread at any time.
The same is probably also needed for multiqueue?
> This looks like it needs a mutex around that. set_property() can be called > from any thread at any time. There's a GST_QUEUE2_MUTEX_LOCK (queue); at the beginning of set_property().
Thanks for the quick fix Carlos! commit 5de03542ea2c77ef50444965739adcd9dc590526 Author: Carlos Rafael Giani <dv@pseudoterminal.org> Date: Sun Sep 11 15:28:43 2016 +0200 queue2: Fix watermark test This carries over code for a similar test from multiqueue to ensure full control over the dataflow while testing. (The previous attempt was racy since the fill level changed without any thread sync with the test code.) https://bugzilla.gnome.org/show_bug.cgi?id=771210 commit 7413064f06a03e6f13a0f0c8d88b42cc83d55891 Author: Carlos Rafael Giani <dv@pseudoterminal.org> Date: Sun Sep 11 15:26:26 2016 +0200 queue2: Update buffering if its enabled and low/high watermarks are changed https://bugzilla.gnome.org/show_bug.cgi?id=771210
@Tim: you're welcome :) (In reply to Sebastian Dröge (slomo) from comment #4) > The same is probably also needed for multiqueue? Unlike with queue2, in multiqueue, there is no single lock for the entire set_property call. Instead, we have code like this: case PROP_LOW_WATERMARK: mq->low_watermark = g_value_get_double (value) * MAX_BUFFERING_LEVEL; recheck_buffering_status (mq); break; and recheck_buffering_status() does lock the multiqueue mutex. Perhaps this code should be rewritten so that the integer write is also behind the lock, but I have found many places in GStreamer where integer writes apparently are assumed to be atomic..