GNOME Bugzilla – Bug 754597
appsrc: fails to push the caps at the start of the data flow when configured in seekable/random-access mode
Last modified: 2015-09-09 09:35:08 UTC
With the "stream-type" property not equal to GST_APP_STREAM_TYPE_STREAM and there's caps set by the application (in NULL state), the appsrc fails to push these caps downstream, when set to PLAYING. This is caused due to the initial seek performed by basesrc, before starting the streaming thread. As the caps are queued by the appsrc, this initial seek flushes the queue, leaving the streaming thread to see the queue empty, immediately after getting launched.
Created attachment 310690 [details] [review] Attaching possible fix. This patch assumes that the initial seek() will always be called, when starting the appsrc.
Any chance you could you make a unit test for this ? I don't think it should push caps events in random-access mode btw, only in push mode (seekable or not).
Yeah. Sure. Will make the test. Agree. random-access mode need not have caps events. So in random access mode if the appsrc pops caps(just in case the application sets caps property), then should it just ignore the caps and not push it down?
It should still save them internally so when queried for the caps, they are returned. Perhaps it shouldn't even put stuff like caps into the internal queue at all in random access mode actually.
(In reply to Tim-Philipp Müller from comment #4) > It should still save them internally so when queried for the caps, they are > returned. Perhaps it shouldn't even put stuff like caps into the internal > queue at all in random access mode actually. If it uses the queue for something in random access mode, then the caps have to be stored there :) This is most likely a regression since 1.4, as before we set the caps immediately and never queued them up.
Comment on attachment 310690 [details] [review] Attaching possible fix. I think a nicer fix would be to not flush caps from the queue when flushing (or only flushing all but the newest caps). That probably requires less changes to the code and is consistent with how other queues work currently.
(In reply to Sebastian Dröge (slomo) from comment #5) > (In reply to Tim-Philipp Müller from comment #4) > > It should still save them internally so when queried for the caps, they are > > returned. Perhaps it shouldn't even put stuff like caps into the internal > > queue at all in random access mode actually. > > If it uses the queue for something in random access mode, then the caps have > to be stored there :) > > > This is most likely a regression since 1.4, as before we set the caps > immediately and never queued them up. Yes, It is a regression. My same app works under 1.4
Created attachment 310867 [details] [review] Attaching the patch as per the discussion. Patch also includes unit test.
Created attachment 310868 [details] [review] Attaching the patch(with unit test) as per the discussion. Just a minor refactoring from the last one.
Review of attachment 310868 [details] [review]: Looks mostly good, thanks :) Can you update the patch? ::: gst-libs/gst/app/gstappsrc.c @@ +573,3 @@ if (obj) { + if (GST_IS_CAPS (obj) && retain_last_caps && + priv->last_caps == GST_CAPS (obj)) { This condition seems a bit weird. You basically want to remember the caps that are closest to the tail of the queue... so why not if (GST_IS_CAPS (obj) && retain_last_caps) { gst_caps_replace (&requeue_caps, GST_CAPS_CAST (obj)); } else { gst_mini_object_unref (obj); } You also would need to use the object lock if you want to use the last_caps field, and in theory I think last_caps could be queued up multiple times... in which case your code would leak references.
Review of attachment 310868 [details] [review]: Thanks for the review. Will make the changes. ::: gst-libs/gst/app/gstappsrc.c @@ +573,3 @@ if (obj) { + if (GST_IS_CAPS (obj) && retain_last_caps && + priv->last_caps == GST_CAPS (obj)) { Yes, will decouple the usage of last_caps from here. It was just an extra check to be certain :) BTW, Is gst_app_src_flush_queued() thread safe? Shouldn't it be guarded by priv->mutex always?
I think it should, yes. Otherwise the user could push buffers into the appsrc while it is currently flushing the queue, resulting most likely in memory corruption of the queue structure.
gst_app_src_flush_queued() is called with the mutex hold from gst_app_src_stop(), does not need the mutex in gst_app_src_dispose() and is missing the mutex in all other places. Can you add that in another commit? I think best is to require the mutex to be taken *outside* the flush function
Yes, Makes sense to take the mutex from outside. Sure, will have those in a different commit. Meanwhile, I will remove the last_caps usage from the flush() for this commit. Guess, will not require the object lock then, right? Also, as I understand it, the mutex is for anything related specifically to the internal queue, right? Just curious on the usage of mutex vs object lock in appsrc.
The mutex is for everything queue related, the object lock for everything related to the appsrc properties
Created attachment 310916 [details] [review] Attaching the patch(with unit test) as per the discussion.
commit 8613525301b93202de0e7ff63eea9367582e7262 Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Sep 9 12:33:02 2015 +0300 appsrc: Always take the mutex before flushing the queue Otherwise the application might push new buffers into the queue while we're flushing, potentially causing the GQueue data structure to become inconsistent and causing crashes soon after. https://bugzilla.gnome.org/show_bug.cgi?id=754597 commit bbe967a2780469daac99d5f599bd54b41651aff0 Author: Vikram Fugro <vikram.fugro@gmail.com> Date: Tue Sep 8 01:35:19 2015 +0530 appsrc: retain the latest caps in queue when flushing - Retain the latest caps in the internal queue, when flushing. - Add a unit test case for the same. https://bugzilla.gnome.org/show_bug.cgi?id=754597