Fix clock sync corruption caused by garbage data when outlet closes before inlet

[mscheltienne]

When an LSL outlet is destroyed while an inlet is still connected, liblsl may record an extra sample containing garbage (maybe garbage memory?). This extra sample can be accompanied by a corrupted clock offset measurement with nonsensical values (e.g., time jumping by ~800,000 seconds, offset value of ~-750,000 seconds).

Since pyxdf seems to use linear regression on clock offsets for synchronization, a single corrupted outlier can drastically skew the regression and produce incorrect timestamps for the entire stream.

This PR adds automatic detection and correction:

• Extra samples are truncated when the actual count exceeds the footer's sample_count
• Corrupted clock offsets are detected usin statistics (MAD-based outlier detection) and removed

See: labstreaminglayer/pylsl#67

@JesseLivezey I think this fix is more robust than your proposal in #149 and also doesn't require the addition of a new argument to the API. Can you give it a shot?

I'll try to dig through liblsl to see if I can quickly get rid of this extra sample at its source, but in the meantime, @cboulay @cbrnr can you take a look at this PR detecting the problem on the reading side?

[JesseLivezey]

@mscheltienne, this PR also fixes the clock sync and sample count issues in the xdfs I'm looking at. I think truncating the time_stamps and time_series is the right thing to do and should have no downsides.

IMO, it would be good to understand the liblsl cause better to understand what the right thing to do with the clock offset data. Might need some help from people more familiar with liblsl on this part.

In the case where the last clock offset may or may not be corrupted by the bug, but it is sometimes a correct offset, the logic in the PR makes sense.

If it is the case that the liblsl bug always causes an additional clock offset to be recorded and this offset can sometimes be corrupted, it would make more sense to truncate the entry.

[mscheltienne]

@JesseLivezey I updated the wording. The rationale is that:

1. We check if there is an extra sample. If there is not, then we don't even check the potentially corrupted clock offset which is likely correct (any large jump will be a sign of disconnect/reconnect or other legitimate clock desynchronization, which are both correctly handled by _clock_sync).
2. If there is an extra sample (compared to the authoritative footer), then we check with a statistical test the last clock offset sample. If it shows any sign of an anomaly, it gets discarded (thus the or, I prefer to be conservative in the sense that removing one sample from the _clock_sync correction is safer than to potentially include a bad sample- but I agree the wording of the comment was confusing).

Regardless of the underlying liblsl bug, the _clock_sync function relies on the clock offsets to compute the correction, so sanitizing those seems like a good idea.

[mscheltienne]

If it is the case that the liblsl bug always causes an additional clock offset to be recorded and this offset can sometimes be corrupted, it would make more sense to truncate the entry.

To add to the previous reply, I think the additional sample does not always yield an additional clock offset measurement; but I think that when it does, it's always wrong. This is why the step (2) to identify a statistically abnormal clock offset is only performed if there is an additional sample.

[JesseLivezey]

Right, I agree that the clock offset check should only happen if there is an extra sample.

I think the additional sample does not always yield an additional clock offset measurement; but I think that when it does, it's always wrong.

OK, if that's the case, then I think the logic in this PR makes sense.

FWIW, I looked at more xdf files that do contain extra samples in the streams. AFAICT, in 23/24 of the files, all final offsets for all streams look sensible in that they

• have values similar to earlier clock offsets in the stream
• are not duplicates of the previous clock offset

In 1/24 of the files, only some of the streams with extra samples have corrupted clock offsets for the final sample.

[cbrnr]

Beautiful! I only have a minor comment before merging.

[mscheltienne]

@cbrnr Once merged, could you cut a release including this fix? I'd like to pin immediately our dependencies to this new version. Edit: if the bugs of 1.17.1 were fixed.

[cbrnr]

@mscheltienne please add a changelog entry. Regarding a new release, sure, I can do that, and I think the bug in 1.17.1 has already been fixed by #140. We should add more details in the CHANGELOG regarding the problematic PR and which PRs get carried over to 1.17.2.

Can you confirm that the bug in 1.17.1 is fixed on main?

[mscheltienne]

Done- for the bug on 1.17.1, I just read the release page and I did not encounter it. I was not aware of #137 and #140 which indeed seems to solve the described bug.

[cbrnr]

Thanks @mscheltienne! I'll try to consolidate the changelog and cut v1.17.2 today!

[cbrnr]

@mscheltienne v1.17.2 is live!

[mscheltienne]

Thank you!