Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
added cropping of capture/background #153
base: main
Are you sure you want to change the base?
added cropping of capture/background #153
Changes from 11 commits
127009b
65627e8
7b41672
cb0fe4f
3eeb58b
01e9a75
f81e26e
112e979
22472ad
7dc9bcc
6894b3e
1769f7e
e9a5242
0f62a57
81c1c83
4c5f72c
ffb9883
7353ff8
5a92b93
5ee0d4a
44dbb6a
4c6431e
975b96f
481cb47
f80a8a6
ea1e350
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We could lift this calculation out of the processing loop to a structure variable, as sizes will not change during operation.
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.
see below
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.
Size will not change, the frame content will change. Mofifying this implies:
if ! rect_calculated
crop = calc_cropping()
else
crop = get_stored_crop_value()
...
I don't think that the calculation will take a lot ot time more than filling the cv::Rect from already calculated variables.
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.
..as above, could be removed in favour of a state variable, calculated just once.
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 must think about 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.
I must review this part.
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 have put the variable bg_stored into the structure background_t.
The code is now:
For the video processing the code remain to the old code, we will probably every time read a new picture from the video source.
Please note that the changes are not effective now, the tests for video or still image don't work as required.
I propose to detect according to the value cnt which has the value -1 for still image and a greater value for video.
I have also changed the corresponding code.
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.
Don't worry too much about this, it's a minor performance enhancement to avoid a couple of floating point ops per frame, when we are running a whole inference model too.. I'd rather the code was readable than maximum speed 😄
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.
Not sure this is correct? Selecting an ROI from the final output is done because we may have centred the frame into the model earlier (line 289), this change removes the selection step. The calculations at line 237 onward should ensure that
in_roidim
cannot be larger thanofinal
(unless the model itself has output dim < input dim,which I have never seen but I guess could occur?which is indeed the case for the bodypix model). Looks like some assumptions in this code need reviewing, and an additional model output roidim should be calculated to use here.After going away and thinking about this - this is a separate bug that you do not need to fix in this PR. Let's raise another issue for this.
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 modified the original code because I had a crash within opencv.
I ha done this because the model used had an output of 33x33.
I have already performed a lot of tests and never shown an error here. I will check this again.TT
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 have just tested this again. With the old code, using backscrub/models/body-pix-float-050-8.tflite I get again a crash, with my correction this work well.
the real size for ofinal is 33x33 and with the ctx.in_roidim we have a size of 256x256!!!
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.
Yep - it's because they have an output stride variable in the model which defaults to 32, so we get input dim/32 == 257/32 ~= 33.
I'm happy to leave the crash for now, you are officially "not making it worse" with this PR 😁, and we'll probably want to revisit the logic in multiple places to fix this properly.
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.
This is now covered by issue #156, and thus can be fixed by a separate PR that properly deals with the fact that we do not account for models that have different input and output sizes at all.
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.
May I suggest:
bs_calc_cropping
or similar.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 am lazy, but you are right.
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, not pushed