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

Fix: Should not set HDR when it is already enabled #12511

Merged

Conversation

Arun-Prasad-V
Copy link
Contributor

Tracked on LRS-962

From FW version 5.14.x.x, the FW doesn't support updating the HDR presets when HDR is already enabled.

@@ -187,10 +187,6 @@ namespace librealsense

synthetic_sensor::open(requests);

// needed in order to restore the HDR sub-preset when streaming is turned off and on
if (_hdr_cfg && _hdr_cfg->is_enabled())
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 piece of code is invoked every time when the streams are opened. Since, FW will throw an error if we try to set it when it is already enabled, removing this code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure that is what we want, if the user explicitly turned on the hdr , if he closed the stream he expect it to stay on, even if we will block him from changing the exp/gain right?
This will even cause a confusion in the UI IMO, the checkbox will be marked but it will be off in FW.

according to this code Alex answer was wrong and streaming stop turn off HDR preset, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quoting Alex's response:
"SubPreset mechanism stays active in FW (even during start and stop streaming commands) till SETSUBPRESET 0 call."

So, my understanding from Alex's response is:

  • Stream CLOSE doesn't affect HDR.
  • So, when the stream is reopened, no need to enable HDR again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this was not the case when this was implemented originally.
Can we verify this behavior and verify he is right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I verified the following scenario from RS Viewer:

  1. HW Reset
  2. Set Exp.1 = 32000, Exp.2 = 1
  3. Enable HDR
  4. Open Depth stream
  5. From frame Metadata, verified that the Actual Exposure values to be 32000 & 1
  6. Close and Reopen Depth stream
  7. From frame Metadata, verified that the Actual Exposure values are still 32000 & 1

Do you want me to verify any other scenarios?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks , that was the required scenario to test

@Arun-Prasad-V Arun-Prasad-V force-pushed the dynamic_hdr_subpreset branch 2 times, most recently from f9886e3 to ad95819 Compare December 12, 2023 17:52
src/hdr-config.cpp Outdated Show resolved Hide resolved
@Arun-Prasad-V Arun-Prasad-V force-pushed the dynamic_hdr_subpreset branch 2 times, most recently from dc14073 to 30c3f90 Compare December 13, 2023 07:02
@Arun-Prasad-V Arun-Prasad-V reopened this Dec 13, 2023
@Arun-Prasad-V Arun-Prasad-V force-pushed the dynamic_hdr_subpreset branch 3 times, most recently from 437ac2a to 7876aa9 Compare December 13, 2023 13:54
@Arun-Prasad-V Arun-Prasad-V marked this pull request as ready for review December 13, 2023 13:54
@Arun-Prasad-V Arun-Prasad-V reopened this Dec 14, 2023
@Arun-Prasad-V Arun-Prasad-V reopened this Dec 14, 2023
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

@Nir-Az Nir-Az merged commit 861db20 into IntelRealSense:development Dec 15, 2023
32 of 33 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.

2 participants