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

Unit test for end recording and start streaming again bug fix #12541

Merged
merged 4 commits into from
Jan 1, 2024

Conversation

noacoohen
Copy link
Contributor

Tracked by RSDEV-1226

@noacoohen noacoohen requested a review from Nir-Az December 26, 2023 12:33
dev = test.find_first_device_or_exit()
depth_sensor = dev.first_depth_sensor()
find_default_profile()
record()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't you need to pass the file_name? maybe it's better to pass it?

temp_dir = tempfile.mkdtemp()
file_name = os.path.join(temp_dir, "recording.bag")

sync = rs.syncer()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need the syncer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we use it to wait for frames and check that the streaming after recording worked well

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like there is nothing to sync since we have only 1 profile.
Maybe we can use a frame queue instead?
Please check the it in the API..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

sync = rs.syncer()
dev = test.find_first_device_or_exit()
depth_sensor = dev.first_depth_sensor()
find_default_profile()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would expect it to return a profile and to pass it to the streaming function? or even being in side the streaming function ?

record()

# after we finish recording we close the sensor and then open it again and try streaming
try_streaming()
Copy link
Collaborator

Choose a reason for hiding this comment

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

stream_and_record?

@Nir-Az
Copy link
Collaborator

Nir-Az commented Dec 26, 2023

Please check this:

14:53:53  -D-         running: ['C:\\Python310_64\\python.exe', '-u', 'C:\\jenkins_sys_rsbuild\\workspace\\LRS_windows_compile_pipeline\\unit-tests\\live/rec-play/test-record-and-stream.py', '--debug', '--context', 'github windows']
14:53:54  -D-         test took 9.910705804824829 seconds


ctx = rs.context()
playback = ctx.load_device(file_name)
depth_sensor = playback.first_depth_sensor()

depth_profile = restart_profile(default_profile)
depth_sensor.open(depth_profile)
depth_sensor.start(sync)
depth_sensor.start(frame_queue)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we reuse try_streaming again maybe here?
and I am not sure but you might want to clear the frame queue between both usage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

temp_dir = tempfile.mkdtemp()
file_name = os.path.join(temp_dir, "recording.bag")

sync = rs.syncer()
frame_queue = rs.frame_queue(1000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why 1000? :)
Do it has a default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is 1, I can change the capacity to be smaller.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's decrease it to allocate less memory..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Nir-Az Nir-Az merged commit b86977e into IntelRealSense:development Jan 1, 2024
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.

2 participants