<p><a href="https://gerrit.asterisk.org/c/asterisk/+/14577">View Change</a></p><p>16 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/format_cap.h">File include/asterisk/format_cap.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/format_cap.h@310">Patch Set #2, Line 310:</a> <code style="font-family:monospace,monospace"> * If buf is NULL the function will internally allocate memory for the ast_str and</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Is there precedent for doing things like this already? (Just out of curiosity).</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h">File include/asterisk/stream.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@129">Patch Set #2, Line 129:</a> <code style="font-family:monospace,monospace"> /*! Which if the lists to "prefer" */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">of</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@207">Patch Set #2, Line 207:</a> <code style="font-family:monospace,monospace">/*!</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Spacing these out would be good</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@289">Patch Set #2, Line 289:</a> <code style="font-family:monospace,monospace"> enum ast_stream_codec_negotiation_prefs_prefer_values prefer;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">These should be doxygenified</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@634">Patch Set #2, Line 634:</a> <code style="font-family:monospace,monospace"> * The resulting stream will contain all of the attributes of the pending stream</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">By attributes do you mean metadata and such as well?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Yes. The returned stream is a clone of the pending stream with the format caps replaced.<br>I'll update the description to include metadata.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@647">Patch Set #2, Line 647:</a> <code style="font-family:monospace,monospace"> struct ast_stream *configired_stream, struct ast_stream_codec_negotiation_prefs *prefs,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">configured_stream</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@735">Patch Set #2, Line 735:</a> <code style="font-family:monospace,monospace">int ast_stream_topology_get_valid_count(const struct ast_stream_topology *topology);</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I don't like valid as a name here... what about get_active_count?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Yeah I went back and forth on many of the names. :)<br>active it is.<br>Thank god for Eclipse global renaming.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@883">Patch Set #2, Line 883:</a> <code style="font-family:monospace,monospace"> * \note The stream topology is NOT altered by this function.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">State that the returned format capabilities should be freed by the caller</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/format_cap.c">File main/format_cap.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/format_cap.c@61">Patch Set #2, Line 61:</a> <code style="font-family:monospace,monospace"> /*! \brief internal string to hold prefernece names */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">preference</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/format_cap.c@720">Patch Set #2, Line 720:</a> <code style="font-family:monospace,monospace"> if (!cap->preference_names) {</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">rats. <br>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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c">File main/stream.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@117">Patch Set #2, Line 117:</a> <code style="font-family:monospace,monospace"> struct ast_str *display_str;</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">doxygen</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">gonna chuck the concept of storing the string representations on the objects.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@130">Patch Set #2, Line 130:</a> <code style="font-family:monospace,monospace"> int final;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">doxygen</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@353">Patch Set #2, Line 353:</a> <code style="font-family:monospace,monospace"> if (!stream->display_str) {</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Does this present a race condition whereby two threads call this at the same time? (Streams are supposed to be immutable)</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">yeah gonna scrap the concept.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@961">Patch Set #2, Line 961:</a> <code style="font-family:monospace,monospace"> if (!topology->display_str) {</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Same question here as I had for streams - is this safe?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">nope. gonna scrap.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@1092">Patch Set #2, Line 1092:</a> <code style="font-family:monospace,monospace"> return ao2_bump(joint_topology);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Must we RAII_VAR? 😄</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/tests/test_format_cap.c">File tests/test_format_cap.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/tests/test_format_cap.c@1159">Patch Set #2, Line 1159:</a> <code style="font-family:monospace,monospace"> ast_str_reset(buffer);</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Yes. This was the only place where we re-used the ast_str.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/14577">change 14577</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/14577"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I2df77dedd0c72c52deb6e329effe057a8e06cd56 </div>
<div style="display:none"> Gerrit-Change-Number: 14577 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 29 Jun 2020 13:41:05 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>