Skip to content

HDR merge - discarding merged frame on resume - #9498

Merged
ev-mp merged 3 commits into
realsenseai:developmentfrom
remibettan:hdr-merge-clean-after-hdr-stop
Aug 3, 2021
Merged

ev-mp merged 3 commits into
realsenseai:developmentfrom
remibettan:hdr-merge-clean-after-hdr-stop

Conversation

@remibettan

Copy link
Copy Markdown
Contributor

Tracked by: DSO-17341

@remibettan
remibettan requested a review from ev-mp July 26, 2021 16:21
@remibettan
remibettan force-pushed the hdr-merge-clean-after-hdr-stop branch from 628a9ad to a740aac Compare July 27, 2021 06:14
Comment thread src/proc/hdr-merge.cpp Resolved
Comment thread unit-tests/live/d400/test-hdr.cpp Outdated

bool frame_metadata_not_enabled = false;

if (!frame_metadata_not_enabled)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The variable is used with double negation - maybe it should be renamed to metadata_is_present for clarity?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread unit-tests/live/d400/test-hdr.cpp Outdated
REQUIRE(depth_sensor.get_option(RS2_OPTION_HDR_ENABLED) == 0.f);

if (!frame_metadata_not_enabled)
if (!stream_frames(pipe, 10, merging_filter))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The frame counter, 10, must be bigger than the internal threshold - I would recommend to make it 30-50 frames.
Also, there is no need to double-check for metadata as it is determined in the first iteration.
Can be skipped here and below

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if metadata is not enabled, the commands within the if statement need to be skipped. So, this flag must remain.

Comment thread unit-tests/live/d400/test-hdr.cpp Outdated
}
}

bool stream_frames(rs2::pipeline& pipe, int num_of_frames, rs2::hdr_merge& merging_filter)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add function description

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@remibettan
remibettan force-pushed the hdr-merge-clean-after-hdr-stop branch from a740aac to 9936be2 Compare August 2, 2021 12:09
@remibettan
remibettan force-pushed the hdr-merge-clean-after-hdr-stop branch from 9936be2 to 1319324 Compare August 3, 2021 08:59

@ev-mp ev-mp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch!

@ev-mp ev-mp changed the title hdr merge - discarding merged frame if counter diff is higher than 10 HDR merge - discarding merged frame on resume Aug 3, 2021
@ev-mp
ev-mp merged commit 894a7db into realsenseai:development Aug 3, 2021
@remibettan
remibettan deleted the hdr-merge-clean-after-hdr-stop branch July 29, 2025 14:09
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