Allow overriding stream list implementation

[mildsunrise]

Hello! This PR proposes to refactor the logic managing the stream_list (which is currently an intrusive singly linked list) into its own functions:

srtp_err_status_t srtp_stream_list_create(srtp_stream_list_t *list);
void              srtp_stream_list_insert(srtp_stream_list_t *list, srtp_stream_t stream);
srtp_stream_t     srtp_stream_list_get(srtp_stream_list_t *list, uint32_t ssrc);
srtp_stream_t     srtp_stream_list_delete(srtp_stream_list_t *list, uint32_t ssrc);
void              srtp_stream_list_for_each(srtp_stream_list_t *list, int (*callback)(srtp_stream_t, void *), void *data);
srtp_err_status_t srtp_stream_list_dealloc(srtp_stream_list_t *list, srtp_stream_t template_);

These functions are then gated by a define, so that downstreams can remove the default linked list implementation and replace it with one suited to their needs.

More precisely, the linked list is generalized to an opaque API (stream_list_priv.h) with only the semantics I thought we'd need (retrieval, insertion, removal, and iteration that allows removing the currently iterating element) documented. It's still an internal API, we're not asking for stability on these semantics or signatures; the only requirement is that stream_list must be updated only through this API, and that this file is kept up to date with any semantics the rest of the code relies on.

It's an alternative to #508 that we believe should be more flexible for everybody and hopefully introduce less maintenance burden on your side, as well as avoid changing the current behavior. What are your thoughts on it?
/cc @murillo128

[murillo128]

@fluffy @bifurcation could you take a look at this, please?

[pabuhler]

In general I think this is a better approach, did a quick review but will try to look in more detail in the next days.

With this API there is the assumption that inserting can never fail, maybe it should return a status.

[mildsunrise]

Yes, inserting a duplicate key is defined as UB. This is because in the current code, when inserting a stream we don't check that another one with the same SSRC already exists. Since we make this assumption, in order to keep the current performance it's necessary to allow implementations to assume that as well... otherwise the default implementation for srtp_stream_list_insert would now need to iterate through the whole linked list to check. Should the code need a checked insertion, it can always call srtp_stream_list_get first to ensure the SSRC doesn't already exist.

This was the reasoning but of course it's easy to change if you prefer a checked insertion, or an advisory returned status

[pabuhler]

I was more thinking if it failed due to some capacity issues or something similar in what ever list implementation was used. If the item did not actually get put in to a list it would cause a memory leak.

[mildsunrise]

I see, that makes sense. I've amended stream_list_priv.h with the new semantics, and added fallback paths in all usages of srtp_stream_list_insert. The fallback path just deallocates the stream and aborts. It may sometimes leave the srtp context in a partially modified state, but that's better than a leak and we do it in other places too.

[fluffy]

So first, I think refactoring along theses lines sounds like a good idea and makes sense. I would want to be very careful this did not break things and I have not carefully reviewed the code but I do like the basic idea.

[fluffy]

I think it we had this, it would make it much easier to also put the optimized version from 508 into the main repo and not just have it in a downstream.

[mildsunrise]

I would want to be very careful this did not break things and I have not carefully reviewed the code but I do like the basic idea.

Agree. Most of the changes just extract code into the functions, but there are 2 places (feac4ce and 6653a86) where I've had to change behavior a bit to generalize, which need thorough review.

[dsdolzhenko]

@mildsunrise Hi! I've tried to implement a custom stream_list in a project that uses libsrtp with your changes included. However, stream_list_priv.h is not part of the public headers set, at least when built with meson as a dependency. Without srtp_stream_t's definition being available in a downstream, it seems to be impossible to create a custom implementation of stream_list. Is it expected to be so?

[pabuhler]

@dsdolzhenko I will try the same, I am not sure that this "feature" will just work using the build files provided with libsrtp out of the box, we would have to think if that is what we would want to support. I have the feeling that to start with this feature is more for custom builds of libsrtp where teams have their own build files for libsrtp.

[mildsunrise]

This is the intent, yes. As mentioned this is an internal API and thus doesn't have the stability guarantees that public APIs have, it's targeted towards projects that do custom builds. We can always support more use cases later, but in any case I'd personally leave the changes sitting there for some time before stabilizing.

[dsdolzhenko]

Ok, got it. Thank you for the clarifications and for the MR ;)

[pabuhler]

Hi @mildsunrise, it has been awhile but I am now trying to get this in. I have made some local api changes, my suggestion is

srtp_stream_list_t srtp_stream_list_alloc();
srtp_err_status_t srtp_stream_list_dealloc(srtp_stream_list_t list);
srtp_err_status_t srtp_stream_list_insert(srtp_stream_list_t list, srtp_stream_t stream);
srtp_stream_t srtp_stream_list_get(srtp_stream_list_t list, uint32_t ssrc);
void srtp_stream_list_remove(srtp_stream_list_t list, srtp_stream_t stream);
void srtp_stream_list_for_each(srtp_stream_list_t list, int (*callback)(srtp_stream_t, void *), void *data);

and have changed the internal implementation to be a double linked list. I am not sure how you have replaced the api in your code so I am not sure if this will break anything for you.
My motivation was to make it a little more consistent with other LibSRTP api's and have the list have as little knowledge as possible about LibSRTP internals.
If this ok for you I will update this PR and you can have a look in more detail.

[pabuhler]

Commit 914cb55 in #632 contains some small proposed changes to this PR. If they look ok in general then will merge them here for complete review.

@mildsunrise do you think it is still compatible with want you initially wanted?

[bifurcation]

I agree that this seems like a good approach. Happy to do a detailed review once we make a decision on #632.

Also agree with @fluffy that it would be good to fold in #508 after we merge this. Together with the fact that people seem to want to do other proprietary implementations, maybe we want three states for the implementation:

• Linked list (default) - the current implementation
• Static array
• Custom (no implementation built, so as not to conflict with custom)

This seems analogous to the crypto library selection, so probably good to do this switching in the same way -- put these in separate files and switch among them using the build system.

[mildsunrise]

if I'm not missing anything, changes proposed by @pabuhler in #632 are:

• create renamed to alloc, delete renamed to remove
• default implementation is now a doubly linked list, so there's an additional intrusive field in streams
• list implementation now allocates its own storage
• dealloc no longer deallocates streams, instead it checks that list is empty
• remove now takes the stream instead of the key

I'm happy with the changes and yes, this is compatible with what we had.

the only thing I want to point out is the last change, where the stream parameter is effectively an iterator for the internal (intrusive) implementation, but not other implementations. so your code shouldn't expect O(1) removes; if that may be needed, we should extend the API to expose iterators I think.

if not the case then I'm 👍 on merging #632 and closing this

[pabuhler]

Thanks @mildsunrise & @bifurcation ,

@mildsunrise you summary is correct.
If needed we could also add a take() function that combines both the get & remove in order to provide fast removal in both the lookup and iterate use cases.
I will merge the changes from #632 in to this PR and than we can try to get a complete review and get this in.

[bifurcation]

Partial review here, just covering the high-level design.

[bifurcation]

Overall, with the discussion from the first review, this looks about right. Just a bunch of minor things to fix.

[bifurcation]

Thanks for all the work on this @mildsunrise and @pabuhler. Looks good to me with the latest changes.

[mildsunrise]

Thank you everyone :) @murillo128 I'll integrate this into the media server when I find a spot