[Asterisk-code-review] Add data buffer API to store packets. (asterisk[15])

[Benjamin Keith Ford]

Benjamin Keith Ford has posted comments on this change. ( https://gerrit.asterisk.org/8604 )

Change subject: Add data buffer API to store packets.
......................................................................


Patch Set 4:

(4 comments)

https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c
File main/data_buffer.c:

https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@228
PS4, Line 228: 		ast_data_buffer_cache_adjust(buffer);
             : 		buffer_payload = data_buffer_payload_alloc(payload, pos);
             : 		if (!buffer_payload) {
             : 			return -1;
             : 		}
> I'm starting to think that the cache might not be useful as well.  If we're
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


https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@237
PS4, Line 237: 		if (existing_payload->pos > pos) {
> I suppose that dealing with RTP seqno wrapping will be something that is de
Yeah, it's up to the consumer to tell the API where to put the packet, which should all be assigned externally


https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@260
PS4, Line 260: 	if (pos == -1) {
> NULL is 0 also.  0 will probably be a valid value in some cases, so you're 
Oh yeah, duh - 0 is NULL :)
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.


https://gerrit.asterisk.org/#/c/8604/4/main/data_buffer.c@260
PS4, Line 260: 	if (pos == -1) {
             : 		buffer_payload = AST_LIST_FIRST(&buffer->payloads);
             : 
             : 		if (buffer_payload) {
             : 			return buffer_payload->payload;
             : 		}
             : 	} else {
             : 		AST_LIST_TRAVERSE(&buffer->payloads, buffer_payload, list) {
             : 			if (buffer_payload->pos == pos) {
             : 				return buffer_payload->payload;
             : 			}
             : 		}
             : 	}
> Is this then just an datastore for objects that can be arbitrarily referenc
Posting this here so that others can see:
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)



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

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff13c5d4795d52356959fe2a57360cd57dfade07
Gerrit-Change-Number: 8604
Gerrit-PatchSet: 4
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 20:17:16 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180321/01a5daa2/attachment.html>