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

Actual FPS simplification & unit change [API change] #12326

Merged
merged 5 commits into from Oct 31, 2023

Conversation

maloel
Copy link
Collaborator

@maloel maloel commented Oct 27, 2023

RS2_FRAME_METADATA_ACTUAL_FPS is now really the actual FPS, same as shown in the viewer as Hardware FPS, although with better precision:

  • Changes the units of ACTUAL_FPS metadata to be *1000 (e.g., 30.1 fps will be represented as 30100 in metadata)
    • This is NOT backwards-compatible!
    • This greatly improves syncer accuracy at lower FPS (see Improve desyncs on different-FPS streams #12315)
    • NOTE: this likely has implications for rosbag recordings, in that the old values will not be *1000... when tested, they play just fine but show the old values
  • Removes discrete mode for ACTUAL_FPS
    • This was used in non-color (i.e., Depth, IR, etc.) d400_device derivatives; actual FPS will now be correct
  • Removes use of ACTUAL_EXPOSURE as predictor of actual FPS
  • If ACTUAL_FPS cannot be calculated (missing previous frame time, or getting a value ~0) then ACTUAL_FPS value will be missing (i.e., supports() will return false; contrast with the previous behavior, which instead put the stream FPS there)
  • Simplifies metadata parsers: joins the supports() and get() functions into single find()
  • Likewise implements a single virtual frame_interface::find_metadata() to both check and get metadata - should save on performance, especially relevant in the syncer

Related to [RSDSO-19336]

@maloel maloel requested a review from Nir-Az October 27, 2023 07:41
@Nir-Az
Copy link
Collaborator

Nir-Az commented Oct 29, 2023

Can we run a "Semi" RealCI and see jo reggression.
We have to be carefull here.
Also need to make sure ROS is not using this field

@maloel
Copy link
Collaborator Author

maloel commented Oct 29, 2023

No ACTUAL_FPS in realsense-ros

@maloel
Copy link
Collaborator Author

maloel commented Oct 30, 2023

Two Semi runs of RealCI came back clean so far.

@Nir-Az Nir-Az changed the title Actual FPS simplification & unit change Actual FPS simplification & unit change [API change] Oct 30, 2023
src/frame.cpp Show resolved Hide resolved
Copy link
Collaborator

@Nir-Az Nir-Az left a comment

Choose a reason for hiding this comment

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

LGTM

@maloel
Copy link
Collaborator Author

maloel commented Oct 31, 2023

We see no regressions on external sync slave setups (which prompted this change):

  • 30-30 FPS 3-second desync every 170 sec (before this PR) is now gone (with this PR)
  • 60-60 behaves well
  • D60-C5 no regression; still no sync at all due to huge spread between D & C arrival times
  • D5-C60 sync happens as expected
    This may or may not be with a faulty external sync setup (currently being investigated).

@maloel
Copy link
Collaborator Author

maloel commented Oct 31, 2023

RealCI on the D457 ran twice; no new behavior observed.

@maloel maloel merged commit a6b48b9 into IntelRealSense:development Oct 31, 2023
16 of 17 checks passed
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.

None yet

2 participants