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

Tue Apr 4 14:04:09 CEST 2023

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.

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