[libcamera-devel] [PATCH v2 03/12] android: camera_device: Add debug to stream initialization

[Hirokazu Honda]

Hi,
LGTM module a few nits.

Re patch title, how about "Add more debug log in stream initialization process"?

On Sun, Sep 6, 2020 at 2:55 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Sep 02, 2020 at 05:22:27PM +0200, Jacopo Mondi wrote:
> > Add debug printouts to the CameraDevice::initializeStreamConfigurations()
> > function that helps to follow the process of building the stream
>
> s/helps/help/
>
> > configurations map.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 8a8072123961..493d6cecde72 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -363,6 +363,9 @@ int CameraDevice::initializeStreamConfigurations()
> >               const std::vector<PixelFormat> &libcameraFormats =
> >                       camera3Format.libcameraFormats;
> >
> > +             LOG(HAL, Debug) << "Testing Android format: "
> > +                             << camera3Format.name;
>
> s/://
>
> Maybe s/Testing/Mapping/ ? Not sure this is a test.
>

How about "Trying to map"?

> I think I'd move this below the JPEG case, to avoid two debug messages
> for JPEG.
>

I don't have a strong preference here, but I wonder if not moving is better?
Therefore, we always pair debug log, Testing... - Mapped...

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>

> > +
> >               /*
> >                * Fixed format mapping for JPEG.
> >                *
> > @@ -375,6 +378,10 @@ int CameraDevice::initializeStreamConfigurations()
> >                */
> >               if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> >                       formatsMap_[androidFormat] = formats::MJPEG;
> > +                     LOG(HAL, Debug) << "Mapped Android format: "
>
> s/://
>
> > +                                     << camera3Format.name << " to: "
>
> s/://
>
> Same in other messages below.
>
> > +                                     << formats::MJPEG.toString()
> > +                                     << " (fixed mapping)";
>
> As this is always done, is there value in printing a message here ?
>
> >                       continue;
> >               }
> >
> > @@ -385,6 +392,10 @@ int CameraDevice::initializeStreamConfigurations()
> >               PixelFormat mappedFormat;
> >               for (const PixelFormat &pixelFormat : libcameraFormats) {
> >
> > +                     LOG(HAL, Debug) << "Testing Android format: "
> > +                                     << camera3Format.name << " with: "
> > +                                     << pixelFormat.toString();
> > +
> >                       /*
> >                        * The stream configuration size can be adjusted,
> >                        * not the pixel format.
> > @@ -420,14 +431,27 @@ int CameraDevice::initializeStreamConfigurations()
> >                * stream configurations map, by testing the image resolutions.
> >                */
> >               formatsMap_[androidFormat] = mappedFormat;
> > +             LOG(HAL, Debug) << "Mapped Android format: "
> > +                             << camera3Format.name << " to: "
> > +                             << mappedFormat.toString();
> >
> >               for (const Size &res : cameraResolutions) {
> >                       cfg.pixelFormat = mappedFormat;
> >                       cfg.size = res;
> >
> > +                     std::stringstream ss;
> > +                     ss << "Testing (" << res.toString() << ")["
> > +                        << mappedFormat.toString() << "]: ";
>
> Maybe use cfg.toString() ?
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +
> >                       CameraConfiguration::Status status = cameraConfig->validate();
> > -                     if (status != CameraConfiguration::Valid)
> > +                     if (status != CameraConfiguration::Valid) {
> > +                             ss << " not supported";
> > +                             LOG(HAL, Debug) << ss.str();
> >                               continue;
> > +                     }
> > +
> > +                     ss << " supported";
> > +                     LOG(HAL, Debug) << ss.str();
> >
> >                       streamConfigurations_.push_back({ res, androidFormat });
> >
>
> --
> Regards,
>
> Laurent Pinchart

Best Regards,
-Hiro

> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel