[EXT] Re: [PATCH] android: camera_device: Save capture settings unconditionally

[Hui Fang]

Hi, Jacopo

Sorry for inconvenience. I just updated patches in the previous email  "[PATCH] android: camera_device: The first valid settings should be saved".
Let's go back to this one.

Yesterday,  after I refined patch, I still used "git send-email", so miss generated this email.

BRs,
Fang Hui
________________________________
From: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Sent: Tuesday, February 27, 2024 3:34 PM
To: Hui Fang <hui.fang at nxp.com>
Cc: libcamera-devel at lists.libcamera.org <libcamera-devel at lists.libcamera.org>; biomifang118 at gmail.com <biomifang118 at gmail.com>
Subject: [EXT] Re: [PATCH] android: camera_device: Save capture settings unconditionally

Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button


Hi,

On Tue, Feb 27, 2024 at 10:13:20AM +0800, Fang Hui wrote:
> If not, it will be deferred to the next frame,

I proposed an extended commit message in the previous review. If
ignored on purpose, could you suggest me how it could be improved.
Just this one line is too short and not explicative imo

> as there's a bug assigned to this issue
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.libcamera.org%2Fshow_bug.cgi%3Fid%3D210&data=05%7C02%7Chui.fang%40nxp.com%7C8da8a20b2bf6442989e708dc37668466%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638446160678315587%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=m3YWD1Ckrhxw3Rq5sDYmToajjGx3L8fwhyjTvXP2WbQ%3D&reserved=0<https://bugs.libcamera.org/show_bug.cgi?id=210>

What I meant was to add:

Bug: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.libcamera.org%2Fshow_bug.cgi%3Fid%3D210&data=05%7C02%7Chui.fang%40nxp.com%7C8da8a20b2bf6442989e708dc37668466%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638446160678322482%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=HgKM6IVcRFCm5eHp0SPmJMBhUGQOkW8Foz3YAu8T2ns%3D&reserved=0<https://bugs.libcamera.org/show_bug.cgi?id=210>
>
> Signed-off-by: Fang Hui <hui.fang at nxp.com>

Before the Signed-off-by tag

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

I still think you should re-initialize lastSettings_ as I proposed in
the review of the previous version. Was it left out intentionally
because you think it's not needed ? Could you explain why ?

As I've said, with your ack, I could have made the changes I proposed
before applying the patch, but as you sent a new version I deduce you
disagree with the proposed changes ?

>
>       LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
>                       << " with " << descriptor->buffers_.size() << " streams";
> --
> 2.25.1
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240228/8e71d177/attachment.htm>