[Asterisk-code-review] Streams: Add features for Advanced Codec Negotiation (asterisk[master])

Mon Jun 29 08:41:05 CDT 2020

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

Change subject: Streams:  Add features for Advanced Codec Negotiation
......................................................................

Patch Set 2:

(16 comments)

https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/format_cap.h 
File include/asterisk/format_cap.h:

https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/format_cap.h@310 
PS2, Line 310:  * If buf is NULL the function will internally allocate memory for the ast_str and
> Is there precedent for doing things like this already? (Just out of curiosity).

There isn't.  It's come up because there were so many places that needed the debug output and it just wasn't practical to keep allocating ast_str's in all the places it was needed.  Gonna scrap the idea and re-think.

https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h 
File include/asterisk/stream.h:

https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@129 
PS2, Line 129: 	/*! Which if the lists to "prefer" */
> of
Ack

https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@207 
PS2, Line 207: /*!
> Spacing these out would be good
Ack

https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@289 
PS2, Line 289: 	enum ast_stream_codec_negotiation_prefs_prefer_values prefer;
> These should be doxygenified
Ack

https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@634 
PS2, Line 634:  * The resulting stream will contain all of the attributes of the pending stream
> By attributes do you mean metadata and such as well?

Yes.  The returned stream is a clone of the pending stream with the format caps replaced.
I'll update the description to include metadata.

https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@647 
PS2, Line 647: 	struct ast_stream *configired_stream, struct ast_stream_codec_negotiation_prefs *prefs,
> configured_stream
Ack

https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@735 
PS2, Line 735: int ast_stream_topology_get_valid_count(const struct ast_stream_topology *topology);
> I don't like valid as a name here... what about get_active_count?

Yeah I went back and forth on many of the names. :)
active it is.
Thank god for Eclipse global renaming.

https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@883 
PS2, Line 883:  * \note The stream topology is NOT altered by this function.
> State that the returned format capabilities should be freed by the caller
Ack

https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/format_cap.c 
File main/format_cap.c:

https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/format_cap.c@61 
PS2, Line 61: 	/*! \brief internal string to hold prefernece names */
> preference
Ack

https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/format_cap.c@720 
PS2, Line 720: 		if (!cap->preference_names) {
> This will come up later - but is this safe to do? What if two threads access it? Would using thread local storage be better so the underlying caps remains protected?

rats.  
The issue with TLS is that the thread that allocates it might not be the thread that causes the destructor to run.  I guess I'll have to re-think this a bit.  Maybe I can create an ast_str in the scope trace stuff, since that's where this capability is used, then free it when the scope ends.

https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c 
File main/stream.c:

https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@117 
PS2, Line 117: 	struct ast_str *display_str;
> doxygen

gonna chuck the concept of storing the string representations on the objects.

https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@130 
PS2, Line 130: 	int final;
> doxygen
Ack

https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@353 
PS2, Line 353: 		if (!stream->display_str) {
> Does this present a race condition whereby two threads call this at the same time? (Streams are supposed to be immutable)

yeah gonna scrap the concept.

https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@961 
PS2, Line 961: 		if (!topology->display_str) {
> Same question here as I had for streams - is this safe?

nope.  gonna scrap.

https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@1092 
PS2, Line 1092: 	return ao2_bump(joint_topology);
> Must we RAII_VAR? 😄
Ack

https://gerrit.asterisk.org/c/asterisk/+/14577/2/tests/test_format_cap.c 
File tests/test_format_cap.c:

https://gerrit.asterisk.org/c/asterisk/+/14577/2/tests/test_format_cap.c@1159 
PS2, Line 1159: 	ast_str_reset(buffer);
> Has the behavior of the underlying function been altered for ast_format_cap_get_names such that you have to explicitly pass in a new one, or reset it yourself?

Yes.  This was the only place where we re-used the ast_str.

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/14577
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I2df77dedd0c72c52deb6e329effe057a8e06cd56
Gerrit-Change-Number: 14577
Gerrit-PatchSet: 2
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Mon, 29 Jun 2020 13:41:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200629/40564e5c/attachment.html>