Skip to content
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

Fix two issues with KWAs #77

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix two issues with KWAs #77

wants to merge 2 commits into from

Conversation

mwoehlke-kitware
Copy link
Member

Modify KWA metadata reader to not crash if the corner points are not present. Modify various bits of the KWA clip-finding logic to add some fuzzing to account for floating-point precision issues that could cause lookup and padding resolution to incorrectly fail, particularly for single-frame requests at the start or end of the KWA.

Modify KWA metadata reader to not crash if the corner points are not
present.
Modify various bits of the KWA clip-finding logic to add some fuzz
factors. This should make the code tolerant of a small amount of
imprecision (which can happen, given that we are using floating point
numbers) when looking up matching clips and resolving padding. This
should fix these operations failing to produce correct results in some
cases. In particular, this could occur if a request is for a single
frame at either the start or end of the KWA.
Copy link
Contributor

@RustyBlue RustyBlue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See one comment - it is a question / musing, and I will approve otherwise, if you don't think any change is necessary.

{
qWarning() << __func__ << "called with malformed times (end < start)";
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling some with case where endTime and startTime are "equal", but actually endTime is slightly less than startTime. Struggling in the sense that I'm wondering if something might go sideways elsewhere - but not obvious to me tht it would. I wonder about making them equal if that is the case - but to what value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect that probably this wouldn't happen, but: Robustness Principle.

What I think is actually happening that necessitated this PR is that time values are running through conversions somewhere that cause what we get from the the KWA and what we get from a query / saved results to be slightly different. This seems to be particularly an issue for single-frame results because any "fuzz" will cause us to have no overlap between the result and the KWA (whereas a multi-frame result might lose a frame but still be non-empty). I expect what we would normally see is that the start and end are bitwise identical (if off by maybe a few ULPs from the "correct" values), but the general approach has been to treat values within a few ULPs as equal.

I don't think there is much value making start and end equal here. If they're close but not bitwise equal, one or both of them is almost surely wrong, and so a) picking one would be arbitrary, and b) anywhere else will need to do fuzzy comparisons anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair, I'm not 100% certain there aren't still places that should do fuzzing but don't; however, this seemed to fix the issues that we're seeing in VIAME.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants