GNOME Bugzilla – Bug 786740
appsink: should not keep a ref on the preroll buffer all the time
Last modified: 2017-09-19 08:20:06 UTC
Created attachment 358309 [details] [review] appsink: do a deep copy of the preroll buffer if it belongs to a pool Otherwise the buffer pool will never re-use this buffer. Which is a problem if it has a limited number of buffers. According to appsink's doc the preroll buffer remains available to the application even after EOS. Do a deep copy instead.
Weird, why do we hang onto the preroll buffer even after EOS and/or after we start playing? Patch looks fine to me (if we need/want to support this).
It is not to address a hang issue here (though it could happen in some situation). It is just that the buffer does not return to its pull while the pipeline is playing. There could be no problem about it but it just looks wrong. And I suspect appsink->preroll_buffer was designed without buffer pool consideration in the first place. For example: if a buffer pool is configured with min=max=N buffers then the pool will iterate over N-1 buffers since one is kept all the time by appsink->preroll_buffer.
hang on to = keep around :)
(In reply to Tim-Philipp Müller from comment #3) > hang on to = keep around :) ah got it thx! :)
(In reply to Tim-Philipp Müller from comment #1) > Weird, why do we hang onto the preroll buffer even after EOS and/or after we > start playing? Yes strange. It behaves like this since its initial implementation dd72f88a8c56126a0b74e98b35151c6d124b9ade. At least appsink should clear its local ref once the app has pulled it out, like for other samples. But pb remains if the app does not care about the preroll buffer since it can get it from the pull-sample callback anyway. > > Patch looks fine to me (if we need/want to support this). Oki thx. I think it improves the situation at minimum while not breaking any existing app that would need to pull the preroll buffer again. Let me know if I can push it.
Review of attachment 358309 [details] [review]: I'm effraid this will be disastrous for scrubs in pause. You could maybe do the copy on drains query, allocation query and when receiving the eos.
Created attachment 358658 [details] [review] appsink: unref preroll buffer upon pull There is no reason for appsink to hang onto the preroll buffer. If needed, the application can just keep a ref on this buffer after calling gst_app_sink_try_pull_preroll.
Created attachment 358668 [details] [review] appsink: unref preroll buffer upon pull Added unit test
Sounds good to me, it's just a small behaviour change that might break applications Should be fine for 1.14 (put into release notes!), don't backport to 1.12
Review of attachment 358668 [details] [review]: I think it's better this way, the old behaviour seems unexpected. Can we also rename ->preroll into ->preroll_buffer ? Looks like a boolean otherwise.
(In reply to Nicolas Dufresne (stormer) from comment #15) > rename ->preroll into ->preroll_buffer ? Looks like a boolean otherwise. Sure, done: commit c134a3c21bf5c360a607d21f805f4cde44efe1a0 Author: Julien Isorce <jisorce@oblong.com> Date: Wed Sep 13 14:06:43 2017 +0100 appsink: rename GstBuffer *preroll to preroll_buffer priv->preroll can be confused with basesink_class->preroll https://bugzilla.gnome.org/show_bug.cgi?id=786740
Comment on attachment 358668 [details] [review] appsink: unref preroll buffer upon pull commit 68518acb53d4808cf8352ed4fd1cb8b331553559 Author: Julien Isorce <jisorce@oblong.com> Date: Tue Aug 29 09:47:51 2017 +0100 appsink: unref preroll buffer upon pull There is no reason for appsink to hang onto the preroll buffer. If needed, the application can just keep a ref on this buffer after calling gst_app_sink_try_pull_preroll. Also added unit test 'test_pull_preroll' make elements/appsink.check https://bugzilla.gnome.org/show_bug.cgi?id=786740
Looking at the commit, why do we clear the preroll buffer only on try_pull_preroll() ? Shouldn't we also clear it when going to playing and/or pulling a normal buffer i.e. if no one actually cares about the preroll buffer?
(In reply to Tim-Philipp Müller from comment #18) > Looking at the commit, why do we clear the preroll buffer only on > try_pull_preroll() ? Looking at the code in gstappsink.c we do clear it in the following places: - GST_EVENT_FLUSH_STOP - basesink_class->stop - gobject_class->dispose - try_pull_preroll > Shouldn't we also clear it if no one actually cares about the preroll buffer? I like this idea. But it does not look trivial to me to implement the condition "no one cares" because the user can still call gst_app_sink_pull_preroll even if he has not set any appsink callbacks or signals. I tried a little bit but this does not look good at all. Feel free to suggest a patch, otherwise let's say there is nothing to do more here. Thx!
I was thinking that if someone calls _pull_sample() they're probably no longer interested in any preroll samples.
Make sense, so something like: --- a/gst-libs/gst/app/gstappsink.c +++ b/gst-libs/gst/app/gstappsink.c @@ -1568,6 +1568,7 @@ gst_app_sink_try_pull_sample (GstAppSink * appsink, GstClockTime timeout) priv = appsink->priv; g_mutex_lock (&priv->mutex); + gst_buffer_replace (&priv->preroll_buffer, NULL); ?
Yes that
Created attachment 359996 [details] [review] appsink: also clear preroll buffer in _pull_sample
Comment on attachment 359996 [details] [review] appsink: also clear preroll buffer in _pull_sample commit 7b1056b9467673d004ea7dd63ad91461b8f616fa Author: Julien Isorce <jisorce@oblong.com> Date: Mon Sep 18 17:06:32 2017 +0100 appsink: also clear preroll buffer in _pull_sample If someone calls gst_app_sink_try_pull_sample they are probably no longer interested in any preroll samples. Useful if the user has not registered a preroll appsink callback. Also added unit test 'test_do_not_care_preroll' make elements/appsink.check that fails without this patch. https://bugzilla.gnome.org/show_bug.cgi?id=786740