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 771210 - tests: new queue2: Add higher-resolution low/high-watermark properties unit test is flaky
tests: new queue2: Add higher-resolution low/high-watermark properties unit t...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other Linux
: Normal critical
: 1.9.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 769449
Blocks:
 
 
Reported: 2016-09-10 19:50 UTC by Tim-Philipp Müller
Modified: 2016-09-30 07:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for updating buffering state when changing low/high watermarks (1.50 KB, patch)
2016-09-11 13:31 UTC, Carlos Rafael Giani
committed Details | Review
Fix watermark test (6.50 KB, patch)
2016-09-11 13:32 UTC, Carlos Rafael Giani
committed Details | Review

Description Tim-Philipp Müller 2016-09-10 19:50:13 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".
Comment 1 Carlos Rafael Giani 2016-09-11 13:31:12 UTC
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.
Comment 2 Carlos Rafael Giani 2016-09-11 13:32:11 UTC
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 3 Sebastian Dröge (slomo) 2016-09-13 08:55:30 UTC
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.
Comment 4 Sebastian Dröge (slomo) 2016-09-13 09:04:43 UTC
The same is probably also needed for multiqueue?
Comment 5 Tim-Philipp Müller 2016-09-17 08:44:23 UTC
> 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().
Comment 6 Tim-Philipp Müller 2016-09-17 09:17:07 UTC
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
Comment 7 Carlos Rafael Giani 2016-09-17 13:42:58 UTC
@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..