GNOME Bugzilla – Bug 758961
adaptivedemux: tests: do not sleep with the GST_TEST_LOCK taken
Last modified: 2018-11-03 13:44:09 UTC
All the test callbacks are called with GST_TEST_LOCK taken. The callbacks should release this lock if they want to sleep. Seek test currently sleeps in callback, so it must temporarily release the GST_TEST_LOCK
Created attachment 316671 [details] [review] proposed patch
It looks like this code moved into adaptive_demux_common.c now, any chance you could check if it's still needed after the refactoring and update the patch if so?
yes, I am currently working on rebasing some patches, including this one.
Created attachment 318522 [details] [review] 1/2 fix usage of test lock adaptive_demux: tests: fix usage of test lock The GST_TEST_LOCK lock currently protects all test related data: priv and the streams obtained from it using getTestOutputDataByPad. So, it must be held during calls of priv->callbacks->... because those functions could use priv, engine or stream. We should replace this big lock with smaller ones, but until then all accesses to a test data must be protected by GST_TEST_LOCK.
Created attachment 318523 [details] [review] 2/2 do not sleep with the GST_TEST_LOCK taken All the test callbacks are called with GST_TEST_LOCK taken. The callbacks should release this lock if they want to sleep. Seek test currently sleeps in callback, so it must temporarily release the GST_TEST_LOCK
Those patches apply cleanly if https://bugzilla.gnome.org/show_bug.cgi?id=760328 patches are applied on master. If not, small conflicts will arise.
Created attachment 321376 [details] [review] 1/2 fix usage of test lock merged with latest master and regenerated the patch
Created attachment 321377 [details] [review] 2/2 do not sleep with GST_TEST_LOCK taken resynced with master and regenerated the patch
Created attachment 326773 [details] [review] adaptive_demux: tests: fix usage of test lock Obsoletes patch 1/2. Re-based to master.
Created attachment 326774 [details] [review] adaptivedemux: tests: do not sleep with the GST_TEST_LOCK taken Obsoletes patch 2/2 Re-based to master and renamed GST_TEST_LOCK to GST_ADAPTIVE_ENGINE_LOCK. The rename was done to make it more intent revealing and to reduce chances of namespace collisions.
Review of attachment 326773 [details] [review]: ::: tests/check/elements/adaptive_demux_engine.c @@ +167,3 @@ priv->user_data); } + GST_TEST_UNLOCK (priv); This means that actions done in appsink_event handler won't be able to cause more events to be sent. A flushing seek, perhaps that would cause a flush-start event would cause this to deadlock. It doesn't at the moment but it could cause potential issues in the future. I'd prefer for implementers of appsink_event to in their implementation use the lock if they need. @@ +193,1 @@ Same here for demux_sent_data
Review of attachment 326774 [details] [review]: This patch does 2 things, it renames the variable and also does what the commit message says. It would be better to split in 2 commits.
(In reply to Thiago Sousa Santos from comment #12) > Review of attachment 326774 [details] [review] [review]: > > This patch does 2 things, it renames the variable and also does what the > commit message says. It would be better to split in 2 commits. Do you mean the move of the GST_TEST_LOCK macro moving into the header file and being renamed GST_ADAPTIVE_ENGINE_LOCK ?
Created attachment 327774 [details] [review] 1/4 - adaptivedemux: tests: fix usage of test lock
Created attachment 327775 [details] [review] 2/4 - adaptivedemux: tests: make GstAdaptiveDemuxTestEngine test lock available
Created attachment 327776 [details] [review] 3/4 - adaptivedemux: tests: do not sleep with the GST_TEST_LOCK taken
Created attachment 327777 [details] [review] 4/4 - adaptivedemux: tests: don't hold GST_ADAPTIVE_ENGINE_LOCK when calling callbacks I've split the patches so that the rename of the GST_TEST_LOCK macro is in its own commit and also changed the engine to not hold the lock when calling a callback. This turned out to be a bit more tricky than I first thought, because by dropping the lock, other events can get processed by the engine. If one of these is a pad-removed signal, a stream could get invalidated while another callback is using it. I've added reference counting that should stop a GstAdaptiveDemuxTestOutputStream from being released while a callback is in progress.
Removing NEEDINFO, as patches have been split up now.
Created attachment 347087 [details] [review] adaptivedemux: tests: fix usage of test lock The GST_TEST_LOCK lock currently protects all test related data: priv and the streams obtained from it using getTestOutputDataByPad. So, it must be held during calls of priv->callbacks->... because those functions could use priv, engine or stream. We should replace this big lock with smaller ones, but until then all accesses to a test data must be protected by GST_TEST_LOCK.
Created attachment 347088 [details] [review] adaptivedemux: tests: make GstAdaptiveDemuxTestEngine test lock available The mutex in GstAdaptiveDemuxTestEngine was only available in adaptive_demux_engine.c. However, some tests need to be able to drop the lock (for example in order to be able to sleep without blocking other threads). By adding GST_ADAPTIVE_ENGINE_LOCK and GST_ADAPTIVE_ENGINE_UNLOCK to the header, any test can make use of the lock.
Created attachment 347089 [details] [review] adaptivedemux: tests: do not sleep with the GST_TEST_LOCK taken All the test callbacks are called with GST_TEST_LOCK taken. The callbacks should release this lock if they want to sleep. Seek test currently sleeps in callback, so it must temporarily release the GST_TEST_LOCK
Created attachment 347090 [details] [review] adaptivedemux: tests: don't hold GST_ADAPTIVE_ENGINE_LOCK when calling callbacks During the review of bug758961, there was a preference for not holding the GST_ADAPTIVE_ENGINE_LOCK so that callbacks can make calls that would otherwise block (e.g. sending events).
Updated to latest GIT master, but it breaks all the tests.
There is a replacement for the "adaptivedemux: tests: fix usage of test lock" patch that doesn't break the tests on ticket: https://bugzilla.gnome.org/show_bug.cgi?id=781632
-- 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-bad/issues/334.