-
Notifications
You must be signed in to change notification settings - Fork 763
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simply metadata propagation API. #1407
base: master
Are you sure you want to change the base?
Conversation
…a.c`. Implement new tests for non-monotnic frames and multiple callbacks.
if (!data) return; | ||
MetaStruct *meta = data; | ||
char key[128], value[128]; | ||
snprintf(key, sizeof(value), "%s_%d", metadata->feature_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sizeof(key)
@@ -46,6 +46,8 @@ typedef struct { | |||
|
|||
typedef struct VmafPredictModel { | |||
VmafModel *model; | |||
unsigned last_highest_seen_index; | |||
unsigned last_lowest_seen_index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why these index tracking fields are needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, as you know if application is multithreaded model scores might be calculated in out of order. Those indexes basically helping them to make in order. I am using those indexes for the current calculated index is in order. For example;
Let's say we have frame order like this:
3, 0, 1, 2
Frame 3 arrives -> Calculate but not propagate, Update last_highest_seen_index
to 3;
Propagated frame -> None
Frame 0 arrives -> Calculate and propagate since frame index (0) == last_lowest_seen_index. Increase last_lowest_seen_index
by 1. It tries to propagate until last_highest_seen_index (3)
but since there is no Frame 1 it stops.
Propagated frame -> Frame 0
Frame 1 arrives -> Calculate and propagate since frame index (1) == last_lowest_seen_index. Increase last_lowest_seen_index
by 1. It tries to propagate until last_highest_seen_index
but since there is no Frame 2 it stops.
Propagated frame -> Frame 1
Frame 2 arrives -> Calculate and propagate since frame index (2) == last_lowest_seen_index. Increase last_lowest_seen_index
by 1. It tries to propagate until last_highest_seen_index
and we have ready Frame 3 so also it propagates that frame.
Propagated frame -> Frame 2, Frame 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a problem if the callbacks come out of order? Using your method, callbacks could be withheld for an unlimited number of frames (1 - N) until frame 0 is ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a problem if the callbacks come out of order?
I don't think it's a problem for us. But for the sake of usability of API I think we need something like this. Otherwise users might need to reorder the frames. For example I am thinking FFmpeg, in that implementation we need to send frames in order to filtergraph (If we want to write features to original frame's metadata otherwise we don't need).
Using your method, callbacks could be withheld for an unlimited number of frames (1 - N) until frame 0 is ready?
Unfortunately, yes. Is there a possibility that frame 0 somehow not calculated? I thought this situation while implementing this and I concluded if somehow one of the frames doesn't calculate then we can't calculate any other future frames due to metrics like Motion
right? So I ended up VMAF can't drop frames.
int flags; | ||
} MetaStruct; | ||
|
||
#include <string.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to add this header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups, no. Sorry. I'll fix asap.
This PR continuation of #1387. Since somehow it's stuck with tests I decided to write changes in clean branch.
Main summary of PR:
scores are complete
processing
test_predict
totest_propagate_metadata
for better test separation.This change eliminates the need for complex reordering logic at the caller level and reduces potential thread synchronization issues by only propagating metadata when all necessary metrics are calculated.
Most of the additions coming from tests. I tried to implement the all possible edge cases that can occur carefully.
Also this PR includes #1406.