GNOME Bugzilla – Bug 630808
valve: move to core
Last modified: 2010-12-31 02:02:27 UTC
Created attachment 171253 [details] [review] less reffing and locking The valve element is reffing locking too much. I am attaching a patch. If especially the locks where taken on purpose if would be nice to add a comment.
I wonder also if we could use atomic ops to read/write 'drop' as those are cheaper than locks.
Imho the _(set|get)_property methods look fishy. Is it really okay to have the default: case first. I would move it down in any case to be more like elsewhere.
just changed the order in the switch statements (comment #3) and committed.
I don't see why you propose removing the check for the drop after returning from pad_push/pad_alloc? It is for the case where you do a pad block after the valve, set drop=TRUE, remove some elements which could cause an error and then return, we want to suppress that error entirely.
That's why I asked for feedback and think there should be a comment to explain that you explicitly want to re-read the property value after the pad_push. I can make the _ref change and a separate patch that just uses atomic ops instead of the logging.
I made it use an atomic, I fixed the doc, I re-implemented discont support (which was broken years ago, so I guess its not critical, but it's still nice to set the marker bit on RTP). I also committed a simple unit test for valve which should cover most cases (since a pretty simple element). If you guys like it, please merge it. I guess the whole thing could also be merged into identity if we think its a better idea.
merge-> move to good/base
Thanks for the update. This looks good now. I would consider moving it to core actually. It is media-agnostic. Anyway +1 for moving from my side. Oliver, please bug another core-dev to get 2nd approval and opinion on where it should go. Then we can mark it as release blocker for the respective module.
I think that in 0.11, the functionality should be part of GstPad. Especially if we can get rid of the nasty new-segment event. It would make it easier to dynamically modify pipelines.
Moved to core, removed from -bad.