[PATCH] android: camera_device: The first valid settings should be saved

[Umang Jain]

Hi Jacopo,

Thank you for the discussion,

On 26/02/24 7:51 pm, Jacopo Mondi wrote:
> Hi Fang Hui
>
> In subject:
>
> android: camera_device: Save capture settings unconditionally
>
> On Mon, Feb 26, 2024 at 11:08:12AM +0800, Fang Hui wrote:
>> If not, it will be deferred to the next frame.
> This commit message is really too short and does not explain why this
> is an issue. With your ack I would change it to:
>
> ------------------------------------------------------------------------------
> As the Android framework sends to the camera device settings
> incrementally (only the ones that change are updated), the CameraDevice
> class in the Android camera HAL keeps a copy of the last received
> settings to be able to apply controls to the libcamera Camera and to
> populate metadata correctly.
>
> When a valid 'camera3Request->settings' is provided, it gets saved to
> 'lastSettings_' but 'descriptor->settings_' is not initialized until
> the next frame (assuming it does not contain more settings).
>
> Fix this by assigning to 'descriptor->settings_' the last saved
> settings unconditionally.
> ------------------------------------------------------------------------------
>
> And now that I wrote the last sentence, I realized lastSettings_ is
> not re-initialized between streaming sessions.
>
> You will likely need:
>
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1108,6 +1108,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>          }
>
>          if (state_ == State::Stopped) {
> +               lastSettings_ = {};
> +
>                  ret = camera_->start();
>                  if (ret) {
>                          LOG(HAL, Error) << "Failed to start camera";
>
> Which I can again add when applying with your ack.

With the original patch hunk below and lastSettings_= {}; and commit 
message fixed,

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

>
> Thanks
>     j
>
>> Signed-off-by: Fang Hui <hui.fang at nxp.com>
>> ---
>>   src/android/camera_device.cpp | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 25cedd44..d45ed1a5 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -952,8 +952,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>   	 */
>>   	if (camera3Request->settings)
>>   		lastSettings_ = camera3Request->settings;
>> -	else
>> -		descriptor->settings_ = lastSettings_;
>> +
>> +	descriptor->settings_ = lastSettings_;
>>
>>   	LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
>>   			<< " with " << descriptor->buffers_.size() << " streams";
>> --
>> 2.25.1
>>