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.
added cropping of capture/background #153
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
base: main
Are you sure you want to change the base?
added cropping of capture/background #153
Changes from 5 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.
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 updates my cropping branch accordingly. Your suggestion is within my 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.
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