[libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor: Enable to set a test pattern mode
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 28 07:26:27 CEST 2021
Hi Hiro,
On Mon, Jun 28, 2021 at 02:16:00PM +0900, Hirokazu Honda wrote:
> On Mon, Jun 28, 2021 at 12:28 PM Laurent Pinchart wrote:
> > On Mon, Jun 28, 2021 at 06:12:32AM +0300, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch.
> >
> > The patch is from Hiro of course :-)
> >
> > Another comment, do we need such a ad-hoc function, or could we handle
> > the test pattern mode using a more generic interface, with a ControlList
> > ? The CameraSensor::setControls() takes V4L2 controls today, which makes
> > it unsuitable for this purpose. I would ideally like to see all this
> > fixed, but I suppose it's a too big task for this series. It adds up to
> > the technical debt though, so there's a high chance that I'll push back
> > more strongly on the next patch that adds a ad-hoc function to
> > CameraSensor, until we reach a point where the push back will be a
> > strong nack and all this will need to be refactored by an unlucky
> > person. All this means that nobody should think that delaying the
> > handling of technical debt is a smart move to move it to someone else's
> > plate, it may come back around :-)
>
> If we want to use CameraSensor::setControls(), it is necessary for a
> pipeline handler to know the v4l2 index associated with a specific
> test pattern mode.
> How can we do that? Is it okay for a pipeline handler to read test
> pattern mode in camera properties?
I'd prefer going the other way around and avoid exposing V4L2 controls
from CameraSensor. I haven't really thought about how to do so yet
though.
> > > On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote:
> > > > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote:
> > > > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote:
> > > > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:
> > > > > > > Provides a function to set the camera sensor a test pattern mode.
> > > > > > >
> > > > > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > > > > ---
> > > > > > > include/libcamera/internal/camera_sensor.h | 1 +
> > > > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++
> > > > > > > 2 files changed, 40 insertions(+)
> > > > > > >
> > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > > > > index e133ebf4..8b9f84c9 100644
> > > > > > > --- a/include/libcamera/internal/camera_sensor.h
> > > > > > > +++ b/include/libcamera/internal/camera_sensor.h
> > > > > > > @@ -43,6 +43,7 @@ public:
> > > > > > > {
> > > > > > > return testPatternModes_;
> > > > > > > }
> > > > > > > + int setTestPatternMode(uint8_t testPatternMode);
> > > > > > >
> > > > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > > > > > > const Size &size) const;
> > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > > > > index 70bbd97a..ce8ff274 100644
> > > > > > > --- a/src/libcamera/camera_sensor.cpp
> > > > > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const
> > > > > > > * \return The list of test pattern modes
> > > > > > > */
> > > > > > >
> > > > > > > +/**
> > > > > > > + * \brief Set the camera sensor a specified controls::TestPatternMode
> > > > > >
> > > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not
> > > > > > documented but I don't get a warning :/
> > > > > >
> > > > > > Also, I'm a bit debated about where to actually place this function
> > > > > > (or better, it's initTestPatternModes which is misplaced)
> > > > > > if we have to respect the declaration order or the way you sorted them
> > > > > > here (which is more logical) is better.
> > > > > >
> > > > > > > + *
> > > > > > > + * \return 0 on success or a negative error code otherwise
> > > > > > > + */
> > > > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)
> > > > > > > +{
> > > > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > > > > > > + testPatternMode) == testPatternModes_.end()) {
> > > > > > > + LOG(CameraSensor, Error) << "Unsupported test pattern mode: "
> > > > > > > + << testPatternMode;
> > > > > > > + return -EINVAL;
> > > > > > > + }
> > > > > >
> > > > > > Do we need this ? The testPatternModes_ map is constructed using the
> > > > > > camera sensor properties, the below find() on the props->testPatternModes
> > > > > > map should gurantee that is supported. Also, if a test pattern mode is
> > > > > > not reported as part of testPatternModes_ applications shouldn't set
> > > > > > it...
> > > > > >
> > > > > > > +
> > > > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> > > > > > > + if (!props)
> > > > > > > + return -EINVAL;
> > >
> > > We already retrieve this in CameraSensor::initStaticProperties(), let's
> > > cache the pointer in the CameraSensor class.
> > >
> > > > > > > +
> > > > > > > + auto it = props->testPatternModes.find(testPatternMode);
> > > > > > > + ASSERT(it != props->testPatternModes.end());
> > > > > >
> > > > > > Yeah, I think the assertion here is more than enough..
> > > > > >
> > > > > > > + const uint8_t index = it->second;
> > > > > > > +
> > > > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });
> > > > > > > + if (ctrls.empty()) {
> > > > > > > + LOG(CameraSensor, Error)
> > > > > > > + << "Failed to retrieve test pattern control";
> > > > > > > + return -EINVAL;
> > > > > > > + }
> > > > > >
> > > > > > We won't register the TestPatternMode control in first place the V4L2
> > > > > > control is not supported..
> > > > >
> > > > > Replace this with ASSERT.
> > > > >
> > > > > > > +
> > > > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);
> > > > > > > + if (value.get<int32_t>() == index)
> > > > > > > + return 0;
> > > > > >
> > > > > > The V4L2 control framework should take care of this by itself, I think
> > > > > > we can set the control regardless
> > > > >
> > > > > Yes, I put this in order to save a redundant Ioctl call.
> > > > > Do you want to remove that?
> > > >
> > > > Well, the ctrls ControlList you create with getControls() goes through
> > > > an IOCTL :) We're talking details, but if we want to retain the
> > > > state of the testPatterMode we can make it a class member, so that the
> > > > code can be reduced to (not tested pseudo-code)
> > > >
> > > > {
> > > > if (testPatternMode == testPatternMode_)
> > > > return 0;
> > > >
> > > > const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> > > > if (!props)
> > > > return -EINVAL;
> > > >
> > > > auto it = props->testPatternModes.find(testPatternMode);
> > > > ASSERT(it != props->testPatternModes.end());
> > > >
> > > > ControlList ctrls(controls());
> > > > ctrls.set(V4L2_CID_TEST_PATTERN, it->second);
> > > >
> > > > int ret = setControls(&ctrls);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > testPatternMode_ = testPatternMode;
> > > >
> > > > return 0;
> > > > }
> > > >
> > > > I also wonder if the ASSERT() is not too harsh and we shouldn't just
> > > > error out. If the application tries to set an unsupported test pattern,
> > > > the whole library would crash, maybe we should be a little more
> > > > indulgent.
> > >
> > > Yes, not crashing is a good idea :-)
> > >
> > > > > > > +
> > > > > > > + value.set<int32_t>(index);
> > > > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value);
> > > > > > > +
> > > > > > > + return setControls(&ctrls);
> > > > > > > +}
> > > > > > > +
> > > > > > > /**
> > > > > > > * \brief Retrieve the best sensor format for a desired output
> > > > > > > * \param[in] mbusCodes The list of acceptable media bus codes
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list