GNOME Bugzilla – Bug 769394
appsink callback race condition
Last modified: 2018-11-03 11:48:28 UTC
In gstappsink.c, it is possible to get undefined behavior if the gst_app_sink_set_callbacks() function is called while the element is in the PLAYING state and actively using the already set callbacks. For example, it is possible for the gst_app_sink_set_callbacks() function to set callbacks.new_sample = NULL just after the gst_app_sink_render() function checks if callback.new_sample == NULL. This can be prevented by adding a mutex around the usage of the callbacks and around the gst_app_sink_set_callbacks() function. Another option is to not allow the callbacks to be set if the element is in the PLAYING state.
Setting the callbacks should probably only be allowed in NULL and maybe READY state. I don't see a use case for setting it later and it complicates the code considerably. Do you want to provide a patch?
Actually, we've been using appsink in this way for quite some time (changing the appsink callbacks in the RUNNING state). I'm surprised we haven't seen an issue with this sooner. It is nice having the ability for another thread to connect up to the appsink without having to disturb the parent pipeline. If we were to only allow the callbacks to be set in the NULL or READY state, how would the gst_app_sink_set_callbacks() function behave? Have it post an error/warning and immediately return? Or, have this function return a bool indicating if the callbacks were actually set? For now, to get around the issue in regards to our application, I've created a patch that adds a mutex for the callback operations (wherever they are executed or set), which appears to fix the problem. I can post that patch if you like. Else, I can work on making a patch for the other approach.
How do you ensure that setting the callbacks during PLAYING/PAUSED is actually not racy? Switching from one callback to another would be rather uncontrolled, you don't know when exactly the new callbacks are going to be called for the first time in relation to the data flow. That's why I suggested to disallow setting the callbacks in PAUSED/PLAYING. It could just do a g_warning() in that case. What's your use case for setting it in PAUSED/PLAYING?
The race condition can be prevented by protecting the set_callbacks() function with a lock, and protecting the segments where callbacks.eos(), callbacks.new_preroll(), and callbacks.new_sample() are checked and called. Our use-case is that we have a capture/archive pipeline that records streams from a camera. We have an RTSP server that can proxy video from the capture pipeline. It connects to the capture pipeline through an appsink that is created when the capture pipeline is created (we do not dynamically add/remove this appsink). So when a client requests a proxy stream, the RTSP server adds a reference to the appsink and sets the callbacks to start receiving data from the appsink (this is done without changing the state of the appsink). The uncontrolled nature of this is fine since the RTSP server just wants the latest stream data as soon as possible. Our motivation for this approach was to keep interactions with the capture pipeline as light as possible, since we do not want to disturb the capture process. That said, I am not opposed to changing the way we interact with the appsinks if it is fundamentally wrong :) I suppose the better approach would be to use a blocking pad-probe, change the appsink to the READY state, set the callbacks, then set it back to PLAYING?
If you only want to get the very latest data when you want to start capturing, just a boolean in the callback to signal if samples should be dropped or not seems the best.
In the absence of patches I am tempted to WONTFIX this, or document / warn if it's changed at runtime. This is supposed to be a highly performant code path. Changing target callbacks at runtime could always be done with an additional indirection application-side. I don't think it's a very common use case. I can see how it's convenient of course.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/280.