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

Experimental Video Reader Refactoring and API Improvements #5839

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jantonguirao
Copy link
Contributor

@jantonguirao jantonguirao commented Mar 5, 2025

Category:

Refactoring / New feature

Description:

  • Added file_root, file_list modes to fn.experimental.readers.video, similar to what's available in the legacy decoder
  • Added support for different file list formats:
    • frame_index: Frame-based seeking
    • timestamp: Time-based seeking (non-inclusive)
    • timestamp_inclusive: Time-based seeking (inclusive)
  • The file_list_format option replaces the legacy operator's boolean flags file_list_frame_num and file_list_include_preceding_frame
  • Added comprehensive testing comparing experimental reader with legacy implementation

Misc:

  • Unified video decoder interface between CPU and GPU implementations
  • Simplified constructor signatures for FramesDecoderGpu and FramesDecoderCpu
  • Added explicit override specifiers for virtual functions
  • Moved common video utilities to a new video_reader_utils.h header
  • Removed duplicate code by consolidating common functionality

Additional information:

Affected modules and functionalities:

  • fn.readers.video, fn.experimental.readers.video

Key points relevant for the review:

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • 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: N/A

@jantonguirao jantonguirao force-pushed the simplify_video_loader branch from 05541b1 to a07b96e Compare March 5, 2025 10:10
Signed-off-by: Joaquin Anton Guirao <[email protected]>
@jantonguirao jantonguirao force-pushed the simplify_video_loader branch from a07b96e to 79ef26d Compare March 5, 2025 10:13
file.frame_count_ = av_rescale_q(duration,
stream->time_base,
file.frame_base_);
file.frame_count_ = stream->nb_frames;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is more accurate so using it if available

@jantonguirao jantonguirao marked this pull request as ready for review March 5, 2025 17:33
@jantonguirao jantonguirao changed the title Simplify video loader Experimental Video Reader Refactoring and API Improvements Mar 5, 2025
@jantonguirao jantonguirao force-pushed the simplify_video_loader branch from f1ad340 to 05aadd5 Compare March 5, 2025 17:34
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [24949282]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [24949282]: BUILD FAILED

Signed-off-by: Joaquin Anton Guirao <[email protected]>
@jantonguirao jantonguirao force-pushed the simplify_video_loader branch from 05aadd5 to 380a2d7 Compare March 5, 2025 18:01
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [24951139]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [24951139]: BUILD FAILED

Signed-off-by: Joaquin Anton Guirao <[email protected]>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [25008498]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [25008498]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [25055285]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [25055285]: BUILD FAILED

Signed-off-by: Joaquin Anton Guirao <[email protected]>
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Signed-off-by: Joaquin Anton Guirao <[email protected]>
@jantonguirao jantonguirao force-pushed the simplify_video_loader branch from 7c63e66 to 081121a Compare March 10, 2025 08:34
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [25154741]: BUILD STARTED

Signed-off-by: Joaquin Anton Guirao <[email protected]>
Signed-off-by: Joaquin Anton Guirao <[email protected]>
@jantonguirao jantonguirao marked this pull request as draft March 10, 2025 09:33
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [25159683]: BUILD STARTED

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.

5 participants