-
Notifications
You must be signed in to change notification settings - Fork 39
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
integrated camera_factory in video_input #232
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed
@@ -6,6 +6,8 @@ | |||
|
|||
|
|||
CAMERA = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename this to camera option? Also, isn't there an enum for what each number means? Please use the enum
@@ -12,7 +12,8 @@ | |||
def video_input_worker( | |||
camera_name: "int | str", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the CameraOption enum class. Then, you will need to change the config.yaml to include this option. You will also need to change the config loading portion of the code (similar to DETECT_TARGET_OPTION_INT).
modules/video_input/video_input.py
Outdated
|
||
|
||
class VideoInput: | ||
""" | ||
Combines image and timestamp together. | ||
""" | ||
|
||
def __init__(self, camera_name: "int | str", save_name: str = "") -> None: | ||
self.device = camera_device.CameraDevice(camera_name, 1, save_name) | ||
def __init__(self, camera_option: int, width: int, height: int) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make camera_option the enum
c52de01
to
4533f3a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed.
camera_name: "int | str", | ||
period: float, | ||
save_name: str, | ||
period: int, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move period with the worker parameters. Also should be float.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It currently is a worker parameter? I changed it to float
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant immediately above output_queue
. Then the parameters for the class object and the worker are grouped separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed.
27393c3
to
8ddc188
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed.
modules/video_input/video_input.py
Outdated
width is the width of the images the camera takes in pixels. | ||
height is the height of the images the camera takes in pixels. | ||
camera_config specifies camera settings. | ||
maybe_image_name is name of iamge log files. Set to None to not log any images. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this to align with logger.
Spelling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed.
WIDTH = 1920 | ||
HEIGHT = 1200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integration test is failing for me, I suggest a resolution of 640x480 .
VIDEO_INPUT_WORKER_PERIOD = 1.0 | ||
CAMERA = 0 | ||
CAMERA = camera_factory.CameraOption.OPENCV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also failing here, but for some reason there is a conversion which results in this being float
in the worker function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, thanks!
No description provided.