The first valid settings should be saved

19520 diff mbox series

The first valid settings should be saved, or will be deferred to the next frame.



BRs,

FangHui

Hi Hui,

Thank you for the patch.

On Wed, Feb 21, 2024 at 01:38:00PM +0800, hui fang wrote:
> The first valid settings should be saved, or will be deferred to the next frame.

This is missing a Signed-off-by tag, and the subject is missing a
prefix. Both issues should have been caught by the checkstyle.py
utility part of libcamera.

Please see https://libcamera.org/contributing.html#submitting-patches
for more information about Signed-off-by, and
https://libcamera.org/coding-style.html#tools for more information about
the coding style and checkstyle.py utility.

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

commit 4c220580fc2c71a978dc768619b6e65f3a5aea4a (HEAD -> master)
Author: Fang Hui <hui.fang@nxp.com>
Date:   Thu Jan 25 02:11:04 2024 +0800

    android: camera_device: The first valid settings should be saved

    If not, it will be deferred to the next frame.

    Signed-off-by: Fang Hui <hui.fang@nxp.com>

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


BRs,
Fang Hui

Laurent Pinchart <laurent.pinchart@ideasonboard.com> 于2024年2月21日周三 21:13写道：

> Hi Hui,
>
> Thank you for the patch.
>
> On Wed, Feb 21, 2024 at 01:38:00PM +0800, hui fang wrote:
> > The first valid settings should be saved, or will be deferred to the
> next frame.
>
> This is missing a Signed-off-by tag, and the subject is missing a
> prefix. Both issues should have been caught by the checkstyle.py
> utility part of libcamera.
>
> Please see https://libcamera.org/contributing.html#submitting-patches
> for more information about Signed-off-by, and
> https://libcamera.org/coding-style.html#tools for more information about
> the coding style and checkstyle.py utility.
>
> > 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_;
>
> --
> Regards,
>
> Laurent Pinchart
>

Hello Fang Hui
   thanks for your patch

The patch does not seem to be in a valid format, it is being
copied into an email ? Could you have a read to https://git-send-email.io/
and try it ? :)

On Fri, Feb 23, 2024 at 11:26:29AM +0800, hui fang wrote:
> commit 4c220580fc2c71a978dc768619b6e65f3a5aea4a (HEAD -> master)
> Author: Fang Hui <hui.fang@nxp.com>
> Date:   Thu Jan 25 02:11:04 2024 +0800
>
>     android: camera_device: The first valid settings should be saved
>
>     If not, it will be deferred to the next frame.

On the patch itself, if I get the issue right, the problem is that the
very first frame won't have descriptor->settings_ initialized and you
patch indeed fixes this. How did you detect the problem, did you get run-time
errors or was that just from looking at the code (I'm asking to know
what test I could run to reproduce the issue).

Thanks
  j

>
>     Signed-off-by: Fang Hui <hui.fang@nxp.com>
>
> 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";
>
>
> BRs,
> Fang Hui
>
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> 于2024年2月21日周三 21:13写道：
>
> > Hi Hui,
> >
> > Thank you for the patch.
> >
> > On Wed, Feb 21, 2024 at 01:38:00PM +0800, hui fang wrote:
> > > The first valid settings should be saved, or will be deferred to the
> > next frame.
> >
> > This is missing a Signed-off-by tag, and the subject is missing a
> > prefix. Both issues should have been caught by the checkstyle.py
> > utility part of libcamera.
> >
> > Please see https://libcamera.org/contributing.html#submitting-patches
> > for more information about Signed-off-by, and
> > https://libcamera.org/coding-style.html#tools for more information about
> > the coding style and checkstyle.py utility.
> >
> > > 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_;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >