[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:32:17 CEST 2021
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.
> 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
>
> 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