| Message ID | 20210409043208.1823330-3-hiroh@chromium.org |
|---|---|
| State | RFC |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, thanks for reviewing. On Fri, Apr 9, 2021 at 5:55 PM Jacopo Mondi <jacopo@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@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? 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. 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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Mon, Apr 12, 2021 at 10:31 PM Jacopo Mondi <jacopo@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@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@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@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > > > > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Hiro, On 12/04/2021 14:50, Jacopo Mondi wrote: > 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@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. Personally, I think test patterns are a feature of the camera, and if available should be exposed (as a libcamera control). Indeed the difficulty might be mapping the menu type options to specific libcamera controls though in a generic way. > 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 :) Perhaps there is some mapping of custom menu items to libcamera controls required: > v4l2-ctl -d /dev/v4l-subdev0 --list-ctrls > VIMC Controls > > test_pattern 0x00f0f000 (menu) : min=0 max=21 default=0 value=0 > v4l2-ctl -d /dev/v4l-subdev0 --list-ctrls-menus > VIMC Controls > > test_pattern 0x00f0f000 (menu) : min=0 max=21 default=0 value=0 > 0: 75% Colorbar > 1: 100% Colorbar > 2: CSC Colorbar > 3: Horizontal 100% Colorbar > 4: 100% Color Squares > 5: 100% Black > 6: 100% White > 7: 100% Red > 8: 100% Green > 9: 100% Blue > 10: 16x16 Checkers > 11: 2x2 Checkers > 12: 1x1 Checkers > 13: 2x2 Red/Green Checkers > 14: 1x1 Red/Green Checkers > 15: Alternating Hor Lines > 16: Alternating Vert Lines > 17: One Pixel Wide Cross > 18: Two Pixels Wide Cross > 19: Ten Pixels Wide Cross > 20: Gray Ramp > 21: Noise Somehow we would have to map which of those is appropriate for a specific Android test pattern? But that should certainly be done in the android layer - not the libcamera layer ... This is not looking very easy to make generic :-( > 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@lists.libcamera.org >>>>>> https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hello, On Mon, Apr 12, 2021 at 08:45:35PM +0100, Kieran Bingham wrote: > On 12/04/2021 14:50, Jacopo Mondi wrote: > > On Mon, Apr 12, 2021 at 10:38:54PM +0900, Hirokazu Honda wrote: > >> On Mon, Apr 12, 2021 at 10:31 PM Jacopo Mondi wrote: > > > > [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. > > Personally, I think test patterns are a feature of the camera, and if > available should be exposed (as a libcamera control). Agreed. Conversion to Android camera metadata values should happen in the HAL (assuming the numerical values defined by the libcamera control would differ from the Android values). > Indeed the difficulty might be mapping the menu type options to specific > libcamera controls though in a generic way. > > > 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 :) > > Perhaps there is some mapping of custom menu items to libcamera controls > required: > > > v4l2-ctl -d /dev/v4l-subdev0 --list-ctrls > > VIMC Controls > > > > test_pattern 0x00f0f000 (menu) : min=0 max=21 default=0 value=0 > > > v4l2-ctl -d /dev/v4l-subdev0 --list-ctrls-menus > > VIMC Controls > > > > test_pattern 0x00f0f000 (menu) : min=0 max=21 default=0 value=0 > > 0: 75% Colorbar > > 1: 100% Colorbar > > 2: CSC Colorbar > > 3: Horizontal 100% Colorbar > > 4: 100% Color Squares > > 5: 100% Black > > 6: 100% White > > 7: 100% Red > > 8: 100% Green > > 9: 100% Blue > > 10: 16x16 Checkers > > 11: 2x2 Checkers > > 12: 1x1 Checkers > > 13: 2x2 Red/Green Checkers > > 14: 1x1 Red/Green Checkers > > 15: Alternating Hor Lines > > 16: Alternating Vert Lines > > 17: One Pixel Wide Cross > > 18: Two Pixels Wide Cross > > 19: Ten Pixels Wide Cross > > 20: Gray Ramp > > 21: Noise > > Somehow we would have to map which of those is appropriate for a > specific Android test pattern? > > But that should certainly be done in the android layer - not the > libcamera layer ... > > This is not looking very easy to make generic :-( If we define standard test patterns for the libcamera test pattern control, then mapping those test patterns to the V4L2 control values would belong to the sensor database. If we make the libcamera test pattern control a device-specific value without any standardization, then it wouldn't belong to the sensor database but in the HAL. An interesting point from the Android camera HAL documentation: "The HAL may choose to substitute test patterns from the sensor with test patterns from on-device memory. In that case, it should be indistinguishable to the ISP whether the data came from the sensor interconnect bus (such as CSI2) or memory." I wonder if that's what most implementations end up doing, and maybe it would make sense, given that there's no standardization of test patterns across different sensor models. At this point my feelign is that we should define libcamera test pattern control values based on how we expect those patterns to be used. The main use case (but I may be missing other use cases) is to support testing of the HAL, and to be able to implement standard tests, we need standard test patterns. Generating them in software and feeding them to the ISP would result in a more standard behaviour. What should we do, however, when the platform only has an inline ISP ? Hiro, could you provide a list (as exhaustive as possible) of use cases for test patterns ? > > Could you provide an example ? > > > >>>> What do you think? > >>>> > >>>>>> + 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 */
Hi Jacopo, Kieran and Laurent, thanks for comments. On Tue, Apr 13, 2021 at 9:55 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > On Mon, Apr 12, 2021 at 08:45:35PM +0100, Kieran Bingham wrote: > > On 12/04/2021 14:50, Jacopo Mondi wrote: > > > On Mon, Apr 12, 2021 at 10:38:54PM +0900, Hirokazu Honda wrote: > > >> On Mon, Apr 12, 2021 at 10:31 PM Jacopo Mondi wrote: > > > > > > [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. > > > > Personally, I think test patterns are a feature of the camera, and if > > available should be exposed (as a libcamera control). > > Agreed. Conversion to Android camera metadata values should happen in > the HAL (assuming the numerical values defined by the libcamera control > would differ from the Android values). > > > Indeed the difficulty might be mapping the menu type options to specific > > libcamera controls though in a generic way. > > > > > 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 :) > > > > Perhaps there is some mapping of custom menu items to libcamera controls > > required: > > > > > v4l2-ctl -d /dev/v4l-subdev0 --list-ctrls > > > VIMC Controls > > > > > > test_pattern 0x00f0f000 (menu) : min=0 max=21 default=0 value=0 > > > > > v4l2-ctl -d /dev/v4l-subdev0 --list-ctrls-menus > > > VIMC Controls > > > > > > test_pattern 0x00f0f000 (menu) : min=0 max=21 default=0 value=0 > > > 0: 75% Colorbar > > > 1: 100% Colorbar > > > 2: CSC Colorbar > > > 3: Horizontal 100% Colorbar > > > 4: 100% Color Squares > > > 5: 100% Black > > > 6: 100% White > > > 7: 100% Red > > > 8: 100% Green > > > 9: 100% Blue > > > 10: 16x16 Checkers > > > 11: 2x2 Checkers > > > 12: 1x1 Checkers > > > 13: 2x2 Red/Green Checkers > > > 14: 1x1 Red/Green Checkers > > > 15: Alternating Hor Lines > > > 16: Alternating Vert Lines > > > 17: One Pixel Wide Cross > > > 18: Two Pixels Wide Cross > > > 19: Ten Pixels Wide Cross > > > 20: Gray Ramp > > > 21: Noise > > > > Somehow we would have to map which of those is appropriate for a > > specific Android test pattern? > > > > But that should certainly be done in the android layer - not the > > libcamera layer ... > > > > This is not looking very easy to make generic :-( > > If we define standard test patterns for the libcamera test pattern > control, then mapping those test patterns to the V4L2 control values > would belong to the sensor database. If we make the libcamera test > pattern control a device-specific value without any standardization, > then it wouldn't belong to the sensor database but in the HAL. > > An interesting point from the Android camera HAL documentation: > > "The HAL may choose to substitute test patterns from the sensor with > test patterns from on-device memory. In that case, it should be > indistinguishable to the ISP whether the data came from the sensor > interconnect bus (such as CSI2) or memory." > > I wonder if that's what most implementations end up doing, and maybe it > would make sense, given that there's no standardization of test patterns > across different sensor models. > > At this point my feelign is that we should define libcamera test pattern > control values based on how we expect those patterns to be used. The > main use case (but I may be missing other use cases) is to support > testing of the HAL, and to be able to implement standard tests, we need > standard test patterns. Generating them in software and feeding them to > the ISP would result in a more standard behaviour. What should we do, > however, when the platform only has an inline ISP ? > > Hiro, could you provide a list (as exhaustive as possible) of use cases > for test patterns ? > In our test, ANDROID_SENSOR_TEST_PATTERN_MODE_COLOR_BARS_FADE_TO_GRAY and ANDROID_SENSOR_TEST_PATTERN_MODE_COLOR_BARS are used. With test pattern mode frames, we test no corruption in frames. https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_frame_fixture.h;l=100;drc=2837ddd0fde71236264c417fc5874ba3646d9a46 I discussed with Jacopo via IRC chat. The proper solution is the following: 1.) Add menu support to controls. 2.) V4L2Device store all supported test pattern values with controls. 3.) CameraSensor gets the test pattern values (name, etc) via V4L2Device::controls(). 4.) CameraSensor converts them to libcamera test pattern control values by using a conversion table in CameraSensorDatabase 5.) IPU3 reports the libcamera test pattern control values to Android HAL. 6.) Android HAL convers the libcamera test pattern control values to Android test pattern values. I will submit the next patch series with this solution (except 6) as RFC with ccing all of you. -Hiro > > > Could you provide an example ? > > > > > >>>> What do you think? > > >>>> > > >>>>>> + 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 */ > > -- > Regards, > > Laurent Pinchart
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" /** * \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); + 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 */
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@chromium.org> --- include/libcamera/internal/v4l2_subdevice.h | 5 + src/libcamera/v4l2_subdevice.cpp | 104 ++++++++++++++++++++ 2 files changed, 109 insertions(+)