[Asterisk-code-review] stream: Add media stream topology definition and API (asterisk[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/4922 )

Change subject: stream:  Add media stream topology definition and API
......................................................................


Patch Set 3: Code-Review-1

(3 comments)

https://gerrit.asterisk.org/#/c/4922/3/include/asterisk/stream.h
File include/asterisk/stream.h:

PS3, Line 310:  * \note The format capabilities reference is NOT altered by this function
             :  * since a new format capabilities structure is created for each media type.
             :  *
             :  * \note Each stream will have its name set to the corresponding media type.
             :  * For example: "AST_MEDIA_TYPE_AUDIO".
             :  *
             :  * \note Each stream will be set to the sendrecv state.
             :  *
             :  * \since 15
             :  */
             : struct ast_stream_topology *ast_stream_topology_create_from_format_cap(
             : 	struct ast_format_cap *cap);
If cap is not meant to be altered then make it 'const'.


https://gerrit.asterisk.org/#/c/4922/3/main/stream.c
File main/stream.c:

Line 123: 	return stream ? stream->name : NULL;
I think an assert on a NULL stream would be better for this function and all the others similarly done.


PS3, Line 319: 	for (type = AST_MEDIA_TYPE_UNKNOWN + 1; type < AST_MEDIA_TYPE_END; type++) {
             : 		struct caps_wrapper *wrapper;
             : 
             : 		if (!ast_format_cap_has_type(cap, type)) {
             : 			continue;
             : 		}
             : 
             : 		wrapper = ast_malloc(sizeof(*wrapper));
             : 		if (!wrapper) {
             : 			goto error;
             : 		}
             : 
             : 		wrapper->cap = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT);
             : 		if (!wrapper->cap) {
             : 			wrapper_destructor(wrapper);
             : 			goto error;
             : 		}
             : 
             : 		wrapper->type = type;
             : 		ast_format_cap_set_framing(wrapper->cap, ast_format_cap_get_framing(cap));
             : 		ast_format_cap_append_from_cap(wrapper->cap, cap, type);
             : 
             : 		if (AST_VECTOR_APPEND(&new_caps, wrapper)) {
             : 			wrapper_destructor(wrapper);
             : 			goto error;
             : 		}
             : 	}
             : 
             : 	for (i = 0; i < AST_VECTOR_SIZE(&new_caps); i++) {
             : 		struct caps_wrapper *wrapper = AST_VECTOR_GET(&new_caps, i);
             : 		struct ast_stream *stream = ast_stream_create(type_names[wrapper->type], type);
             : 
             : 		if (!stream) {
             : 			goto error;
             : 		}
             : 
             : 		stream->formats = wrapper->cap;
             : 		wrapper->cap = NULL;
             : 		stream->state = AST_STREAM_STATE_SENDRECV;
             : 		if (!ast_stream_topology_append_stream(topology, stream)) {
             : 			goto error;
             : 		}
             : 	}
Do you need an intermediary structure vector here and two loops? It looks like you could combine the two into one and drop the new_caps vector?


-- 
To view, visit https://gerrit.asterisk.org/4922
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic930232d24d5ad66dcabc14e9b359e0ff8e7f568
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-HasComments: Yes