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

Message ID 20230331-simple-controls-v1-1-f7b8327d2c26@baylibre.com
State Superseded
Headers show
Series
  • [libcamera-devel] pipeline: simple: set controlInfo_ to sensor_->controls()
Related show

Commit Message

Mattijs Korpershoek March 31, 2023, 3:09 p.m. UTC 
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.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@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(+)


---
base-commit: ac7511dc4c594f567ddff27ccc02c30bf6c00bfd
change-id: 20230331-simple-controls-fd92853c7cff

Best regards,

Comments

Jacopo Mondi April 3, 2023, 8:17 a.m. UTC | #1 
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 :)

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


> Signed-off-by: Mattijs Korpershoek <mkorpershoek@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@baylibre.com>
>
Mattijs Korpershoek April 4, 2023, 9:09 a.m. UTC | #2 
Hi Jacopo,

Thank you for your review and detailed reponse.

On lun., avril 03, 2023 at 10:17, Jacopo Mondi <jacopo.mondi@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@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@baylibre.com>
>>
Jacopo Mondi April 4, 2023, 10:43 a.m. UTC | #3 
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@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@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@baylibre.com>
> >>
Mattijs Korpershoek April 4, 2023, 12:04 p.m. UTC | #4 
Hi Jacopo,

On mar., avril 04, 2023 at 12:43, Jacopo Mondi <jacopo.mondi@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@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@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@baylibre.com>
>> >>
Mattijs Korpershoek April 4, 2023, 4:12 p.m. UTC | #5 
On mar., avril 04, 2023 at 14:04, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:

> Hi Jacopo,
>
> On mar., avril 04, 2023 at 12:43, Jacopo Mondi <jacopo.mondi@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@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@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@baylibre.com>
>>> >>

Patch
diff mbox series 

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