GNOME Bugzilla – Bug 701015
Using a g_thread for gnonlin pipeline update / forward eos on the streaming thread.
Last modified: 2013-09-24 15:56:37 UTC
Thibault saunier and I have been working on that, there's a branch there : https://github.com/MathieuDuponchelle/gnonlin/commits/eos_thread This branch passes more tests than master, and fixes a longstanding bug that had gone under the radar for two years, where when removing a ghostpad we wouldn't remove the probe handler associated to its target. I create a new thread, waiting on a GCond, that gets signalled when we need to update. When we need to forward the eos, we also update the pipeline before that in the streaming thread, because we want it to be in the correct state when the eos is forwarded.
Thast is really cool stuff. Could you cleanup your branch, there is a lot do / undu, left over comment (even C++ style comment). Aslo, starting a new thread each time might not be the best thing, discussion with Edward lead to the conclusion we should use a GstTask, and pause it when it's not needed. It will also ensure we never endup with two seeking threads (which would make no sense). Other then that, it's pretty nice that you take the time to look at those issues.
(In reply to comment #1) > Thast is really cool stuff. Could you cleanup your branch, there is a lot do / > undu, left over comment (even C++ style comment). We know the history is not reaaly nice but we have been work both on that one which leaded to what we have now. > Aslo, starting a new thread each time might not be the best thing, discussion > with Edward lead to the conclusion we should use a GstTask, and pause it when > it's not needed. It will also ensure we never endup with two seeking threads > (which would make no sense). Other then that, it's pretty nice that you take > the time to look at those issues. We are currently starting the thread between NULL to READY and killing it from READY to NULL which iirc is the correct way of doing it. We should never end up with various threads the way it is implemented, if it happens, it is a bug that we need to fix.
The alternative would be to only start the thread when it is needed and shut it down afterwards. Starting threads is nowadays very fast, so shouldn't cause much overhead. Starting/stopping the thread in NULL<->READY sounds ok, but actually READY<->PAUSED should be sufficient here too (there's no dataflow in READY, and this thread is only used when dataflow is happening). Using a GstTask here would probably simplify code a bit, as it has functions to start/stop/pause the thread which you otherwise have to implement yourself. Not exactly rocket science, but still nice to not worry about that ;)
*** Bug 595187 has been marked as a duplicate of this bug. ***
(In reply to comment #3) > The alternative would be to only start the thread when it is needed and shut it > down afterwards. Starting threads is nowadays very fast, so shouldn't cause > much overhead. Starting/stopping the thread in NULL<->READY sounds ok, but > actually READY<->PAUSED should be sufficient here too (there's no dataflow in > READY, and this thread is only used when dataflow is happening). It's not a problem to put the thread creation / joining in READY<->PAUSED, we were wondering about the standard way to do so. > Using a GstTask here would probably simplify code a bit, as it has functions to > start/stop/pause the thread which you otherwise have to implement yourself. Not > exactly rocket science, but still nice to not worry about that ;) If you look at the code, the thread starting / stopping is actually extra light (one boolean). Don't know if it's really needed to switch to GstTask ?
(In reply to comment #5) > (In reply to comment #3) > > The alternative would be to only start the thread when it is needed and shut it > > down afterwards. Starting threads is nowadays very fast, so shouldn't cause > > much overhead. Starting/stopping the thread in NULL<->READY sounds ok, but > > actually READY<->PAUSED should be sufficient here too (there's no dataflow in > > READY, and this thread is only used when dataflow is happening). > > It's not a problem to put the thread creation / joining in READY<->PAUSED, we > were wondering about the standard way to do so. I have no strong opinion on this, doesn't really matter much :) > > Using a GstTask here would probably simplify code a bit, as it has functions to > > start/stop/pause the thread which you otherwise have to implement yourself. Not > > exactly rocket science, but still nice to not worry about that ;) > > If you look at the code, the thread starting / stopping is actually extra light > (one boolean). Don't know if it's really needed to switch to GstTask ? Well, and the GCond and all around that :) It's not *needed*, just seems nice to make things even simpler.
Yep true, I used a g_thread partly because I didn't know the API and wanted to discover it as well, I'll look at GstTask's API then. For the thread creation, I actually think having it in NULL<->READY is better because that way we keep the same thread all over, not really necessary but Thibault also thought it was the best place to do that iirc.
(In reply to comment #7) > Yep true, I used a g_thread partly because I didn't know the API and wanted to > discover it as well, I'll look at GstTask's API then. It has a loop for you already (you provide a loop function, not a thread function), and you can pause/start the task easily with some API. So basically could have just the EOS handling code in there, start the task whenever there's an EOS received, and after it is done handling the EOS pause the task. > For the thread creation, I actually think having it in NULL<->READY is better > because that way we keep the same thread all over, not really necessary but > Thibault also thought it was the best place to do that iirc. Then let's just do that :)
gnlcomposition: Forward EOS from the streaming thread when appropriate. > if (!(priv->segment->flags & GST_SEEK_FLAG_SEGMENT) > && priv->ghostpad) { >- GST_LOG_OBJECT (comp, "Pushing out EOS"); >+ GST_ERROR_OBJECT (comp, >+ "Pushing out EOS in eos_main_thread, should not happen"); > gst_pad_push_event (priv->ghostpad, gst_event_new_eos ()); > } else if (priv->segment->flags & GST_SEEK_FLAG_SEGMENT) { > gint64 epos; This should send GST_ELEMENT_ERROR() and make the pipeline fail. (if that code still exist after the rest, not sure). >+ for (tmp = comp->priv->objects_stop; tmp; tmp = g_list_next (tmp)) { >+ GnlObject *object = (GnlObject *) tmp->data; >... >+ if ((!reverse && comp->priv->segment_stop < object->stop) || >+ (reverse && comp->priv->segment_start > object->start)) { You should not have to iterate here, object_starts and object_stops are sorted, so choosing the first of object_starts for reverse, and last of object_stops for normal playback should be sufficiant. gnlcomposition: add eos_thread_function to replace eos_main_thread. + This is a WIP commit. Sqash with "composition: Cleanup and fix commit introducing our own thread", write a proper commit message, choose a main author, add a Signed-off-by: for the second author if you think it's appropriate (honestly second patch removed mostly all the first one). gnlcomposition: remove unused pending_idle private member >@@ -92,11 +92,9 @@ struct _GnlCompositionPrivate > thread-safe Seek handling. > flushing_lock : mutex to access flushing and pending_idle > flushing : >- pending_idle : > */ Trailing white space. Maybe squask this commit too. composition: Rename "eos thread" to "update pipeline thread" I would squash this too.
Ok, as per IRC discussion, all have been updated, iteration part is kept for now, white space was already there, and the rest was squashed for ease of reading. GNL is unreleased yet, so it's not affected by the freeze (while this is arguably bug fixing). 11 files changed, 525 insertions(+), 444 deletions(-) commit 8b1e549921f2543e94ffc1f8cbd6dc0ab2aa1ed4 Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Mon May 27 20:52:36 2013 +0200 tests: sanitization. + Create a no_install libtestutils. + Remove code from common.h to put it in common.c + Abstract away bus polling from test_simplest # TODO use it as much as possible. + Fix various little errors spotted thanks to new flags. commit 7ac0f028b1ebe0a3a8e06c98b1773a200335ebea Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Sat May 25 03:05:40 2013 +0200 gnlcomposition: When removing a ghostpad, remove the probe_handlers associated to its target. commit e710031b1dd5a442f8ecb9520cb563e1e28fdd84 Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Sat May 25 03:43:20 2013 +0200 gnlcomposition: replace eos_main_thread with update_pipeline_func. + This function is called in a thread of its own. commit 59092b1d764a2b6d1d3016f8691ab46cfb420f8f Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Fri May 24 16:23:26 2013 +0200 gnlcomposition: Forward EOS from the streaming thread when appropriate.