<p>Benjamin Keith Ford <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/8604">View Change</a></p><p>Patch set 4:</p><p>(4 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c">File main/data_buffer.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@228">Patch Set #4, Line 228:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ast_data_buffer_cache_adjust(buffer);<br> buffer_payload = data_buffer_payload_alloc(payload, pos);<br> if (!buffer_payload) {<br> return -1;<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'm starting to think that the cache might not be useful as well. If we're</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We talked about this a bit offline and the consensus seemed to be that we are going to have to allocate memory at some point, so maybe having it already in the cache will save us some time. There's also the option of adding a function to get a payload that, if called, also pops off the payloads prior to it and puts them back in the cache if there's room. I couldn't think of a scenario when we would need older payloads, so I'm open to adding this in, but maybe someone knows of a scenario where removing older payloads would cause problems</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@237">Patch Set #4, Line 237:</a> <code style="font-family:monospace,monospace"> if (existing_payload->pos > pos) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I suppose that dealing with RTP seqno wrapping will be something that is de</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yeah, it's up to the consumer to tell the API where to put the packet, which should all be assigned externally</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@260">Patch Set #4, Line 260:</a> <code style="font-family:monospace,monospace"> if (pos == -1) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">NULL is 0 also. 0 will probably be a valid value in some cases, so you're </blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Oh yeah, duh - 0 is NULL :)<br>I think grabbing from the head is a small concern at this point, so this can be ignored and if we need it later it's easy enough to do so.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@260">Patch Set #4, Line 260:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (pos == -1) {<br> buffer_payload = AST_LIST_FIRST(&buffer->payloads);<br><br> if (buffer_payload) {<br> return buffer_payload->payload;<br> }<br> } else {<br> AST_LIST_TRAVERSE(&buffer->payloads, buffer_payload, list) {<br> if (buffer_payload->pos == pos) {<br> return buffer_payload->payload;<br> }<br> }<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is this then just an datastore for objects that can be arbitrarily referenc</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Posting this here so that others can see:<br>We came to the conclusion that for now, the cache is going to store an arbitrary number of allocated memory spots for payloads, and will add that number every time the cache runs out, but only up to the max number of payloads the buffer can hold (e.g., buffer has 10 payloads, its max is 11, cache ran out, we only add 1 payload instead of, say, the 5 we would normally add)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/8604">change 8604</a>. To unsubscribe, 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/8604"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Iff13c5d4795d52356959fe2a57360cd57dfade07 </div>
<div style="display:none"> Gerrit-Change-Number: 8604 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Matthew Fredrickson <creslin@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 21 Mar 2018 20:17:16 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>