Skip to content

Fix two issues with KWAs #77

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.

Sign up for GitHub

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

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

Fix two issues with KWAs #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bab36b1
Don't crash if KWA corner points are missing
mwoehlke-kitware Mar 16, 2021
7a9abf6
Tolerate some imprecision for KWA clips
mwoehlke-kitware Mar 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Libraries/VgVideo/vgKwaArchive.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,9 @@ vgKwaVideoClip* vgKwaArchivePrivate::findClip(
}
}

const double clipStart = clip->firstTime().Time;
const double clipEnd = clip->lastTime().Time;
constexpr auto epsilon = vgKwaVideoClip::timeEpsilon;
const double clipStart = clip->firstTime().Time * (1.0 - epsilon);
const double clipEnd = clip->lastTime().Time * (1.0 + epsilon);

// Check for overlap
if (clipStart <= requestEnd || clipEnd >= requestStart)
Expand Down
37 changes: 22 additions & 15 deletions Libraries/VgVideo/vgKwaFrameMetadata.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,22 +77,29 @@ vgKwaFrameMetadata::vgKwaFrameMetadata(

d->homography = vgMatrix3d{homography.data_block()};

double max = qMax(qMax(qMax(corners[0][0], corners[0][1]),
qMax(corners[1][0], corners[1][1])),
qMax(qMax(corners[2][0], corners[2][1]),
qMax(corners[3][0], corners[3][1])));

if (max < 400)
if (!corners.empty())
{
d->worldCornerPoints.GCS = 4326;
d->worldCornerPoints.UpperLeft.Latitude = corners[0][0];
d->worldCornerPoints.UpperLeft.Longitude = corners[0][1];
d->worldCornerPoints.UpperRight.Latitude = corners[1][0];
d->worldCornerPoints.UpperRight.Longitude = corners[1][1];
d->worldCornerPoints.LowerLeft.Latitude = corners[2][0];
d->worldCornerPoints.LowerLeft.Longitude = corners[2][1];
d->worldCornerPoints.LowerRight.Latitude = corners[3][0];
d->worldCornerPoints.LowerRight.Longitude = corners[3][1];
double max = qMax(qMax(qMax(corners[0][0], corners[0][1]),
qMax(corners[1][0], corners[1][1])),
qMax(qMax(corners[2][0], corners[2][1]),
qMax(corners[3][0], corners[3][1])));

if (max < 400)
{
d->worldCornerPoints.GCS = 4326;
d->worldCornerPoints.UpperLeft.Latitude = corners[0][0];
d->worldCornerPoints.UpperLeft.Longitude = corners[0][1];
d->worldCornerPoints.UpperRight.Latitude = corners[1][0];
d->worldCornerPoints.UpperRight.Longitude = corners[1][1];
d->worldCornerPoints.LowerLeft.Latitude = corners[2][0];
d->worldCornerPoints.LowerLeft.Longitude = corners[2][1];
d->worldCornerPoints.LowerRight.Latitude = corners[3][0];
d->worldCornerPoints.LowerRight.Longitude = corners[3][1];
}
else
{
d->worldCornerPoints.GCS = -1;
}
}
else
{
Expand Down
24 changes: 15 additions & 9 deletions Libraries/VgVideo/vgKwaVideoClip.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,8 +490,9 @@ vgKwaVideoClip::vgKwaVideoClip(const vgKwaVideoClip& other,
foreach_iter (MetadataIterator, mdIter, od->metadata)
{
// Get raw time
const double time = mdIter.key().Time;
if (time >= startTime && time <= endTime)
const auto time = mdIter.key().Time;
const auto fuzz = time * timeEpsilon;
if ((time + fuzz) >= startTime && (time - fuzz) <= endTime)
{
// Copy metadata, if in range
d->metadata.insert(mdIter.key(), mdIter.value());
Expand All @@ -503,8 +504,9 @@ vgKwaVideoClip::vgKwaVideoClip(const vgKwaVideoClip& other,
foreach_iter (FrameIterator, fIter, od->frames)
{
// Get raw time
const double time = fIter.key().Time;
if (time >= startTime && time <= endTime)
const auto time = fIter.key().Time;
const auto fuzz = time * timeEpsilon;
if ((time + fuzz) >= startTime && (time - fuzz) <= endTime)
{
// Copy frame, if in range
d->frames.insert(fIter.key(), fIter.value());
Expand Down Expand Up @@ -588,19 +590,23 @@ bool vgKwaVideoClip::resolvePadding(
return false;
}

CHECK_ARG(startTime <= endTime, false);
if (endTime < startTime && !qFuzzyCompare(startTime, endTime))
{
qWarning() << __func__ << "called with malformed times (end < start)";
return false;
}
Copy link
Contributor

@RustyBlue RustyBlue Mar 18, 2021

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

@mwoehlke-kitware mwoehlke-kitware Mar 24, 2021

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

@mwoehlke-kitware mwoehlke-kitware Mar 24, 2021

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.


// Fix up arguments
if (startTime == -1 && endTime == -1)
{
startTime = std::numeric_limits<qint64>::min();
endTime = std::numeric_limits<qint64>::max();
startTime = -std::numeric_limits<double>::infinity();
endTime = +std::numeric_limits<double>::infinity();
}
padding = qMax(padding, 0.0);

// Check for some overlap
const double myStartTime = d->frames.begin().key().Time;
const double myEndTime = (d->frames.end() - 1).key().Time;
const double myStartTime = d->frames.firstKey().Time * (1.0 - timeEpsilon);
const double myEndTime = d->frames.lastKey().Time * (1.0 + timeEpsilon);
CHECK_ARG(myStartTime < endTime && myEndTime > startTime, false);

if (d->metadata.isEmpty() || padding == 0.0)
Expand Down
3 changes: 3 additions & 0 deletions Libraries/VgVideo/vgKwaVideoClip.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ class VG_VIDEO_EXPORT vgKwaVideoClip : public vgVideo
public:
typedef vgTimeMap<vgKwaFrameMetadata> MetadataMap;

/// Factor used to "fuzz" times to account for floating point errors.
static constexpr auto timeEpsilon = 1e-15;

explicit vgKwaVideoClip(const QUrl& indexUri);
virtual ~vgKwaVideoClip();

Expand Down