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

[Mattijs Korpershoek]

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

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

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