[libcamera-devel] [PATCH] pipeline: simple: set controlInfo_ to sensor_->controls()

[Jacopo Mondi]

Hi Mattijs

On Tue, Apr 04, 2023 at 11:09:25AM +0200, Mattijs Korpershoek wrote:
> Hi Jacopo,
>
> Thank you for your review and detailed reponse.
>
> On lun., avril 03, 2023 at 10:17, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:
>
> > Hi Mattijs
> >
> > On Fri, Mar 31, 2023 at 05:09:58PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
> >> Each pipeline has a ControlInfoMap to list the set of controls supported
> >> by the camera.
> >>
> >> ControlInfoMap() default constructor -used by simple pipeline-,
> >> initializes its idmap_ member to nullptr.
> >>
> >> Other methods such as ControlInfoMap::find() don't check for idmap_'s
> >> validity so the following code (from find()) crashes:
> >>
> >>         auto iter = idmap_->find(id);
> >>         auto iter = nullptr->find(id);
> >>
> >> Fix this by assigning the sensor's control info map.
> >>
> >
> > Eh, I wish it was that simple :)
>
> Hmm, it fixed my nullptr crash, so I figured, it's "good enough". Seems
> I was terribly wrong :)
>

Don't worry, many people got fooled by this, me included :)

> >
> > The issue of missing controls for Simple is plaguing it since a long
> > time and it's a rather complex one.
> >
> > First of all, you're here registering controls straight as they come
> > from the v4l2-subdev v4l2-controls interface, which means you're
> > exposing to applications the V4L2 controls numeric identifiers.
> > Libcamera aims instead to expose to applications its own defined
> > controls (control_ids.yaml) for sake of applications portability
> > across platforms.
> >
> > This also requires that the controls' valid ranges and values should
> > be abstracted away from the platform details, and it's up to the
> > pipeline-handler/IPA to re-scale them in values that make sense for
> > the hardware in use.
> >
> > For platforms with an ISP, most controls come from the IPA module
> > which register both controls to drive the algorithms (enable/disable,
> > mode of operations etc) and controls to manually tune the parameters
> > of the image processing pipeline. When these controls directly apply
> > to the sensor (as ExposureTime and AnalogueGain, in example) you can
> > see how the CameraSensorHelper hierarchy helps translating the
> > controls to sensor-consumable values in the IPA modules.
> >
> > The closest example to Simple we have is the uvcvideo pipeline, where
> > you can see how controls from the v4l2 interface are translated to
> > libcamera generic controls.
> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp#n582
> >
> > For Simple there have been a few attempts in the past and recently
> > there was desire to re-tackle the issue for supporting auto-focus on
> > the Pinephone platform but I've lost track of the development on the
> > fron (cc Adam and Rafael)
> > https://patchwork.libcamera.org/patch/15078/#21709
>
> Understood. For the time being, I don't feel confident enough to
> make the controls in Simple pipeline behave as they should. Especially
> if others are already working on this.
>
> I do want to fix the nullptr dereference I'm facing when using Simple
> Pipeline along with the provided Android camera HAL.
>
> Here is a stacktrace (from Android) when this happens:
> http://codepad.org/CiLLcPNW
>

This puzzles me a bit...

Indeed the camera controls for Simple are not populated, but I wasn't
expecting a segfault, but rather an assertion failure.

The call trace points to

int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)

where the request's controls are populated translating Android
controls to libcamera's ones.

The request list is populated with the Camera's validator when
request is created

Request::Request(Camera *camera, uint64_t cookie)
	: Extensible(std::make_unique<Private>(camera)),
	  cookie_(cookie), status_(RequestPending)
{
	controls_ = new ControlList(controls::controls,
				    camera->_d()->validator());

I presume the call to

	controls.set(controls::ScalerCrop, cropRegion);

in CameraDevice goes through

	template<typename T, typename V>
	void set(const Control<T> &ctrl, const V &value)
	{
		ControlValue *val = find(ctrl.id());
		if (!val)
			return;

		val->set<T>(value);
	}

which calls

ControlValue *ControlList::find(unsigned int id)
{
	if (validator_ && !validator_->validate(id)) {

and finally

bool CameraControlValidator::validate(unsigned int id) const
{
	const ControlInfoMap &controls = camera_->controls();
	return controls.find(id) != controls.end();
}

with the offendin call being

	return controls.find(id) != controls.end();

If you look at the implementation of

ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
{
	auto iter = idmap_->find(id);
	if (iter == idmap_->end())
		return end();

	return find(iter->second);
}

you'll see that the default constructor for ControlInfoMap doesn't
intialized idmap_ which is initialized at class declaration time as

	const ControlIdMap *idmap_ = nullptr;

To sum it up, a Camera is constructed with an unsafe controlInfo_


> Is adding some null checks in ControlInfoMap::find() an acceptable
> solution?
>
> >
> >
> >> Signed-off-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
> >> ---
> >> Alternatively, we could null check everywhere in controls.cpp or
> >> get rid of the default constructor for ControlInfoMap.
> >> ---

And if I would have read this, I would have spare the above. Well,
we're on the same page at least :)

I tried rather hard to make idmap_ an instance intead of a pointer.

This however has two consequences:
1) operator= cannot be default-generated, and in the implementation I
   tried I was not able to correctly copy the Map in. I'm afraid the only
   solution here would be to use composition instead of having
   ControlInfoMap inherit from unordered_map

2) ControlInfo might needs to reference the global controls::controls id
   map, which might be expensive to copy in if idmap_ is now made an
   instance. This is also relevant for control serialization, which
   happens on IPC borders and is in a rather hot path

I'm afraid we'll have to protect the ControlInfoMap class methods with
an
        if (!idmap_)
                return end();

What do you think ?


> >>  src/libcamera/pipeline/simple/simple.cpp | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >> index 2423ec10c878..4db3dc2812b0 100644
> >> --- a/src/libcamera/pipeline/simple/simple.cpp
> >> +++ b/src/libcamera/pipeline/simple/simple.cpp
> >> @@ -537,6 +537,7 @@ int SimpleCameraData::init()
> >>  	}
> >>
> >>  	properties_ = sensor_->properties();
> >> +	controlInfo_ = sensor_->controls();
> >>
> >>  	return 0;
> >>  }
> >>
> >> ---
> >> base-commit: ac7511dc4c594f567ddff27ccc02c30bf6c00bfd
> >> change-id: 20230331-simple-controls-fd92853c7cff
> >>
> >> Best regards,
> >> --
> >> Mattijs Korpershoek <mkorpershoek at baylibre.com>
> >>