GNOME Bugzilla – Bug 368536
bin_change_state makes children go through unnecessary state changes
Last modified: 2009-08-18 09:39:47 UTC
I noticed that in a bin when doing a change_state lets say from PLAYING to NULL , change_state is called of course and it changes the children state to the next state in the transition namely here PAUSE , but the problem is it does this without checking the children current state, if the children was in state NULL for example there's no reason to bring it to PAUSED to go to NULL after .. This could cause problems when for exemple the change state of the element to PAUSE would fail and then it would make the pipeline state change form PLAYING to NULL fail also , even if there's no reason to fail here... So here's a patch to check the children state and to skip all children state changes that are not in the same direction as the bin state changes (bin changes downward , children changes downwards only too). Also if the children state is already at the bin's pending state it will skip the transition since it's not needed either... Note that I still set the base_time even if I skip the transition just to be sure... I was unsure about this.. ? feel free to comment Also I could commit this myself ( and would like too ) :) .. just tell me if you guys approuve the patch or if it needs changes... (Tested with my regression , I could not see any problems)
Created attachment 75738 [details] [review] proposed patch to fix the issue
Created attachment 76101 [details] [review] Patch that fixes problem but takes into account the elements that are changing state I forgot to take into account elements that could be changing state... this patch takes it into account , checking that the child pending state is in line with the bin change sate if there's a pending state on the child, rather then checking the current state
Created attachment 77304 [details] [review] missing unref There is a missing unref in the "else" of the previous patch. This cause some memleak since the finalize fonction of filter was not called when I test in my setup.
I'm considering adding this patch add some point, currently reworking the state changes a little to add the needed latency calculations, after that I'll take a deeper look.
Wim should this patch be merged now?
*** Bug 537033 has been marked as a duplicate of this bug. ***
Created attachment 118386 [details] [review] patch updated for 0.10.20
Wim after almost 2 years could we commit this ? I added an updated patch for 0.10.20....
The patch seems to be broken in 0.10.20 .. it breaks ASYNC changes .. I'm working on it..
Created attachment 119337 [details] [review] Updated patch fixed ASYNC changes bug My previous condition to check if we should skip was a stupid mistake :( .. don't know how I could do this.. sorry :( now I only allow skipping a state if there is not pending states for the element so it should be ok with ASYNC state changes... if(child_pending != GST_STATE_VOID_PENDING || (child_pending == GST_STATE_VOID_PENDING && ((pending > child_current && next > child_current) || (pending < child_current && next < child_current )))) Please review...
Created attachment 127033 [details] [review] Updated patch This patch check for the last return value of the elements that are skipped by the state change, this makes sure we do not skip a NO_PREROLL return value for example..
Created attachment 127042 [details] [review] Fix stupid mistake in last patch I was using the bin's return state rather then then client elements, fixed that..
Created attachment 127043 [details] [review] Fix stupid mistake in last patch I was using the bin's return state rather then then child elements, fixed that..
Wim, is this patch good for comitting now?
Created attachment 140780 [details] [review] Improved patch Updated patch that moves the check to a place where we can take appropriate locking.
Works fine for me
commit bf8af3f734acd6fe6df84c380124f9fd89370d6e Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Tue Aug 18 11:38:38 2009 +0200 docs: Update the design docs for bin state changes according to last commit commit c87d55170512153d3025f32275f198e9b86f1172 Author: Antoine Tremblay <hexa00@gmail.com> Date: Tue Aug 18 11:36:36 2009 +0200 gstbin: Don't try to change children's state if they're already in the state Fixes bug #368536.