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

Mattijs Korpershoek mkorpershoek at baylibre.com
Tue Apr 4 18:12:02 CEST 2023 

On mar., avril 04, 2023 at 14:04, Mattijs Korpershoek <mkorpershoek at baylibre.com> wrote:

> Hi Jacopo,
>
> On mar., avril 04, 2023 at 12:43, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:
>
>> 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 agree, we are on the same page. Thank you for the write-up though,
> it's a way better explanation than what I've done in the commit message.
>
>>
>> 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 ?
>
> I think this is the most trivial solution and i'm willing to implement
> this and submit it.
>
> Another alternative I can think of is not providing a default
> constructor for ControlInfoMap but that seems difficult/also not
> necessarily a good idea.
>
> I will test and submit the trivial solution in the coming days. Thanks
> again for your help.

I've submitted this here:
https://lists.libcamera.org/pipermail/libcamera-devel/2023-April/037443.html

>
> Mattijs
>
>>
>>
>>> >>  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>
>>> >>

More information about the libcamera-devel mailing list