GNOME Bugzilla – Bug 769449
queue2: Add higher-resolution low/high-watermark properties
Last modified: 2016-09-10 19:50:13 UTC
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 332593 [details] [review] Add higher-resolution low/high-watermark properties
Created attachment 332596 [details] [review] Add higher-resolution low/high-watermark properties, v2 Renamed rel_level to buf_level and other misc fixes
Review of attachment 332596 [details] [review]: Generally looks good to me but all the renaming makes this change look huge and will make things more difficult (like merge conflict in private branches, git blame, etc) ::: plugins/elements/gstqueue2.c @@ +388,1 @@ G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); There's G_PARAM_DEPRECATED @@ +940,1 @@ + return MIN (buf_level, MAX_BUF_LEVEL); Why all these changes in this function? Only needed change here is replacing 100 with MAX_BUF_LEVEL @@ +943,2 @@ static gboolean +get_rel_buf_level (GstQueue2 * queue, gboolean * is_buffering, gint * buf_level) Why all this renaming? :) Basically it's exactly the same function as before, just you changed all variable names
Because it is no longer a percentage. "Percentage" implies a 0-100 range. We could just keep the name "percent", but wouldn't that be confusing as well?
But - I could do the renaming in a separate step. Perhaps this is the least confusing option? So, first just a percentage->buf_level rename, then introduce MAX_BUF_LEVEL? Or are we content with keeping it "percentage"? In the latter case, there should be at least a comment explaining that even though it is named "percent", the range isn't 0-100.
As a separate commit would make sense. buf_level is not so nice though, buffering_level or buffer_level maybe. We don't have to pay a fee per character :)
Agreed. I'll split it up into two commits then. I also have a similar patch for multiqueue, and will split it up as well. One related question: In the past, I required period updates of the buffer level. To that end, I had to intercept the creation of a queue2 element inside uridecodebin via the element-added signal, because the current-level-* properties weren't forwarded by uridecodebin. I could of course patch uridecodebin do add these properties to it, but then it would continue with playbin etc. Or, I could introduce some sort of buffer level query, which aggregates buffer level stats from each queue2 and multiqueue element upstream. Has something like this already been attempted?
Created attachment 332618 [details] [review] Distinguish between buffering-level and buffering-percentage As it turns out, the patch can be considerably simplified, since the percentage for buffering messages itself does not have to be changed to use the higher-resolution range (it is just a percentage, so its stays in the 0..100 range). This in turn means that by refactoring some buffering level related functions, many other parts such as the SET_PERCENT macro do not have to be modified. This new patch separates the concepts of "buffering level" and "buffering percentage" as a prerequisite for the new low/high watermark properties.
Created attachment 332619 [details] [review] Add higher-resolution low/high-watermark properties, v3 New patch for low/high watermark properties with higher resolution
Created attachment 332621 [details] [review] Distinguish between buffering-level and buffering-percentage, v2 Typo
Review of attachment 332619 [details] [review]: Does not apply anymore, otherwise seems good to go :) ::: plugins/elements/gstqueue2.c @@ +389,1 @@ G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); G_PARAM_DEPRECATED @@ +393,3 @@ + "(Deprecated: use high-watermark instead)", + 0, 100, DEFAULT_HIGH_WATERMARK * 100, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); G_PARAM_DEPRECATED
Review of attachment 332621 [details] [review]: Also does not apply anymore, otherwise good to go
They actually do apply. But bugzilla shows them in reverse order - "Distinguish between buffering-level and buffering-percentage, v2" comes first. Patches are alphabetically sorted? If so, I'll add index numbers in the future.
commit db66cb51b38fbffc2e1fb336a9636b22ec0fe241 Author: Carlos Rafael Giani <dv@pseudoterminal.org> Date: Wed Aug 3 15:20:20 2016 +0200 queue2: Add higher-resolution low/high-watermark properties low/high-watermark 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%. Also adding a test to check the fill and watermark level behavior. https://bugzilla.gnome.org/show_bug.cgi?id=769449 commit e0f1a9e618e16e887d1d2b94e52ba02ae316fd78 Author: Carlos Rafael Giani <dv@pseudoterminal.org> Date: Wed Aug 3 15:27:40 2016 +0200 queue2: Distinguish between buffering percentage and buffering level To make the code clearer, and to facilitate future improvements, introduce a distinction between the buffering level and the buffering percentage. Buffering level: the queue's current fill level. The low/high watermarks are in this range. Buffering percentage: percentage relative to the low/high watermarks (0% = low watermark, 100% = high watermark). To that end, get_buffering_percent() is renamed to get_buffering_level(), and the code at the end that transforms to the buffering percentage is factored out into a new convert_to_buffering_percent() function. Also, the buffering level range is parameterized by adding a new constant called MAX_BUFFERING_LEVEL. https://bugzilla.gnome.org/show_bug.cgi?id=769449
Indeed. Ordering patches accordingly would be useful in the future :)
The unit test is flaky, I will clone a new bug for that. Should be fixed or disabled.