search

EricssonResearch / openwebrtc-gst-plugins

OpenWebRTC specific GStreamer plugins
BSD 2-Clause "Simplified" License
51 stars 46 forks source link

SCTP plugin #28

Closed gkiagia closed 9 years ago

gkiagia commented 9 years ago

Implement plugin for using SCTP transport in a pipeline. The use case is building support for WebRTC's Data Channel in openwebrtc.

These plugins are submitted here just because it's easier for the moment. The intention is to have them in gst-plugins-bad at some point.

mparis commented 9 years ago

First at all, good work!! ;) Moreover I have invested some time to review the patch and I have found some memory leaks, some problems managing locks and other minor-issues. I hope that my comments help to improve the code.

gkiagia commented 9 years ago

Thank you for your review. These are the fixes for the issues that you have pointed out, except locking in set/get_property and splitting the members of the element in a private structure.

Regarding locking, I feel it is unnecessary. All the properties are meant to be set before starting the element and they are only used once in the READY to PAUSED state change. I don't see why someone would want to change them simultaneously from different threads at runtime.

Regarding the private structure, I also feel it is unnecessary and complicates the code imho. This is a plugin, not a library, so this structure is never used anywhere outside this implementation. Therefore, I feel that adding yet another structure just complicates the code and has no benefit.

DanielLindstrm commented 9 years ago

Looks good imo.

mparis commented 9 years ago

This changes fix the most important issues. I don't mind about the private structure and the getter/setter thread safe, so it is OK for me ;).

When do you think that these elements will be merged? Is there any example about how to use them?

Best!!

superdump commented 9 years ago

Merged. Thanks!