[libcamera-devel] [RFC PATCH 2/5] libcamera: V4L2Subdevice: Add getter/setter function for test pattern mode
Hirokazu Honda
hiroh at chromium.org
Mon Apr 12 15:38:54 CEST 2021
Hi Jacopo,
On Mon, Apr 12, 2021 at 10:31 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Hiro,
>
> On Mon, Apr 12, 2021 at 10:12:07PM +0900, Hirokazu Honda wrote:
> > Hi Jacopo, thanks for reviewing.
> >
> > On Fri, Apr 9, 2021 at 5:55 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > >
> > > Hi Hiro,
> > >
> > > On Fri, Apr 09, 2021 at 01:32:05PM +0900, Hirokazu Honda wrote:
> > > > The supported test pattern modes can be obtained by calling
> > > > VIDIOC_QUERYMENU to a camera sensor device. This implements the
> > > > getter and setter functions for the test pattern mode in
> > > > V4L2SubDevice.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > ---
> > > > include/libcamera/internal/v4l2_subdevice.h | 5 +
> > > > src/libcamera/v4l2_subdevice.cpp | 104 ++++++++++++++++++++
> > > > 2 files changed, 109 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> > > > index d2b9ca55..f2f5d3f6 100644
> > > > --- a/include/libcamera/internal/v4l2_subdevice.h
> > > > +++ b/include/libcamera/internal/v4l2_subdevice.h
> > > > @@ -60,6 +60,9 @@ public:
> > > > int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > > Whence whence = ActiveFormat);
> > > >
> > > > + std::vector<int32_t> getAvailableTestPatternModes();
> > > > + int setTestPatternMode(int32_t testPatternMode);
> > > > +
> > > > static std::unique_ptr<V4L2Subdevice>
> > > > fromEntityName(const MediaDevice *media, const std::string &entity);
> > > >
> > > > @@ -74,6 +77,8 @@ private:
> > > > unsigned int code);
> > > >
> > > > const MediaEntity *entity_;
> > > > +
> > > > + std::map<int32_t, uint32_t> testPatternModeMap_;
> > > > };
> > > >
> > > > } /* namespace libcamera */
> > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > > index 721ff5a9..130e9c4d 100644
> > > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > > @@ -24,6 +24,7 @@
> > > > #include "libcamera/internal/media_object.h"
> > > > #include "libcamera/internal/utils.h"
> > > >
> > > > +#include "android/metadata/system/camera_metadata_tags.h"
> > >
> > > I'm afraid this is not correct. We don't want any android specific
> > > definition in libcamera core. libcamera run on non-android systems,
> > > and we don't want generic application to depend on anything Andoroid.
> > >
> > > > /**
> > > > * \file v4l2_subdevice.h
> > > > * \brief V4L2 Subdevice API
> > > > @@ -523,4 +524,107 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > > > return sizes;
> > > > }
> > > >
> > > > +/**
> > > > + * \var V4L2Subdevice::testPatternModeMap_
> > > > + * \brief The map from controls::SensorTestPatternMode to an index value to be
> > > > + * used for V4L2_CID_TEST_PATTERN.
> > > > + *
> > > > + * The map is constructed in getAvailableTestPatternModes() and referred in
> > > > + * setTestPatternMode() to find a value associated with the requested test
> > > > + * pattern mode.
> > > > + */
> > > > +/**
> > > > + * \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?
Then the definitions are converted to Android ones in HAL configuration?
> > 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.
So I think it is more natural to put the info to CameraSensorDatabase.
-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