GNOME Bugzilla – Bug 763757
multiqueue: Make sure mq->percent remains valid after modifying high-percent value
Last modified: 2016-04-15 16:17:31 UTC
Created attachment 324105 [details] [review] patch for scaling mq->percent after setting high-percent Currently, if the high-percent value is modified right after data was inserted, it is possible that the mq->percent value is actually invalid (read: larger than high-percent). This happens because the mq->percent value isn't updated when high-percent is set. It still contains the value from earlier, which as said is larger Given the SET_PERCENT code: #define SET_PERCENT(mq, perc) G_STMT_START { \ if (perc != mq->percent) { \ mq->percent = perc; \ mq->percent_changed = TRUE; \ GST_DEBUG_OBJECT (mq, "buffering %d percent", perc); \ } \ } G_STMT_END and the code in update_buffering(): ... percent = get_percentage (sq); if (mq->buffering) { if (percent >= mq->high_percent) { mq->buffering = FALSE; } /* make sure it increases */ percent = MAX (mq->percent, percent); SET_PERCENT (mq, percent); } else { ... What happens is this: 1. Data is received, mq->percent is set for example to 10 2. high-percentage is set, for example to 5 3. In update_buffering(), (percent >= mq->high_percent) evaluates to TRUE; mq->buffering is set to FALSE; the MAX () expression returns 10, because mq->percent wasn't updated in step #2 earlier; as a result, SET_PERCENT sees that perc == mq->percent and therefore does not set mq->percent_changed to TRUE; therefore, since mq->percent_changed is not set to TRUE, no 100% buffering message is posted. By scaling mq->percent in the set_property() function, the MAX () behavior is corrected, and 100% buffering is posted.
Comment on attachment 324105 [details] [review] patch for scaling mq->percent after setting high-percent I would prefer that we recalculate the values instead of just scaling. The scaling is faster but gives +/- 1 of the real result. It's not really a code path that is called often, so let's just do it right here.
Created attachment 325884 [details] [review] improved patch which recalculates fill level & adds tests Improved patch. Fill level percentage is now recalculated. Also, the multiqueue check file gets additional tests to check for the aforementioned case.
Created attachment 325889 [details] [review] improved patch which recalculates fill level & adds tests (v2, slightly improved tests)
Review of attachment 325889 [details] [review]: Looks good to me ::: plugins/elements/gstmultiqueue.c @@ +622,3 @@ case PROP_HIGH_PERCENT: mq->high_percent = g_value_get_int (value); + recheck_buffering_status (mq); Shouldn't this also happen for low-percent?
Perhaps. On one hand, only high_percent is used for computing percentage: percent = percent * 100 / mq->high_percent; On the other hand, low_percent is used for determining if mq->buffering should be set to TRUE. This however is a separate issue I think: what happens (or what should happen) if the first data block that is sent into the queue already pushes the fill level above low_percent? And what happens if low_percent is then lowered to fall below the fill level? This could be a separate commit.
Shouldn't it post a buffering message if because of the new low-percent setting we should be buffering now again?
That's what I meant. But since this also requires new tests, and this commit is about what happens when *high*-percent is modified, I think the *low*-percent changes should be handled in a separate followup issue.
A follow-up commit at least, yes
OK, I'll add a separate second patch here for low-percent then as well.
Created attachment 325988 [details] [review] patch for rechecking buffering status after low-percent change Here's the patch. It also adds a test case to check for what happens when low-percent changes.
commit 3bd5aeac52a7e88860270a3e4a460409d68b50cd Author: Carlos Rafael Giani <dv@pseudoterminal.org> Date: Thu Apr 14 11:54:32 2016 +0200 multiqueue: Recheck buffering status after changing low threshold https://bugzilla.gnome.org/show_bug.cgi?id=763757 commit a1d2f387c60e3bc9da93ddd0cf976fad96d53b95 Author: Carlos Rafael Giani <dv@pseudoterminal.org> Date: Thu Apr 14 00:09:44 2016 +0200 multiqueue: Recalculate fill level after changing high-threshold This ensures the following special case is handled properly: 1. Queue is empty 2. Data is pushed, fill level is below the current high-threshold 3. high-threshold is set to a level that is below the current fill level Since mq->percent wasn't being recalculated in step #3 properly, this caused the multiqueue to switch off its buffering state when new data is pushed in, and never post a 100% buffering message. The application will have received a <100% buffering message from step #2, but will never see 100%. Fix this by recalculating the current fill level percentage during high-threshold property changes in the same manner as it is done when use-buffering is modified. https://bugzilla.gnome.org/show_bug.cgi?id=763757
If possible, make these patches part of an 1.8.1 release.