[jantonguirao]

Category:

Bug fix (non-breaking change which fixes an issue)

Description:

fn.experimental.readers.video (CPU and GPU) keeps a process-wide cache of frame indices, keyed by file name, so that a file is indexed once per process. That cache had a data race:

• FrameIndexCache::find took a shared lock, looked the file up and returned an iterator after releasing the lock. Callers then copied the FrameIndex through that iterator without holding any lock.
• At the same time, insert() (under the exclusive lock) could overwrite the very same entry, when two readers index the same file, or insert a new entry and rehash the map, which invalidates every outstanding iterator.

This happens whenever more than one video reader runs in the same process and they set up or switch files at the same time: several pipelines with a video reader over the same (or overlapping) file lists, pipelines built from different threads, or two readers in one pipeline. The result is undefined behavior, which can show up as a torn or corrupted copy of the index (wrong frame count, timestamps or keyframe flags, and so wrong frames decoded) or as a crash. Fixing it is also a prerequisite for decoding a batch with several decoders in one reader (#6563, #6564), where the decoders of a single reader get the indices of the files they open concurrently.

A second, smaller problem: a cached index was never invalidated. If a file was rewritten during the lifetime of the process, every later reader reused the index of the old contents.

This PR fixes both:

• The cache moves to its own files, frame_index_cache.{h,cc}. FrameIndexCache::instance() is defined in the .cc file, so that there is one cache per process.
• GetOrBuild(filename, stamp, build) returns the index of a file, building it with build() if it is not cached. Each index is built once: other callers asking for the same file wait for that build (a shared future) instead of building it again. The build runs outside the lock, and lookups take a shared_mutex in shared mode, so lookups and the indexing of different files run in parallel. If a build throws, the exception is rethrown to every caller waiting for it and the entry is removed, so that a later call can try again.
• The returned index is an immutable std::shared_ptr<const FrameIndex>, which stays valid whatever happens to the map afterwards.
• Indices are kept for the lifetime of the process, as on main, so that readers created later (e.g. a new pipeline over the same files) do not index the files again. An index is built again when the file's size or modification time (FileStamp) changed. The stamp is taken before the decoder opens the file, so that an index of a file rewritten in the meantime is not cached as that of the new version.
• Find(filename) returns a cached, ready index or nullptr; it never builds and never waits.
• Each reader keeps the indices of its files (VideoLoaderDecoder::indices_), obtained in PrepareMetadataImpl, and uses them when it opens a file during decoding (FileIndex), without cache lookups after setup. So an index cannot change under a reader, even if the file is rewritten and another reader rebuilds its index.

The debug statistics printed by the old cache's destructor are removed with it.

Split out of #6545. Independent of the other PRs of the split; based directly on main.

Additional information:

Affected modules and functionalities:

• dali/operators/video/reader/frame_index_cache.h, frame_index_cache.cc (new): FrameIndexCache with GetOrBuild, Find, FileStamp.
• dali/operators/video/reader/video_reader_decoder_op.cc:
  • the old in-file FrameIndexCache class is removed;
  • VideoLoaderDecoder::PrepareMetadataImpl gets each file's index with GetOrBuild and stores it in indices_;
  • VideoLoaderDecoder::FileIndex / VideoReaderDecoder::FileIndex give the reader the index it was set up with;
  • VideoReaderDecoder::Prefetch sets the decoder's index from FileIndex instead of looking up (or building) it in the cache.
• dali/operators/video/reader/frame_index_cache_test.cc (new).

Only fn.experimental.readers.video uses this cache; fn.experimental.decoders.video is not affected.

Key points relevant for the review:

• GetOrBuild: the double lookup (shared lock, then exclusive lock) and the id that tells an entry from a later one of the same file, so that Finish does not publish (or erase) an entry that was replaced, because the file changed, while it was being built.
• The FileStamp is taken in PrepareMetadataImpl before the decoder opens the file. A file that cannot be stat-ed gets the stamp {-1, -1}, so its index is still cached and reused.
• In Prefetch, a file the reader opens must have an index from setup; a missing one is reported as an internal error (DALI_ENFORCE).

Tests:

• Existing tests apply
  • decoder/test_video.py (fn.experimental.readers.video tests), checkpointing/test_dali_checkpointing.py: test_experimental_video_reader*
• New tests added
  • Python tests
  • GTests
    • frame_index_cache_test.cc:
      • FrameIndexCacheTest.BuildsEachIndexOnceUnderConcurrency: 16 threads call GetOrBuild over 64 keys; each index is built exactly once and every caller gets the right index.
      • FrameIndexCacheTest.IndexStaysValidAfterOtherInsertions: an index stays valid while 999 more entries force rehashing.
      • FrameIndexCacheTest.FailedBuildIsReportedAndRetried
      • FrameIndexCacheTest.FindDoesNotBuildOrWait
      • FrameIndexCacheTest.IndexIsKeptAfterItsLastUser
      • FrameIndexCacheTest.IndexIsRebuiltWhenTheFileChanges
  • Benchmark
  • Other
• N/A

Checklist

Documentation

• Existing documentation applies
• Documentation updated
  • Docstring
  • Doxygen
  • RST
  • Jupyter
  • Other
• N/A

DALI team only

Requirements

• Implements new requirements
• Affects existing requirements
• N/A

REQ IDs: N/A

JIRA TASK: DALI-4935

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[Copilot]

🟡 Changes recommended

Repeating the new tests in one process reuses cached entries, causing assertion failures and an indefinite wait.

1 open issue

• Use fresh cache key namespaces for repeated test invocations · New

What changed in this PR

This PR makes the experimental video reader’s process-wide frame index cache thread-safe and adds file-change invalidation, supporting concurrent readers and future parallel decoding.

Changes:

• Introduces a shared cache with immutable indices, coordinated builds, and retry after failures.
• Keeps setup-time indices in each reader for reuse during decoding.
• Adds tests for concurrency, index lifetime, failure handling, and invalidation.

File	Description
dali/​operators/​video/​reader/​video_reader_decoder_op.cc	Integrates the new cache and retains each reader’s indices.
dali/​operators/​video/​reader/​frame_index_cache.h	Defines cache interfaces, ownership, and coordinated builds.
dali/​operators/​video/​reader/​frame_index_cache.cc	Implements file stamps, synchronized lookups, and publication.
dali/​operators/​video/​reader/​frame_index_cache_test.cc	Tests concurrency, retention, retries, and file-change invalidation.

_{Review effort: Balanced}

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; the previous test gap is addressed and no new actionable issues were found.

Summary

The PR replaces unlocked cache iterators with shared, immutable frame indices. Readers keep the indices obtained during setup.

• Video readers keep a stable frame index for each file.

Diagram

sequenceDiagram
  participant Reader
  participant Cache
  participant Builder
  Reader->>Cache: GetOrBuild(filename, stamp)
  alt Matching index is ready
    Cache-->>Reader: Shared immutable index
  else Matching build is pending
    Cache-->>Reader: Wait for shared result
  else No matching entry
    Cache->>Builder: Build outside the lock
    Builder-->>Cache: Index or exception
    Cache->>Cache: Update only if entry id still matches
    Cache-->>Reader: Result or exception
  end

Loading

_{Reviews (2) · Last reviewed commit: "Make frame index cache tests repeatable ..." · Reviewed by Greptile}