{BugFix} Core - Restore Gen2 time sync and load TimeSync records on demand - #390
Closed
kongchen1992 wants to merge 1 commit into
Closed
{BugFix} Core - Restore Gen2 time sync and load TimeSync records on demand#390kongchen1992 wants to merge 1 commit into
kongchen1992 wants to merge 1 commit into
Conversation
…emand Summary: Explanation: Every Aria Gen 2 recording currently loses its cross-device time sync. On an affected file `supports_time_domain()` reports false for SubGhz, Utc, TimeCode and TicSync alike, `get_first_time_ns()` and `get_index_by_time_ns()` throw for those domains, and every `SensorData` comes back with an empty sync-timestamp map. Reproduced on the published Gen 2 sample pair: the SubGHz receiver recording fails with `Time domain SubGhz not supported for the stream RGB Camera Class facebookresearch#1`. Two defects combined: 1. `determineTimeSyncModeFromJson` decided the Gen 2 mode from `metadata.recording.subghz_mode` alone. Recordings predating the OS change that introduced that field carry no such hint, so a genuine SubGHz receiver resolved to `NotEnabled`. The Gen 2 branch also never looked for UTC, so a recording with SubGHz simply switched off -- the common case -- resolved to `NotEnabled` too, despite carrying a perfectly usable UTC stream. 2. `TimeSyncMapper` treated `NotEnabled` as "these streams are unusable" and skipped its preload, returning before `timesyncPlayers_` was even assigned. That left `supportsMode()` false for every mode at once, so a single SubGHz-only signal silently disabled all four time domains. The Gen 2 branch now falls back to the streams the file actually contains, the way Gen 1 already did, and keeps the metadata field for the one case the streams cannot express: a broadcaster is the clock reference, so it logs no mapping stream of its own. Removing the skip alone would restore correctness at an unacceptable price: opening the multi-hour Manifold recording from T272680899 goes from 27s to 424s, because `TimeSyncMapper` walked every record of every TimeSync stream up front. That is the worst possible access order for a remote file. TimeSync records are interleaved with sensor data across the whole recording, so reading them cold pulls -- and then evicts -- essentially every cache block before any sensor read can reuse one. `TimeSyncMapper` now reads no record payloads at construction. It keeps each stream's record index, which is already in memory, and fetches samples the first time a conversion needs them, locating the bracketing pair through the index rather than by scanning. A sequential sweep then picks up each TimeSync record from the block it is already reading, and a cursor keeps the common case at O(1) instead of a binary search. Also in this change, all consequences of the above: - `MetadataTimeSyncMode::SubGhz` and `::Utc` were never registered with pybind, so Python printed `MetadataTimeSyncMode.???` and `== MetadataTimeSyncMode.SubGhz` raised `AttributeError`. Both values, and the two missing `timeSyncModeStr()` cases, are added. - `convertFrom*` took `auto` rather than `const auto&` and so copied the entire sample vector on every call. Harmless while the vector was always empty, ruinous the moment it is not. - `convertFromSyncTimeToDeviceTimeNs` called `front()` without the empty-vector guard its counterpart had. - `recordInfoTimeNs_` was written and never read; it becomes the index timeline the on-demand search runs on. Reproducibility: Open any Gen 2 recording and call `supports_time_domain(stream_id, TimeDomain.UTC)`: false before this change, true after, on files that carry a UTC stream. The log line `Skipping TimeSyncMapper preload: file metadata reports time-sync NotEnabled (N TimeSync stream(s) present but unused)` marks each affected open. Reviewed By: ryanfrawley Differential Revision: D115652745
Contributor
|
@kongchen1992 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D115652745. |
Contributor
|
This pull request has been merged in c6267ce. |
This file contains hidden or 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
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.
Summary:
Explanation:
Every Aria Gen 2 recording currently loses its cross-device time sync. On an
affected file
supports_time_domain()reports false for SubGhz, Utc, TimeCodeand TicSync alike,
get_first_time_ns()andget_index_by_time_ns()throw forthose domains, and every
SensorDatacomes back with an empty sync-timestampmap. Reproduced on the published Gen 2 sample pair: the SubGHz receiver
recording fails with
Time domain SubGhz not supported for the stream RGB Camera Class #1.Two defects combined:
determineTimeSyncModeFromJsondecided the Gen 2 mode frommetadata.recording.subghz_modealone. Recordings predating the OS changethat introduced that field carry no such hint, so a genuine SubGHz receiver
resolved to
NotEnabled. The Gen 2 branch also never looked for UTC, so arecording with SubGHz simply switched off -- the common case -- resolved to
NotEnabledtoo, despite carrying a perfectly usable UTC stream.TimeSyncMappertreatedNotEnabledas "these streams are unusable" andskipped its preload, returning before
timesyncPlayers_was even assigned.That left
supportsMode()false for every mode at once, so a singleSubGHz-only signal silently disabled all four time domains.
The Gen 2 branch now falls back to the streams the file actually contains, the
way Gen 1 already did, and keeps the metadata field for the one case the streams
cannot express: a broadcaster is the clock reference, so it logs no mapping
stream of its own.
Removing the skip alone would restore correctness at an unacceptable price:
opening the multi-hour Manifold recording from T272680899 goes from 27s to 424s,
because
TimeSyncMapperwalked every record of every TimeSync stream up front.That is the worst possible access order for a remote file. TimeSync records are
interleaved with sensor data across the whole recording, so reading them cold
pulls -- and then evicts -- essentially every cache block before any sensor read
can reuse one.
TimeSyncMappernow reads no record payloads at construction. It keeps eachstream's record index, which is already in memory, and fetches samples the first
time a conversion needs them, locating the bracketing pair through the index
rather than by scanning. A sequential sweep then picks up each TimeSync record
from the block it is already reading, and a cursor keeps the common case at O(1)
instead of a binary search.
Also in this change, all consequences of the above:
MetadataTimeSyncMode::SubGhzand::Utcwere never registered with pybind,so Python printed
MetadataTimeSyncMode.???and== MetadataTimeSyncMode.SubGhzraised
AttributeError. Both values, and the two missingtimeSyncModeStr()cases, are added.
convertFrom*tookautorather thanconst auto&and so copied the entiresample vector on every call. Harmless while the vector was always empty,
ruinous the moment it is not.
convertFromSyncTimeToDeviceTimeNscalledfront()without the empty-vectorguard its counterpart had.
recordInfoTimeNs_was written and never read; it becomes the index timelinethe on-demand search runs on.
Reproducibility:
Open any Gen 2 recording and call
supports_time_domain(stream_id, TimeDomain.UTC): false before this change, true after, on files that carry aUTC stream. The log line
Skipping TimeSyncMapper preload: file metadata reports time-sync NotEnabled (N TimeSync stream(s) present but unused)marks eachaffected open.
Reviewed By: ryanfrawley
Differential Revision: D115652745