[libcamera-devel] Revert "libcamera: V4L2Device: Remove the controls order assumption in updateControls()"
diff mbox series

Message ID 20210525085528.11477-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel] Revert "libcamera: V4L2Device: Remove the controls order assumption in updateControls()"
Related show

Commit Message

Laurent Pinchart May 25, 2021, 8:55 a.m. UTC 
This reverts commit 34bee5e84ecba01e0ded5cacbc46c277c5a0edba.

The commit introduced a breakage in the master branch, reported by
linux-surface users already. Let's revert it while discussing the
propert fix.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/v4l2_device.cpp | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

Comments

Jacopo Mondi May 25, 2021, 9:04 a.m. UTC | #1 
Hi Laurent,

On Tue, May 25, 2021 at 11:55:28AM +0300, Laurent Pinchart wrote:
> This reverts commit 34bee5e84ecba01e0ded5cacbc46c277c5a0edba.
>
> The commit introduced a breakage in the master branch, reported by
> linux-surface users already. Let's revert it while discussing the
> propert fix.

I see a potential fix on the list but for now let's unblock users fast
Acked-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/v4l2_device.cpp | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 7f7e5b8fdf09..caafbc2d16bb 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -244,6 +244,12 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>
>  	ControlList ctrls{ controls_ };
>
> +	/*
> +	 * Start by filling the ControlList. This can't be combined with filling
> +	 * v4l2Ctrls, as updateControls() relies on both containers having the
> +	 * same order, and the control list is based on a map, which is not
> +	 * sorted by insertion order.
> +	 */
>  	for (uint32_t id : ids) {
>  		const auto iter = controls_.find(id);
>  		if (iter == controls_.end()) {
> @@ -617,13 +623,19 @@ void V4L2Device::updateControlInfo()
>  void V4L2Device::updateControls(ControlList *ctrls,
>  				Span<const v4l2_ext_control> v4l2Ctrls)
>  {
> -	for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {
> -		const unsigned int id = v4l2Ctrl.id;
> +	unsigned int i = 0;
> +	for (auto &ctrl : *ctrls) {
> +		if (i == v4l2Ctrls.size())
> +			break;
>
> -		ControlValue value = ctrls->get(id);
> -		switch (value.type()) {
> +		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> +		unsigned int id = ctrl.first;
> +		ControlValue &value = ctrl.second;
> +
> +		const auto iter = controls_.find(id);
> +		switch (iter->first->type()) {
>  		case ControlTypeInteger64:
> -			value.set<int64_t>(v4l2Ctrl.value64);
> +			value.set<int64_t>(v4l2Ctrl->value64);
>  			break;
>
>  		case ControlTypeByte:
> @@ -638,11 +650,11 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  			 * \todo To be changed when support for string controls
>  			 * will be added.
>  			 */
> -			value.set<int32_t>(v4l2Ctrl.value);
> +			value.set<int32_t>(v4l2Ctrl->value);
>  			break;
>  		}
>
> -		ctrls->set(id, value);
> +		i++;
>  	}
>  }
>
> --
> Regards,
>
> Laurent Pinchart
>
Hirokazu Honda May 25, 2021, 9:04 a.m. UTC | #2 
Hi Laurent, thanks for reverting.

On Tue, May 25, 2021 at 6:03 PM Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Laurent,
>
> On Tue, May 25, 2021 at 11:55:28AM +0300, Laurent Pinchart wrote:
> > This reverts commit 34bee5e84ecba01e0ded5cacbc46c277c5a0edba.
> >
> > The commit introduced a breakage in the master branch, reported by
> > linux-surface users already. Let's revert it while discussing the
> > propert fix.
>
> I see a potential fix on the list but for now let's unblock users fast
> Acked-by: Jacopo Mondi <jacopo@jmondi.org>
>
>
Acked-by: Hirokazu Honda <hiroh@chromium.org>

Thanks a lot,
-Hiro

> Thanks
>    j
>
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/v4l2_device.cpp | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/libcamera/v4l2_device.cpp
> b/src/libcamera/v4l2_device.cpp
> > index 7f7e5b8fdf09..caafbc2d16bb 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -244,6 +244,12 @@ ControlList V4L2Device::getControls(const
> std::vector<uint32_t> &ids)
> >
> >       ControlList ctrls{ controls_ };
> >
> > +     /*
> > +      * Start by filling the ControlList. This can't be combined with
> filling
> > +      * v4l2Ctrls, as updateControls() relies on both containers having
> the
> > +      * same order, and the control list is based on a map, which is not
> > +      * sorted by insertion order.
> > +      */
> >       for (uint32_t id : ids) {
> >               const auto iter = controls_.find(id);
> >               if (iter == controls_.end()) {
> > @@ -617,13 +623,19 @@ void V4L2Device::updateControlInfo()
> >  void V4L2Device::updateControls(ControlList *ctrls,
> >                               Span<const v4l2_ext_control> v4l2Ctrls)
> >  {
> > -     for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {
> > -             const unsigned int id = v4l2Ctrl.id;
> > +     unsigned int i = 0;
> > +     for (auto &ctrl : *ctrls) {
> > +             if (i == v4l2Ctrls.size())
> > +                     break;
> >
> > -             ControlValue value = ctrls->get(id);
> > -             switch (value.type()) {
> > +             const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> > +             unsigned int id = ctrl.first;
> > +             ControlValue &value = ctrl.second;
> > +
> > +             const auto iter = controls_.find(id);
> > +             switch (iter->first->type()) {
> >               case ControlTypeInteger64:
> > -                     value.set<int64_t>(v4l2Ctrl.value64);
> > +                     value.set<int64_t>(v4l2Ctrl->value64);
> >                       break;
> >
> >               case ControlTypeByte:
> > @@ -638,11 +650,11 @@ void V4L2Device::updateControls(ControlList *ctrls,
> >                        * \todo To be changed when support for string
> controls
> >                        * will be added.
> >                        */
> > -                     value.set<int32_t>(v4l2Ctrl.value);
> > +                     value.set<int32_t>(v4l2Ctrl->value);
> >                       break;
> >               }
> >
> > -             ctrls->set(id, value);
> > +             i++;
> >       }
> >  }
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
>
Jean-Michel Hautbois May 25, 2021, 9:05 a.m. UTC | #3 
Hi Laurent,

On 25/05/2021 11:04, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Tue, May 25, 2021 at 11:55:28AM +0300, Laurent Pinchart wrote:
>> This reverts commit 34bee5e84ecba01e0ded5cacbc46c277c5a0edba.
>>
>> The commit introduced a breakage in the master branch, reported by
>> linux-surface users already. Let's revert it while discussing the
>> propert fix.
> 
> I see a potential fix on the list but for now let's unblock users fast
> Acked-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Thanks
>    j
> 
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
This is indeed a blocker on master, so:
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> ---
>>  src/libcamera/v4l2_device.cpp | 26 +++++++++++++++++++-------
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index 7f7e5b8fdf09..caafbc2d16bb 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -244,6 +244,12 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>>
>>  	ControlList ctrls{ controls_ };
>>
>> +	/*
>> +	 * Start by filling the ControlList. This can't be combined with filling
>> +	 * v4l2Ctrls, as updateControls() relies on both containers having the
>> +	 * same order, and the control list is based on a map, which is not
>> +	 * sorted by insertion order.
>> +	 */
>>  	for (uint32_t id : ids) {
>>  		const auto iter = controls_.find(id);
>>  		if (iter == controls_.end()) {
>> @@ -617,13 +623,19 @@ void V4L2Device::updateControlInfo()
>>  void V4L2Device::updateControls(ControlList *ctrls,
>>  				Span<const v4l2_ext_control> v4l2Ctrls)
>>  {
>> -	for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {
>> -		const unsigned int id = v4l2Ctrl.id;
>> +	unsigned int i = 0;
>> +	for (auto &ctrl : *ctrls) {
>> +		if (i == v4l2Ctrls.size())
>> +			break;
>>
>> -		ControlValue value = ctrls->get(id);
>> -		switch (value.type()) {
>> +		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
>> +		unsigned int id = ctrl.first;
>> +		ControlValue &value = ctrl.second;
>> +
>> +		const auto iter = controls_.find(id);
>> +		switch (iter->first->type()) {
>>  		case ControlTypeInteger64:
>> -			value.set<int64_t>(v4l2Ctrl.value64);
>> +			value.set<int64_t>(v4l2Ctrl->value64);
>>  			break;
>>
>>  		case ControlTypeByte:
>> @@ -638,11 +650,11 @@ void V4L2Device::updateControls(ControlList *ctrls,
>>  			 * \todo To be changed when support for string controls
>>  			 * will be added.
>>  			 */
>> -			value.set<int32_t>(v4l2Ctrl.value);
>> +			value.set<int32_t>(v4l2Ctrl->value);
>>  			break;
>>  		}
>>
>> -		ctrls->set(id, value);
>> +		i++;
>>  	}
>>  }
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
Umang Jain May 25, 2021, 9:07 a.m. UTC | #4 
Hi Laurent,

On 5/25/21 2:25 PM, Laurent Pinchart wrote:
> This reverts commit 34bee5e84ecba01e0ded5cacbc46c277c5a0edba.
>
> The commit introduced a breakage in the master branch, reported by
> linux-surface users already. Let's revert it while discussing the
> propert fix.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
I started facing

cam: 
../../../../../tmp/portage/media-libs/libcamera-9999/work/libcamera-9999/include/libcamera/controls.h:150: 
T libcamera::ControlValue::get() const [T = long]: Assertion `type_ == 
details::control_type<std::remove_cv_t<T>>::value' failed.
Aborted (core dumped)

today morning, under the impression it was due to my local changes for 
IPA controls on nautilus. So, applying this revert solves the issue on 
my end.

Tested-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   src/libcamera/v4l2_device.cpp | 26 +++++++++++++++++++-------
>   1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 7f7e5b8fdf09..caafbc2d16bb 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -244,6 +244,12 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>   
>   	ControlList ctrls{ controls_ };
>   
> +	/*
> +	 * Start by filling the ControlList. This can't be combined with filling
> +	 * v4l2Ctrls, as updateControls() relies on both containers having the
> +	 * same order, and the control list is based on a map, which is not
> +	 * sorted by insertion order.
> +	 */
>   	for (uint32_t id : ids) {
>   		const auto iter = controls_.find(id);
>   		if (iter == controls_.end()) {
> @@ -617,13 +623,19 @@ void V4L2Device::updateControlInfo()
>   void V4L2Device::updateControls(ControlList *ctrls,
>   				Span<const v4l2_ext_control> v4l2Ctrls)
>   {
> -	for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {
> -		const unsigned int id = v4l2Ctrl.id;
> +	unsigned int i = 0;
> +	for (auto &ctrl : *ctrls) {
> +		if (i == v4l2Ctrls.size())
> +			break;
>   
> -		ControlValue value = ctrls->get(id);
> -		switch (value.type()) {
> +		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> +		unsigned int id = ctrl.first;
> +		ControlValue &value = ctrl.second;
> +
> +		const auto iter = controls_.find(id);
> +		switch (iter->first->type()) {
>   		case ControlTypeInteger64:
> -			value.set<int64_t>(v4l2Ctrl.value64);
> +			value.set<int64_t>(v4l2Ctrl->value64);
>   			break;
>   
>   		case ControlTypeByte:
> @@ -638,11 +650,11 @@ void V4L2Device::updateControls(ControlList *ctrls,
>   			 * \todo To be changed when support for string controls
>   			 * will be added.
>   			 */
> -			value.set<int32_t>(v4l2Ctrl.value);
> +			value.set<int32_t>(v4l2Ctrl->value);
>   			break;
>   		}
>   
> -		ctrls->set(id, value);
> +		i++;
>   	}
>   }
>

Patch
diff mbox series 

diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 7f7e5b8fdf09..caafbc2d16bb 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -244,6 +244,12 @@  ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
 
 	ControlList ctrls{ controls_ };
 
+	/*
+	 * Start by filling the ControlList. This can't be combined with filling
+	 * v4l2Ctrls, as updateControls() relies on both containers having the
+	 * same order, and the control list is based on a map, which is not
+	 * sorted by insertion order.
+	 */
 	for (uint32_t id : ids) {
 		const auto iter = controls_.find(id);
 		if (iter == controls_.end()) {
@@ -617,13 +623,19 @@  void V4L2Device::updateControlInfo()
 void V4L2Device::updateControls(ControlList *ctrls,
 				Span<const v4l2_ext_control> v4l2Ctrls)
 {
-	for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {
-		const unsigned int id = v4l2Ctrl.id;
+	unsigned int i = 0;
+	for (auto &ctrl : *ctrls) {
+		if (i == v4l2Ctrls.size())
+			break;
 
-		ControlValue value = ctrls->get(id);
-		switch (value.type()) {
+		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
+		unsigned int id = ctrl.first;
+		ControlValue &value = ctrl.second;
+
+		const auto iter = controls_.find(id);
+		switch (iter->first->type()) {
 		case ControlTypeInteger64:
-			value.set<int64_t>(v4l2Ctrl.value64);
+			value.set<int64_t>(v4l2Ctrl->value64);
 			break;
 
 		case ControlTypeByte:
@@ -638,11 +650,11 @@  void V4L2Device::updateControls(ControlList *ctrls,
 			 * \todo To be changed when support for string controls
 			 * will be added.
 			 */
-			value.set<int32_t>(v4l2Ctrl.value);
+			value.set<int32_t>(v4l2Ctrl->value);
 			break;
 		}
 
-		ctrls->set(id, value);
+		i++;
 	}
 }