[libcamera-devel] [PATCH v2 04/12] android: camera_device: Break out size calculation

[Hirokazu Honda]

On Sun, Sep 6, 2020 at 3:00 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Sep 02, 2020 at 05:22:28PM +0200, Jacopo Mondi wrote:
> > As the RAW stream sizes needs to be calculated differently from the
> > processed one, break out break out the procedure to calculate the
>
> s/break out break out/break out/
>
> > processed (RGB/YUV) resolutions from initializeStreamConfigurations() in
> > order to prepare for RAW sizes calculation.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 55 ++++++++++++++++++++++++-----------
> >  src/android/camera_device.h   |  5 ++++
> >  2 files changed, 43 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 493d6cecde72..58b2ad27c5e2 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -283,6 +283,37 @@ int CameraDevice::initialize()
> >       return ret;
> >  }
> >
> > +std::vector<Size> CameraDevice::filterYUVResolutions(CameraConfiguration *cameraConfig,
> > +                                                  const PixelFormat &pixelFormat,
> > +                                                  const std::vector<Size> &resolutions)

Pass StreamConfiguration* rather than CameraConfiguration?

> > +{
> > +     std::vector<Size> supportedResolutions;
> > +
> > +     StreamConfiguration &cfg = cameraConfig->at(0);
> > +     for (const Size &res : resolutions) {
> > +             cfg.pixelFormat = pixelFormat;
> > +             cfg.size = res;
> > +
> > +             std::stringstream ss;
> > +             ss << "Tested (" << res.toString() << ")["
>
> Testing became Tested ? :-)
>
> > +                << pixelFormat.toString() << "]: ";
> > +
> > +             CameraConfiguration::Status status = cameraConfig->validate();
> > +             if (status != CameraConfiguration::Valid) {
> > +                     ss << " not supported";
> > +                     LOG(HAL, Debug) << ss.str();
> > +                     continue;
> > +             }
> > +
> > +             ss << " supported";
> > +             LOG(HAL, Debug) << ss.str();
> > +
> > +             supportedResolutions.push_back(res);
> > +     }
> > +
> > +     return supportedResolutions;
> > +}
> > +
> >  /*
> >   * Initialize the format conversion map to translate from Android format
> >   * identifier to libcamera pixel formats and fill in the list of supported
> > @@ -435,24 +466,14 @@ int CameraDevice::initializeStreamConfigurations()
> >                               << 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() << "]: ";
> > -
> > -                     CameraConfiguration::Status status = cameraConfig->validate();
> > -                     if (status != CameraConfiguration::Valid) {
> > -                             ss << " not supported";
> > -                             LOG(HAL, Debug) << ss.str();
> > -                             continue;
> > -                     }
> > -
> > -                     ss << " supported";
> > -                     LOG(HAL, Debug) << ss.str();
> > +             const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat);
> > +             if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > +                     continue;
>
> I'd move this to patch 05/12, as it introduces a change not described in
> the commit message (and breaks bisection unless I'm mistaken).
>

I would also add a comment description here about why we breakout in the case.

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> >
> > +             std::vector<Size> resolutions = filterYUVResolutions(cameraConfig.get(),
> > +                                                                  mappedFormat,
> > +                                                                  cameraResolutions);
> > +             for (const Size &res : resolutions) {
> >                       streamConfigurations_.push_back({ res, androidFormat });
> >
> >                       /*
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 3934f194f1b5..359a163ebab9 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -93,6 +93,11 @@ private:
> >       };
> >
> >       int initializeStreamConfigurations();
> > +     std::vector<libcamera::Size>
> > +     filterYUVResolutions(libcamera::CameraConfiguration *cameraConfig,
> > +                          const libcamera::PixelFormat &pixelFormat,
> > +                          const std::vector<libcamera::Size> &resolutions);
>
> You can make this a static function.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +
> >       std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
> >       libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> >       void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel