-
Notifications
You must be signed in to change notification settings - Fork 38
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
Enable selection of sensor format to avoid a cropped FoV #100
base: main
Are you sure you want to change the base?
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.
Thanks for the PR.
Can you add a before/after comparison to this PR? This will make it more clear what the new parameter do.
The new parameters and their behaviour and interaction with the other sizes and stream roles needs documentation, if this should make it into the upstream code. I think having two sets of sizes is confusing as it is not clear from the parameter names what the effect is.
src/CameraNode.cpp
Outdated
declare_parameter<int64_t>("mode_width", {}, param_descr_ro); | ||
declare_parameter<int64_t>("mode_height", {}, param_descr_ro); |
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.
What is the meaning of mode
in the parameter prefix? I think these new parameters also need to be documented, as it is not clear what they do without looking at the code.
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.
The term mode seems to be the term for this -- and is a short for sensor mode;
- https://www.raspberrypi.com/documentation/computers/camera_software.html#common-options
- How to change the default sensor mode that gets selected by the video configuration? raspberrypi/picamera2#756
We could opt for a style similar to the rpicam tools; where mode is width:height:bit:pack
, and maybe calling it raw_mode or sensor_mode would make abit more sense.
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.
According to the first source, "sensor mode" refers to what the node uses as format, e.g. SRGGB10_CSI2P
:
Sensor mode identifiers have the following form: S<Bayer order><Bit-depth>_<Optional packing> : <Resolution list>
How are the rpicam-apps terming these parameters?
We could opt for a style similar to the rpicam tools; where mode is
width:height:bit:pack
, and maybe calling it raw_mode or sensor_mode would make abit more sense.
This would also work. In contrast to the other stream roles, the raw roles are always discrete, i.e. the stream configuration for the raw stream will always be adjusted to one of the raw formats.
src/CameraNode.cpp
Outdated
roles.push_back(get_role(get_parameter("role").as_string())); | ||
const libcamera::Size mode_size(get_parameter("mode_width").as_int(), get_parameter("mode_height").as_int()); | ||
|
||
if (!mode_size.isNull()) { | ||
roles.push_back(libcamera::StreamRole::Raw); | ||
} |
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.
What happens if a user already selected raw
for the role
? Are we then adding the same role twice with potentially different conflicting configurations?
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 agree, a check should be made, that if role is raw then this is not having an effect (and then ignore it).
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.
Alternatively, we could remove raw
from the options for role
. The device specific raw formats aren't supported by the ROS message anyway, and I am not sure if there is a benefit of publishing in Bayer format and then doing the debayering somewhere else.
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.
For now I have edited the code to not use my changes if the first role is a Raw.
Have you had a look at the |
When you update the PR, can you rebase and squash your commits? I merged a change that avoids referring to the read-only parameters multiple times. Have a look at this and try following the same with the new read-only parameters that you want to add. |
Adjustments to fit current codebase
Setting the libcamera::SensorConfiguration sensor;
sensor.outputSize = libcamera::Size(4608, 2592);
sensor.bitDepth = 10;
if (sensor.isValid())
cfg->sensorConfig = sensor;
else
throw std::runtime_error("invalid sensor configuration"); before the
without the need to manage an additional stream. But the size and bit depth have to match exactly for the validation to pass. |
Work to resolve #99
It enables the raw role, and adjusts the size of it to match the desired input, this avoids cropping where libcamera by default selects a sensor mode that is not having the full FoV. This is a problem with RPi Camera V2 (see https://picamera.readthedocs.io/en/release-1.13/fov.html#sensor-modes).
Example:
Output: