[libcamera-devel] pipeline: raspberrypi: Fix for staggered write on reset

3873 diff mbox series

The reset function in staggered write was using the wrong index when
looking for the last updated camera parameters. This would cause
possibly stale exposure values to be written to the camera on a
mode switch for captures.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Naush

Thanks for submitting this one!

On Thu, 28 May 2020 at 15:56, Naushir Patuck <naush@raspberrypi.com> wrote:

> The reset function in staggered write was using the wrong index when
> looking for the last updated camera parameters. This would cause
> possibly stale exposure values to be written to the camera on a
> mode switch for captures.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
> b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
> index 391e13f5..b26fa63d 100644
> --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
> @@ -46,7 +46,7 @@ void StaggeredCtrl::reset()
>  {
>         std::lock_guard<std::mutex> lock(lock_);
>
> -       int lastSetCount = std::max<int>(0, setCount_ - 1);
> +       int lastSetCount = std::max<int>(0, setCount_);
>

Actually I wonder whether we shouldn't just go with

       uint32_t lastSetCount = setCount_;

(the std::max seems a bit redundant now that we don't subtract 1).


>         std::unordered_map<uint32_t, int32_t> lastVal;
>
>         /* Reset the counters. */
> --
> 2.25.1
>
>

Hi David,

On Thu, 28 May 2020 at 17:06, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Hi Naush
>
> Thanks for submitting this one!
>
> On Thu, 28 May 2020 at 15:56, Naushir Patuck <naush@raspberrypi.com> wrote:
>>
>> The reset function in staggered write was using the wrong index when
>> looking for the last updated camera parameters. This would cause
>> possibly stale exposure values to be written to the camera on a
>> mode switch for captures.
>>
>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>> ---
>>  src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
>> index 391e13f5..b26fa63d 100644
>> --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
>> @@ -46,7 +46,7 @@ void StaggeredCtrl::reset()
>>  {
>>         std::lock_guard<std::mutex> lock(lock_);
>>
>> -       int lastSetCount = std::max<int>(0, setCount_ - 1);
>> +       int lastSetCount = std::max<int>(0, setCount_);
>
>
> Actually I wonder whether we shouldn't just go with
>
>        uint32_t lastSetCount = setCount_;
>

Agreed, this is not needed any more.  New patch incoming.

Naush


> (the std::max seems a bit redundant now that we don't subtract 1).
>
>>
>>         std::unordered_map<uint32_t, int32_t> lastVal;
>>
>>         /* Reset the counters. */
>> --
>> 2.25.1
>>