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 584536 - [PLUGIN-MOVE] Move shapewipe to -good
[PLUGIN-MOVE] Move shapewipe to -good
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 0.10.19
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-06-01 19:27 UTC by Sebastian Dröge (slomo)
Modified: 2010-02-12 10:36 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Sebastian Dröge (slomo) 2009-06-01 19:27:13 UTC
Hi,
for the next good/bad releases we should move shapewipe to -good. It has documentation and a unit test, an example application and does everything it should ;)
Comment 1 Tim-Philipp Müller 2009-08-10 16:45:26 UTC
Anyone sponsoring the move, ie. who's done some basic reviewing of the API and code and done some basic testing?
Comment 2 Tim-Philipp Müller 2009-08-11 08:36:43 UTC
Next cycle then I guess.
Comment 3 Sebastian Dröge (slomo) 2010-01-14 19:38:58 UTC
Marking as blocker for next release.
Comment 4 Tim-Philipp Müller 2010-02-04 14:35:38 UTC
So, what's the status of this? Could you run down the moving-plugins check list? If not, there's only a few days left..
Comment 5 Sebastian Dröge (slomo) 2010-02-04 16:59:14 UTC
Well, a second review is missing... so unless someone reviews the element this won't happen this release :)
Comment 6 Wim Taymans 2010-02-04 17:10:19 UTC
I'm willing to review this over the weekend. Is that soon enough to get included in this release?
Comment 7 Tim-Philipp Müller 2010-02-04 17:14:57 UTC
> I'm willing to review this over the weekend. Is that soon enough to get
> included in this release?

Of course.
Comment 8 Wim Taymans 2010-02-07 21:50:36 UTC
Some comments:

 - the default property values need to be #defined at the start and then those defines used in the
  paramspec and property init code.

 - the shutdown case can cause a deadlock. The case is when the state change code from PAUSED to
  READY signals the gcond right before the chain function takes the mutex and starts waiting for the
  gcond. One way of solving this is to use a boolean (flushing or something) to indicate that a wait
  operation is needed or not. Then the flag should be set and the gcond signaled inside the lock (one
  reason why signaling a gcond outside of a lock is not a good idea in most cases).

 - When the shutdown is detected (assuming the deadlock is removed) in the chain function, the chain
   function should return WRONG_STATE, not UNEXPECTED (which is only used for EOS cases).

- When going to READY, gst_shape_wipe_reset() is called, which also resets the property values that
  the user could have configured (or being modified by the user installed controller). This is not
  expected behaviour of an element, the properties should keep the last configured value.

- in the state change function, the if (transition == GST_STATE_CHANGE_PAUSED_TO_READY) should
  simply be moved into a case in the above switch block.

- A flush on the mask sinkpad should remove the queued mask buffer.

- it's nicer in the chain function to handle the error cases (and add associated debug log) at the end
  of the function.
Comment 9 Sebastian Dröge (slomo) 2010-02-08 07:28:00 UTC
Thanks for the review, should all be fixed now:

commit ad7eff41a8f5243df1bd9ff84a121e5e6f90dcab
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Mon Feb 8 08:26:33 2010 +0100

    shapewipe: Improve/add debug output

commit 364c53fd61f57d7f45300bd6be5c4b1f3aad55db
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Mon Feb 8 08:20:44 2010 +0100

    shapewipe: Always hold the mask mutex before signalling the GCond

commit d875dce9bb8b1c48c27b80426a3084987b774839
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Mon Feb 8 08:19:48 2010 +0100

    shapewipe: Move chain function error cases at the end of the function and ad

commit 8d213d51f6557ed9f42f9c3fdd7c7cf5d3af0ac9
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Mon Feb 8 08:12:11 2010 +0100

    shapewipe: Fix race condition during shutdown that can lead to a deadlock

commit 2d1f06103ab44992dc2e5c8f750cf80ad0e1c596
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Mon Feb 8 08:11:33 2010 +0100

    shapewipe: Drop mask buffer on FLUSH events

commit 6df57956512799489f6f4f95083c5a128dd175e2
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Mon Feb 8 08:09:55 2010 +0100

    shapewipe: Update copyright year

commit 87216b83b90a304b2839739c2552a73cf68cb5e0
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Mon Feb 8 08:08:44 2010 +0100

    shapewipe: Don't reset properties when going PAUSED->READY
    
    Also use defines for the default values of the properties.
Comment 10 Tim-Philipp Müller 2010-02-12 09:29:12 UTC
Feel free to move this to -good now. Please add a CRUFT* entry to -bad/Makefile.am as well when you're done.
Comment 11 Sebastian Dröge (slomo) 2010-02-12 10:36:00 UTC
For -bad:

commit ad13ec6933b5f253bd35341e0514d31bfaf5458f
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Fri Feb 12 11:35:02 2010 +0100

    docs: Update documentation

commit f79842e308c50f7e95d24ebb7f9214ba8a9c8804
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Fri Feb 12 11:21:23 2010 +0100

    Moved 'shapewipe' from -bad to -good
    
    Fixes bug #584536.

For -good:

commit 92ddc932ebfeb0a9d093e2d3703d82f08103f42f
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Fri Feb 12 11:32:40 2010 +0100

    docs: Update documentation

commit bc611043a908a22a19a5fc82fb014d5c399cccea
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Fri Feb 12 11:18:26 2010 +0100

    Moved 'shapewipe' from -bad to -good
    
    Fixes bug #584536.