[libcamera-devel] [RFC PATCH 2/5] libcamera: V4L2Subdevice: Add getter/setter function for test pattern mode
Jacopo Mondi
jacopo at jmondi.org
Mon Apr 12 15:50:02 CEST 2021
Hi Hiro,
On Mon, Apr 12, 2021 at 10:38:54PM +0900, Hirokazu Honda wrote:
> Hi Jacopo,
>
> On Mon, Apr 12, 2021 at 10:31 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi Hiro,
> >
[snip]
> > > > > +/**
> > > > > + * \fn V4L2Subdevice::getAvailableTestPatternModes()
> > > > > + * \brief Retrieve the available test pattern modes.
> > > > > + *
> > > > > + * \return The available control::SensorTestPatternMode values.
> > > > > + */
> > > > > +std::vector<int32_t> V4L2Subdevice::getAvailableTestPatternModes()
> > > > > +{
> > > > > + std::vector<int32_t> patternModes;
> > > > > + if (!testPatternModeMap_.empty()) {
> > > > > + for (const auto &mode : testPatternModeMap_)
> > > > > + patternModes.push_back(mode.first);
> > > > > + return patternModes;
> > > > > + }
> > > > > +
> > > > > + v4l2_queryctrl ctrl;
> > > > > + memset(&ctrl, 0, sizeof(ctrl));
> > > > > + ctrl.id = V4L2_CID_TEST_PATTERN;
> > > > > + int ret = ioctl(VIDIOC_QUERYCTRL, &ctrl);
> > > >
> > > > I'm not sure if you have considered the following and found any
> > > > blocker, but:
> > > >
> > > > - Controls are enumerated in V4L2Device::listControls() with
> > > > VIDIOC_QUERY_EXT_CTRL
> > > > - Menu controls are accepted but not really handled there yet. I think
> > > > you should modify listControls() to handle menu controls properly
> > > > and store them as ControlInfo. ControlInfo currently support being
> > > > constructed with a Span<> of values but as far as I can tell
> > > > V4l2ControlInfo does not.
> > > > - Once you have valid ControlInfo for the menu control, it should
> > > > contain the V4L2 identifers for the menu entries
> > > > - The pipeline handler should then populate the libcamera controls in
> > > > Camera::controls_ by inspecting the V4L2 controls returned by the
> > > > v4l2 subdevice as we do today in IPU3::listControls(). You should
> > > > use the libcamera controls identifiers that you have defined in
> > > > patch #1, not the android defined values
> > > > - The camera HAL inspects the Camera::controls() to translate from
> > > > libcamera defined controls to Android defined metadata
> > > >
> > > > Does this make sense to you ?
> > > >
> > >
> > > That sounds good to me.
> > > However, do you think it makes more sense to add available test
> > > pattern modes to CameraSensorDataBase?
> >
> > Do you mean recording the V4L2 test pattern modes there ? What would we gain
> > compared to fetching them from the kernel interface ?
> >
> > If you mean the Android test pattern mode then no, the sensor database
> > is a core libcamera construct, it knows nothing about Android. One
> > option for Android-specific values is instead the HAL configuration
> > file.
> >
>
> Hmm, so should we have our own test pattern mode definition a part of
> which are equivalent to some useful Android test pattern modes?
If you mean recording them in the HAL configuration file you can
record the android values (or better, their numeric values). There is
not need to have them defined as libcamera controls if we have no
strict need to do so.
> Then the definitions are converted to Android ones in HAL configuration?
>
That would happen if the Camera HAL has to convert from the libcamera
controls to android ones. It really depends if libcamera wants a
control for the test pattern modes.
CC-ed Laurent
> > > The reason is, as you saw in this patch, there is no way of mapping to
> > > V4L2_CID_TEST_PATTERN value to android one.
> > > This is a problem in v4l2 api. An app has to know beforehand what test
> > > pattern mode the name returned by a driver represents.
> >
> > Ah do you mean that the test patter names are driver specific ? Good
> > job V4L2! I see your point there. I won't be ashamed of having them in
> > the HAL configuration file, or to perform a best-effort mapping the
> > Camera HAL. Not sure what's best tbh
> >
>
> Right. The available test pattern modes are not device specific, like location.
Well, not really. Location has a set of values it can be assigned to.
The test pattern modes, if I got you right are free formed text, which
makes it very hard to translate them to a fixed set of values like the
ones android defines.
> So I think it is more natural to put the info to CameraSensorDatabase.
>
I still don't get what you would record in the sensor database, maybe
I'm still asleep :)
Could you provide an example ?
Thanks
j
> -Hiro
>
> > >
> > > What do you think?
> > > -Hiro
> > >
> > > > Thanks
> > > > j
> > > >
> > > > > + if (ret < 0) {
> > > > > + LOG(V4L2, Error) << "Unable to get test pattern mode :"
> > > > > + << strerror(-ret);
> > > > > + return {};
> > > > > + }
> > > >
> > > > > +
> > > > > + v4l2_querymenu menu;
> > > > > + memset(&menu, 0, sizeof(menu));
> > > > > + menu.id = ctrl.id;
> > > > > + for (menu.index = ctrl.minimum;
> > > > > + static_cast<int>(menu.index) <= ctrl.maximum; menu.index++) {
> > > > > + if (ioctl(VIDIOC_QUERYMENU, &menu) != 0) {
> > > > > + continue;
> > > > > + }
> > > > > +
> > > > > + const std::string modeName(reinterpret_cast<char *>(menu.name));
> > > > > + std::optional<int32_t> androidTestPatternMode;
> > > > > + /*
> > > > > + * ov13858 and ov5670.
> > > > > + * No corresponding value for "Vertical Color Bar Type 3" and
> > > > > + * "Vertical Color Bar Type 4".
> > > > > + */
> > > > > + if (modeName == "Disabled") {
> > > > > + androidTestPatternMode =
> > > > > + ANDROID_SENSOR_TEST_PATTERN_MODE_OFF;
> > > > > + } else if (modeName == "Vertical Color Bar Type 1") {
> > > > > + androidTestPatternMode =
> > > > > + ANDROID_SENSOR_TEST_PATTERN_MODE_COLOR_BARS;
> > > > > + } else if (modeName == "Vertical Color Bar Type 2") {
> > > > > + androidTestPatternMode =
> > > > > + ANDROID_SENSOR_TEST_PATTERN_MODE_COLOR_BARS_FADE_TO_GRAY;
> > > > > + }
> > > > > +
> > > > > + if (androidTestPatternMode) {
> > > > > + testPatternModeMap_[*androidTestPatternMode] = menu.index;
> > > > > + patternModes.push_back(*androidTestPatternMode);
> > > > > + } else {
> > > > > + LOG(V4L2, Debug) << "Skip test pattern mode: "
> > > > > + << modeName;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + return patternModes;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * \fn V4L2Subdevice::getAvailableTestPatternModes()
> > > > > + * \brief Set the test pattern mode.
> > > > > + *
> > > > > + * \return 0 on success or a negative error code otherwise.
> > > > > + */
> > > > > +int V4L2Subdevice::setTestPatternMode(int32_t testPatternMode)
> > > > > +{
> > > > > + auto it = testPatternModeMap_.find(testPatternMode);
> > > > > + if (it != testPatternModeMap_.end()) {
> > > > > + LOG(V4L2, Error) << "Unsupported test pattern mode: "
> > > > > + << testPatternMode;
> > > > > +
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + v4l2_control ctrl;
> > > > > + memset(&ctrl, 0, sizeof(ctrl));
> > > > > + ctrl.id = V4L2_CID_TEST_PATTERN;
> > > > > + ctrl.value = it->second;
> > > > > + int ret = ioctl(VIDIOC_S_CTRL, &ctrl);
> > > > > + if (ret < 0) {
> > > > > + LOG(V4L2, Error) << "Unable to set test pattern mode: "
> > > > > + << strerror(-ret);
> > > > > +
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > } /* namespace libcamera */
> > > > > --
> > > > > 2.31.1.295.g9ea45b61b8-goog
> > > > >
> > > > > _______________________________________________
> > > > > libcamera-devel mailing list
> > > > > libcamera-devel at lists.libcamera.org
> > > > > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list