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 769894 - validate: Race when checking whether some actions have completed
validate: Race when checking whether some actions have completed
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-devtools
git master
Other Linux
: Normal major
: 1.9.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-08-14 15:31 UTC by Edward Hervey
Modified: 2016-09-30 07:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
validate:scenario: Wait for ASYNC_DONE to set async state change DONE (2.65 KB, patch)
2016-08-14 23:06 UTC, Thibault Saunier
committed Details | Review

Description Edward Hervey 2016-08-14 15:31:33 UTC
This race happens quite quickly with the GES scrubbing validate tests. The core reason is because of the way GES timeline posts its own ASYNC messages and handles seeks asynchronously.

Note : This is *not* an issue in GES.

Take the following scenario (the direction of the seek doesn't matter):
pause, playback-time=0.0
seek, playback-time=0.0, start="duration - 0.5", flags=accurate+flush

* The pause action is executed
* The pipeline prerolls
* The STATE_CHANGED message (READY=>PAUSED) is posted on the bus from a streaming thread
* The ASYNC_DONE message is posted on the bus from a streaming thread
* The main thread pops out the STATE_CHANGED message from the bus
** The pause action is marked as done
** The next action is executed (flushing seek)
** ges-pipeline posts ASYNC_START and returns TRUE for the seek event
* The main thread pops out the next message (ASYNC_DONE from the initial preroll)
** it checks for the current action, sees that it's a seek, and marks it as done
 .. but the seek wasn't executed by then

The visible result of this is seeing such scenarios failing with:
"event: position after a seek is wrong : Reported position after accurate seek in PAUSED state should be exactly what the user asked for"

Because validate checks the position after it *thinks* the seek event was executed ... but it wasn't really.
Comment 1 Thibault Saunier 2016-08-14 19:28:29 UTC
Should we just also wait for ASYNC_DONE to happen when going to PAUSED? Other idea would be to check the message seqnum for the seek events but I am afraid this is not well supported. What do you prefer?
Comment 2 Thibault Saunier 2016-08-14 23:06:57 UTC
Created attachment 333302 [details] [review]
validate:scenario: Wait for ASYNC_DONE to set async state change DONE

Fixes
Comment 3 Sebastian Dröge (slomo) 2016-08-16 06:46:16 UTC
If you wait for *both* that should do the job here. Note however that you can't assume to get the same number of async-done as seeks and state changes you submitted. If there are multiple happening at the same time, you will only receive a single async-done.

This could be problematic with things automatically changing states, or the handling of the BUFFERING messages.
Comment 4 Tim-Philipp Müller 2016-08-16 06:56:04 UTC
One might be able to look at the sticky SEGMENT event on a pad and correlate the seqnum of that to the seqnum of SEEK events created.
Comment 5 Thibault Saunier 2016-08-16 18:57:06 UTC
(In reply to Sebastian Dröge (slomo) from comment #3)
> If you wait for *both* that should do the job here. Note however that you
> can't assume to get the same number of async-done as seeks and state changes
> you submitted. If there are multiple happening at the same time, you will
> only receive a single async-done.
> This could be problematic with things automatically changing states, or the
> handling of the BUFFERING messages.

I am trying to think about issues it can cause issues but in GstValidate everything is currently very sequential and I can not think of a scenario failling with that approach. Basically in the patch I proposed I am just making sure that when set_state returns ASYNC, we always wait for 1- the new state to be reached 2- an ASYNC_DONE message on the bus (is there a particular order for those events to happen?). I can't reproduce that bug with it.
Comment 6 Sebastian Dröge (slomo) 2016-08-17 06:15:05 UTC
ASYNC_DONE always comes after the STATE_CHANGED message IIRC.

I think it can cause issues in gst-validate in combination with scrub seeking. Are you always waiting for ASYNC_DONE before sending the next seek? If not, you might run into problems if you later wait for ASYNC_DONE, depending on the circumstances.

Should be good to go though, it's definitely improving things and more correct.
Comment 7 Thibault Saunier 2016-09-05 17:51:49 UTC
Attachment 333302 [details] pushed as 6e9c672 - validate:scenario: Wait for ASYNC_DONE to set async state change DONE