[Asterisk-code-review] RFC sdp: Initial SDP creation (asterisk[master])

[George Joseph]

George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/5119 )

Change subject: RFC sdp: Initial SDP creation
......................................................................


Patch Set 3:

(13 comments)

https://gerrit.asterisk.org/#/c/5119/3/include/asterisk/sdp_options.h
File include/asterisk/sdp_options.h:

Line 66: struct ast_sdp_options {
> I'm not 100% sure how I feel about the options structure no longer being op
Done


Line 120: 
> If we move forward with making ast_sdp_options non-opaque (transparent?), t
> If we move forward with making ast_sdp_options non-opaque
 > (transparent?), then everything from this line down can just be
 > removed since you can just set and get the fields of the structure
 > directly.

no need.  stuck with opaque.


https://gerrit.asterisk.org/#/c/5119/3/include/asterisk/sdp_state.h
File include/asterisk/sdp_state.h:

PS3, Line 58: /*!
            :  * \brief Get the local topology
            :  *
            :  */
            : struct ast_stream_topology *ast_sdp_state_get_local_topology(struct ast_sdp_state *sdp_state);
            : 
            : /*!
            :  * \brief Get the sdp_state options
            :  *
            :  */
            : struct ast_sdp_options *ast_sdp_state_get_options(struct ast_sdp_state *sdp_state);
> I think both of these functions should return const structures. This way, i
Done


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

PS3, Line 206: /*!
             :  * \brief Get the opaque stream data
             :  *
             :  * \param stream The media stream
             :  *
             :  * \retval non-NULL success
             :  * \retval NULL failure
             :  *
             :  * \since 15
             :  */
             : void *ast_stream_get_data(struct ast_stream *stream);
             : 
             : /*!
             :  * \brief Set the opaque stream data
             :  *
             :  * \param stream The media stream
             :  * \param data Opaque data
             :  * \param data_free_fn Callback to free data when stream is freed. May be NULL for no action.
             :  *
             :  * \return data
             :  *
             :  * \since 15
             :  */
             : void *ast_stream_set_data(struct ast_stream *stream, void *data,
             : 	ast_stream_data_free_fn data_free_fn);
> It depends on the purpose of the data. If it's something you are frequently
> It depends on the purpose of the data. If it's something you are
 > frequently using in the media path, you want access to it fast. The
 > faster the better, so direct access like this is good.

using array with enum.


https://gerrit.asterisk.org/#/c/5119/3/main/sdp.c
File main/sdp.c:

> I'm not going to point out every single place this applies, but consider co
Done


Line 36: 	ast_debug(5, "free\n");
> All of these debug lines should either be removed or expanded on so they're
Done


PS3, Line 58: void ast_sdp_payload_free(struct ast_sdp_payload *payload)
            : {
            : 	ast_debug(5, "free\n");
            : 	return;
            : }
> This function should be calling
Done


Line 474: 	char rtmap[64];
> The rtmap buffer isn't used for anything in this function. Its size is take
Done


PS3, Line 612: 		if (ast_sdp_m_add_format(m_line, options, rtp_code, 0, format, 0)) {
             : 			ast_sdp_m_free(m_line);
             : 			return -1;
             : 		}
> This off-nominal leaks the "format" variable.
Done


Line 726: 	if (!o_line) {
> This should be
Done


PS3, Line 721: o_line = ast_sdp_o_alloc("user", 99999, 0, "IN4", "127.0.0.1");
             : 	if (!o_line) {
             : 		goto error;
             : 	}
             : 	c_line = ast_sdp_c_alloc("IN4", "127.0.0.1");
             : 	if (!o_line) {
             : 		goto error;
             : 	}
> Since you added the media_address SDP option, we should be able to fill in 
Done


Line 731: 	if (!o_line) {
> This should be
Done


Line 734: 	t_line = ast_sdp_t_alloc(999, 9999);
> There's not a good reason to be allocating t-lines for SDPs we generate. If
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefbd877af7f5a4d3c74deead1bff8802661b0d48
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes