[libcamera-devel] [RFC PATCH 2/5] libcamera: V4L2Subdevice: Add getter/setter function for test pattern mode
Hirokazu Honda
hiroh at chromium.org
Tue Apr 13 09:40:40 CEST 2021
Hi Jacopo, Kieran and Laurent, thanks for comments.
On Tue, Apr 13, 2021 at 9:55 AM Laurent Pinchart
<laurent.pinchart at 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
More information about the libcamera-devel
mailing list