After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 368536 - bin_change_state makes children go through unnecessary state changes
bin_change_state makes children go through unnecessary state changes
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 0.10.25
Assigned To: Wim Taymans
GStreamer Maintainers
: 537033 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2006-10-31 20:46 UTC by Antoine Tremblay
Modified: 2009-08-18 09:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch to fix the issue (5.19 KB, patch)
2006-10-31 20:47 UTC, Antoine Tremblay
none Details | Review
Patch that fixes problem but takes into account the elements that are changing state (2.31 KB, patch)
2006-11-06 18:15 UTC, Antoine Tremblay
none Details | Review
missing unref (2.32 KB, patch)
2006-11-28 18:36 UTC, Yves Lefebvre
none Details | Review
patch updated for 0.10.20 (4.70 KB, patch)
2008-09-09 20:46 UTC, Antoine Tremblay
none Details | Review
Updated patch fixed ASYNC changes bug (4.56 KB, patch)
2008-09-24 21:55 UTC, Antoine Tremblay
none Details | Review
Updated patch (1.99 KB, patch)
2009-01-22 20:15 UTC, Antoine Tremblay
none Details | Review
Fix stupid mistake in last patch (1.99 KB, patch)
2009-01-22 21:46 UTC, Antoine Tremblay
none Details | Review
Fix stupid mistake in last patch (1.99 KB, patch)
2009-01-22 21:46 UTC, Antoine Tremblay
none Details | Review
Improved patch (2.42 KB, patch)
2009-08-14 14:49 UTC, Wim Taymans
committed Details | Review

Description Antoine Tremblay 2006-10-31 20:46:10 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)
Comment 1 Antoine Tremblay 2006-10-31 20:47:01 UTC
Created attachment 75738 [details] [review]
proposed patch to fix the issue
Comment 2 Antoine Tremblay 2006-11-06 18:15:13 UTC
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
Comment 3 Yves Lefebvre 2006-11-28 18:36:39 UTC
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.
Comment 4 Wim Taymans 2007-02-27 18:16:19 UTC
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.
Comment 5 Christian Fredrik Kalager Schaller 2008-02-11 18:14:10 UTC
Wim should this patch be merged now? 
Comment 6 Michael Smith 2008-06-09 19:26:15 UTC
*** Bug 537033 has been marked as a duplicate of this bug. ***
Comment 7 Antoine Tremblay 2008-09-09 20:46:43 UTC
Created attachment 118386 [details] [review]
patch updated for 0.10.20
Comment 8 Antoine Tremblay 2008-09-09 20:47:27 UTC
Wim after almost 2 years could we commit this ?

I added an updated patch for 0.10.20....


Comment 9 Antoine Tremblay 2008-09-24 21:09:57 UTC
The patch seems to be broken in 0.10.20 .. it breaks ASYNC changes ..

I'm working on it.. 
Comment 10 Antoine Tremblay 2008-09-24 21:55:54 UTC
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...
Comment 11 Antoine Tremblay 2009-01-22 20:15:25 UTC
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..
Comment 12 Antoine Tremblay 2009-01-22 21:46:18 UTC
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..
Comment 13 Antoine Tremblay 2009-01-22 21:46:43 UTC
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..
Comment 14 Sebastian Dröge (slomo) 2009-05-07 13:29:38 UTC
Wim, is this patch good for comitting now?
Comment 15 Wim Taymans 2009-08-14 14:49:52 UTC
Created attachment 140780 [details] [review]
Improved patch

Updated patch that moves the check to a place where we can take appropriate locking.
Comment 16 Sebastian Dröge (slomo) 2009-08-18 06:48:42 UTC
Works fine for me
Comment 17 Sebastian Dröge (slomo) 2009-08-18 09:39:47 UTC
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.