Framesync API #1325
Framesync API #1325
Conversation
Co-authored-by: Niranjan Kumar <niranjan.kumar@ittiam.com> Co-authored-by: Mallikarjun Kamble <mallikarjun.kamble@ittiam.com> Co-authored-by: Nil Fons Miret <nilf@netflix.com>
kylophone
force-pushed
the
framesync_api
branch
from
b4804cb
to
8ca784e
Compare
|
Squashed and rebased, ready for merge. |
| new_buf_node->next = NULL; | ||
| fs_ctx->buf_cnt++; | ||
|
|
||
| new_buf_node->frame_data = *data = malloc(data_sz); |
There was a problem hiding this comment.
nit: There is some duplicated logic here, maybe it is possible to extract out the "create new node" and "allocate node's data" into two functions to be reused. But we can refactor this later.
There was a problem hiding this comment.
Yes, agreed with 2 functions the code can made reusable.
| for (unsigned i = 0; i < fs_ctx->buf_cnt; i++) { | ||
| if ((buf_que->index == index) && (buf_que->buf_status == BUF_ACQUIRED)) { | ||
| buf_que->buf_status = BUF_FILLED; | ||
| if (data != buf_que->frame_data) |
There was a problem hiding this comment.
naive question: what is the purpose of this check?
It seems that to succeed, the pair (index, data) needs to match the pair (buf_que->index, buf_que->frame_data). Curious about:
- Why that condition is needed
- Why the treatment is different if the index doesn't match (do nothing) vs if the index matches but the data pointer doesn't (mark as filled and return -1)
- What if the entire loop finishes without finding a match?
There was a problem hiding this comment.
Forwarded to Ittiam.
There was a problem hiding this comment.
Q1:
Index matching is done to find the node which holds the data buffer of picture "index"
Q2:
Index matching is the primary check to change the buffer status to FILLED,
Data pointer checking is for error checking purpose, which can be cleaned up if not necessary. if the caller has not passed the matching data buffer, then there can be OOB or unallocated buffer access, this additional check is added to catch such cases.
Q3:
This condition is not applicable since the thread which has acquired new buffer for any particular picture index is responsible for submitting it after filling, so index should match with a node in the queue.
There was a problem hiding this comment.
Thanks @IttiamVijayakumarGR for clarifying, I was discussing this with @kylophone and it seemed like the extra check can be cleaned up, so I did that in the PR below. I will leave the last call to Kyle though.
There was a problem hiding this comment.
Discussed this with @nilfm99, we'll merge this one as-is for now to unblock the funque upstreaming work. However, I think it's likely that we'll have some small changes to the api in a later iteration. Since this is an internal api with just one use so far, we do have some flexibility when it comes to refactoring.
Re-opening of PR #1287.