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
[G-API] Pipeline modeling tool: Skip frames #21719
[G-API] Pipeline modeling tool: Skip frames #21719
Conversation
auto call_every_nth = | ||
call_every_nth_opt.has_value() ? call_every_nth_opt.value() : 1; | ||
if (call_every_nth <= 0) { | ||
throw std::logic_error(node_name + " call_every_nth must be greater than zero"); |
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.
suggest to print value also
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.
Good point
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.
Done
@@ -326,7 +335,7 @@ int main(int argc, char* argv[]) { | |||
} | |||
auto output = | |||
check_and_read<OutputDescr>(node_fn, "output", node_name); | |||
builder.addDummy(node_name, time, output); | |||
builder.addDummy(node_name, call_every_nth_u, time, output); |
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.
how's about making struct for call_every_nth_u
and further possible params?
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.
Do you mean like this:
builder.addDummy(NodeParams{node_name, call_every_nth_u}, time, output);
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.
👍🏻 yes, something like that
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.
Done
// G-API doesn't support dynamic number of inputs/outputs. | ||
if (inputs.size() > 1u) { | ||
throw std::logic_error( | ||
"skip_frame_nth is supported only for single input subgraphs"); |
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.
printing of inputs.size()
will help in troubleshooting
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.
Good point.
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.
Done
call.run(subgr_inputs, subgr_outputs); | ||
auto comp = cv::GComputation(cv::GProtoInputArgs{subgr_inputs}, | ||
cv::GProtoOutputArgs{subgr_outputs}); | ||
call = CallNode{call.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.
do we over-wrap existing call operation with call_every_nth != 1 by a new operation with call_every_nth == 1 which has a counter and the same comp?
If my understanding is correct than in this case do we just return the previous Mat without actual calculation? if yes then why it is called as "skip frames"? I thought that by skip frames it means receiving something empty frames of increasing next frame awaiting duration
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.
Agree, naming is confusing. This is about calling some nodes(kernels) on every n
iterations. (not on every iteration)
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.
I see,
Then question - Does it affect final measurement is some sort of way? maybe it had better to print information about node invocation count also in final report?
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.
If node works not on every frame, it obviously improves performance.
maybe it had better to print information about node invocation count also in final report
Not sure that this is useful info, let's wait for the feedback firstly
…eline_modeling_tool-skip-frames-for-nodes
state.reset(new SubGraphState{}); | ||
state->cc = comp.compile(in, std::move(compile_args)); | ||
|
||
GAPI_Assert(state->cc.outMetas().size() == 1u); |
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.
There is no sense to double check it since it was checked in outMeta
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.
Done
@@ -326,7 +335,7 @@ int main(int argc, char* argv[]) { | |||
} | |||
auto output = | |||
check_and_read<OutputDescr>(node_fn, "output", node_name); | |||
builder.addDummy(node_name, time, output); | |||
builder.addDummy(node_name, call_every_nth_u, time, output); |
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.
Do you mean like this:
builder.addDummy(NodeParams{node_name, call_every_nth_u}, time, output);
call.run(subgr_inputs, subgr_outputs); | ||
auto comp = cv::GComputation(cv::GProtoInputArgs{subgr_inputs}, | ||
cv::GProtoOutputArgs{subgr_outputs}); | ||
call = CallNode{call.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.
Agree, naming is confusing. This is about calling some nodes(kernels) on every n
iterations. (not on every iteration)
// G-API doesn't support dynamic number of inputs/outputs. | ||
if (inputs.size() > 1u) { | ||
throw std::logic_error( | ||
"skip_frame_nth is supported only for single input subgraphs"); |
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.
Good point.
auto call_every_nth = | ||
call_every_nth_opt.has_value() ? call_every_nth_opt.value() : 1; | ||
if (call_every_nth <= 0) { | ||
throw std::logic_error(node_name + " call_every_nth must be greater than zero"); |
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.
Good point
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.
LGTM
08d11b9
to
702cfb0
Compare
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.
Have a little idea what happens here but hope the app is ok.
Please provide a meaningful description in the future, it will be mandatory for all our MRs.
SubGraphState& state) { | ||
// NB: Make a call on the first iteration and skip the furthers. | ||
if (state.call_counter == 0) { | ||
state.cc(in, state.last_result); |
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.
So there may be an extra copy, right? Since this internal GComputation has its own writeBack()
inside.
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.
If fact, there is extra copy.
https://github.com/opencv/opencv/blob/4.x/modules/gapi/src/executor/gexecutor.cpp#L163
because adapter->data() == out_mat.data
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.
Sorry, there is NO extract copy
Co-authored-by: Dmitry Matveev <dmitry.matveev@intel.com>
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.
👍
@alalek Could you merge it, please? |
…ing_tool-skip-frames-for-nodes [G-API] Pipeline modeling tool: Skip frames * Add skip feature * Refactoring * Fix warning * Put more comments * Fix comments to review * Agregate common params into structure * Fix warning * Clean up & add test * Add assert * Fix warning on Mac * Update modules/gapi/samples/pipeline_modeling_tool.cpp Co-authored-by: Dmitry Matveev <dmitry.matveev@intel.com>
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.
Build configuration