stbt.match: Optimization: Merge regions-of-interest that overlap completely #569
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
drothlis
force-pushed
the
merge-nested-rois
branch
from
cf3b680
to
200eef7
Compare
…lass Note that I use `image[r.slice]` instead of `stbt.crop(image, r)` because the latter raises "ValueError: frame with dimensions (360, 640, 3) doesn't contain Region(x=0, y=0, right=640, bottom=361)" -- I suppose it's the pyramid upscaling that is introducing an extra 1 or 2 pixels to some of the ROIs.
…letely Currently we're getting rois like this:  Note that `cv2.findContours` with `cv2.RETR_EXTERNAL` is working correctly: those internal regions aren't internal contours, they're completely outside the large contour. It's only when we take the *bounding rectangle* of the large contour, that the smaller ones fall inside it. So we have to manually filter them out. I couldn't find a suitable OpenCV function to do it so I've implemented a simple quadratic loop. After `_merge_regions` it looks like this:  This is a simple quadratic implementation, but it shouldn't matter because we don't expect huge numbers of regions. On my laptop it takes: | number of regions | ms | |-------------------|-----| | 20 | 0.2 | | 200 | 4 | | 2000 | 27 |
So that we only need 1 comparison for each pair of regions (the smaller region can't possibly contain the larger region). Also modify the list in-place. Performance on my laptop: | number of regions | ms | previous implementation ms | |-------------------|------|----------------------------| | 20 | 0.06 | 0.2 | | 200 | 0.6 | 4 | | 2000 | 9 | 27 |
drothlis
force-pushed
the
merge-nested-rois
branch
from
200eef7
to
779581c
Compare
wmanley
reviewed
Feb 14, 2019
| (min(s.w - 1, r.x + r.w + t.w - 1), | ||
| min(s.h - 1, r.y + r.h + t.h - 1)), | ||
| (min(s.w - 1, r.right + t.w - 1), | ||
| min(s.h - 1, r.bottom + t.h - 1)), |
There was a problem hiding this comment.
Right, this is the same as:
d = roi.extend(right=template.shape[1], bottom=template.shape[0])
d = Region.intersect(d, _image_region(source_with_rois))
cv2.rectangle(source_with_rois, (d.x, d.y), (d.right - 1, d.bottom - 1),
(0, 255, 255), thickness=1)Maybe we should have our own draw_rectangle function that takes a Region encompassing the last two lines.
|
I ran it on one of my test-packs. There are some regressions but overall it seems a worthwhile improvement (average 12ms improvement across all 1100 template×screenshot pairs):
The worst slowdown (see below) had 350 regions!
|
|
This was the best improvement: 18 largeish regions that are redundant, because we already need to search the entire frame anyway.
|
drothlis
force-pushed
the
merge-nested-rois
branch
from
82bcd45
to
8442eab
Compare
To test the performance impact of various `stbt.match` optimisations. I have added screenshots of one pathological case for the "merge regions-of-interest that overlap completely" optimisation, and one case that justifies it. (You can test your own images by adding them under `tests/images/performance/` without committing them to git.)
For more consistent results. Only tested on my laptop.
For more consistent results across different implementations of `_merge_regions`.
drothlis
force-pushed
the
merge-nested-rois
branch
from
8442eab
to
121e53d
Compare
`slice` doesn't feel like a property to me like `width` or `height`.
wmanley
reviewed
Feb 15, 2019
_stbt/types.py
Outdated
| @@ -129,6 +129,11 @@ def height(self): | |||
| """The height of the region, measured in pixels.""" | |||
| return self.bottom - self.y | |||
|
|
|||
| @property | |||
| def slice(self): | |||
There was a problem hiding this comment.
I'm not sure this makes sense as a property as much as a function. Perhaps as_slice() or to_slice()?
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently we're getting rois like this:
Note that
cv2.findContourswithcv2.RETR_EXTERNALis working correctly: those internal regions aren't internal contours, they're completely outside the large contour. It's only when we take the bounding rectangle of the large contour, that the smaller ones fall inside it. So we have to manually filter them out. I couldn't find a suitable OpenCV function to do it so I've implemented a simple quadratic loop.After
_merge_regionsit looks like this:This is a quadratic implementation, but it shouldn't matter because we don't expect huge numbers of regions. On my laptop it takes:
TODO:
stbt.match.