<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 13, 2021 at 7:17 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Thu, May 06, 2021 at 04:54:47PM +0900, Hirokazu Honda wrote:<br>
> This enables retrieving supported test pattern modes through<br>
> CameraSensorInfo.<br>
<br>
I'm afraid this hits a shortcoming of the current CameraSensor class.<br>
<br>
In order:<br>
- The CameraSensor class accepts lists V4L2 controls in get/setControls<br>
- The CameraSensor class exposes libcamera::properties through<br>
properties()<br>
- The CameraSensor class exposes the -subdev- V4L2 controls through<br>
controls()<br>
- The CameraSensor class allows to get a snapshot of the current<br>
sensor configuration through CameraSensorInfo (mostly for IPAs, in<br>
facts the structure definition is in the process of being moved to<br>
the IPA headers).<br>
<br>
So we have quite some confusion there, and we need to design better<br>
the control interface for the class.<br>
<br>
In the meantime, items like test patters have not a real place where<br>
they belong:<br>
<br>
- We can't expose the raw V4L2 control indices and let the ph<br>
translate them to libcamera controls like we do with exposures, crop<br>
rectangles and such, as the translation requires accessing the<br>
sensor database, which is not (ideally) exposed to ph<br>
- We can't model them as properties, the supported test patters are static<br>
information but they can be enabled/disabled, something we don't<br>
allow for properties (at least conceptually).<br>
- CameraSensorInfo reports the current configuration parameters, it<br>
would be fair to report if test pattern is enabled or disabled, but<br>
the list of supported patterns does not really belong there.<br>
<br>
we've discussed the issue with the group, and for the time being it is<br>
probably easier to create and ad-hoc methods along the lines of<br>
CameraSensor::testPatternModes() to retrieve them. It's not ideal but<br>
it's probably the easiest decision to safely back-track later on.<br>
<br>
Sorry for conflicting directions, the CameraSensor class control<br>
interface will need to be re-thought with this new use-case in mind.<br>
<br></blockquote><div><br></div><div>I see. Can you review other patches in this series assuming CameraSensor::testPatternModes()?</div><div><br></div><div> -Hiro</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks<br>
j<br>
<br>
<br>
><br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
> include/libcamera/internal/camera_sensor.h | 5 ++++<br>
> src/libcamera/camera_sensor.cpp | 28 ++++++++++++++++++++++<br>
> 2 files changed, 33 insertions(+)<br>
><br>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h<br>
> index 3fa3a419..115e266d 100644<br>
> --- a/include/libcamera/internal/camera_sensor.h<br>
> +++ b/include/libcamera/internal/camera_sensor.h<br>
> @@ -38,6 +38,8 @@ struct CameraSensorInfo {<br>
><br>
> uint32_t minFrameLength;<br>
> uint32_t maxFrameLength;<br>
> +<br>
> + std::vector<int32_t> testPatternModes;<br>
> };<br>
><br>
> class CameraSensor : protected Loggable<br>
> @@ -79,6 +81,8 @@ private:<br>
> void initVimcDefaultProperties();<br>
> void initStaticProperties();<br>
> int initProperties();<br>
> + void initTestPatternModes(<br>
> + const std::map<int32_t, int32_t> &testPatternModeMap);<br>
><br>
> const MediaEntity *entity_;<br>
> std::unique_ptr<V4L2Subdevice> subdev_;<br>
> @@ -90,6 +94,7 @@ private:<br>
> V4L2Subdevice::Formats formats_;<br>
> std::vector<unsigned int> mbusCodes_;<br>
> std::vector<Size> sizes_;<br>
> + std::vector<int32_t> testPatternModes_;<br>
><br>
> Size pixelArraySize_;<br>
> Rectangle activeArea_;<br>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp<br>
> index 1db263cf..eedd2f89 100644<br>
> --- a/src/libcamera/camera_sensor.cpp<br>
> +++ b/src/libcamera/camera_sensor.cpp<br>
> @@ -408,6 +408,32 @@ void CameraSensor::initVimcDefaultProperties()<br>
> activeArea_ = Rectangle(pixelArraySize_);<br>
> }<br>
><br>
> +void CameraSensor::initTestPatternModes(<br>
> + const std::map<int32_t, int32_t> &testPatternModeMap)<br>
> +{<br>
> + const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);<br>
> + if (v4l2TestPattern == controls().end()) {<br>
> + LOG(CameraSensor, Debug) << "No static test pattern map for \'"<br>
> + << model() << "\'";<br>
> + return;<br>
> + }<br>
> +<br>
> + for (const ControlValue &value : v4l2TestPattern->second.values()) {<br>
> + const int32_t index = value.get<int32_t>();<br>
> +<br>
> + const auto it = testPatternModeMap.find(index);<br>
> + if (it != testPatternModeMap.end()) {<br>
> + LOG(CameraSensor, Debug) << "Test pattern mode (index="<br>
> + << index << ") ignored";<br>
> + continue;<br>
> + }<br>
> +<br>
> + LOG(CameraSensor, Debug) << "Test pattern mode (index="<br>
> + << index << ") added";<br>
> + testPatternModes_.push_back(it->second);<br>
> + }<br>
> +}<br>
> +<br>
> void CameraSensor::initStaticProperties()<br>
> {<br>
> const CameraSensorProperties *props = CameraSensorProperties::get(model_);<br>
> @@ -416,6 +442,8 @@ void CameraSensor::initStaticProperties()<br>
><br>
> /* Register the properties retrieved from the sensor database. */<br>
> properties_.set(properties::UnitCellSize, props->unitCellSize);<br>
> +<br>
> + initTestPatternModes(props->testPatternModeMap);<br>
> }<br>
><br>
> int CameraSensor::initProperties()<br>
> --<br>
> 2.31.1.607.g51e8a6a459-goog<br>
><br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>