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.

Sign up for GitHub

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

Jump to bottom

Remove redundant frame copies inside librealsense core #10573

Merged
merged 1 commit into from
Jun 9, 2022
Merged

Remove redundant frame copies inside librealsense core #10573

merged 1 commit into from
Jun 9, 2022

Conversation

Nir-Az
Copy link
Collaborator

@Nir-Az Nir-Az commented Jun 8, 2022

Tracked on [LRS-437]

@Nir-Az Nir-Az requested review from maloel and ev-mp June 8, 2022 11:29
ev-mp
ev-mp approved these changes Jun 9, 2022
View reviewed changes
Copy link
Collaborator

@ev-mp ev-mp left a comment

Choose a reason for hiding this comment

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

Looks good, it is up to you whether to keep the previos code as a comment

@Nir-Az
Copy link
Collaborator Author

Nir-Az commented Jun 9, 2022

Looks good, it is up to you whether to keep the previos code as a comment

The reason I left it is that we set a frame_holder object without setting the frame vector inside,
I want it to be clear that I didn't forgot to do it, it's was intentionally left unassigned.

This function can be refactored in the future, we didn't want to deal with it currently.

maloel
maloel reviewed Jun 9, 2022
View reviewed changes
src/sensor.cpp
//REMOVED! - no need to add 2 copies to a frame
//byte* pix = (byte*)fo.pixels;
//std::vector<byte> pixels(pix, pix + fo.frame_size);
//fr->data = pixels;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is dangerous.
This leaves fr->data (for regular frames) basically uninitialized (empty), right? I.e., as long as it's not used, we're OK. So why not remove it?
I believe a composite frame still uses it... maybe we can move it there?
(not for this PR, but general thought)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, like we said the generate_frame_from_data() needs to be refactored

@maloel maloel merged commit b5bed9d into IntelRealSense:development Jun 9, 2022
@puzzlepaint puzzlepaint mentioned this pull request Aug 24, 2022
Closed
@Nir-Az Nir-Az deleted the remove_unnecessary_frame_copies branch September 30, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maloel maloel maloel approved these changes

@ev-mp ev-mp ev-mp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Nir-Az @ev-mp @maloel