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

[Jacopo Mondi]

Hi Hiro,

On Mon, Sep 07, 2020 at 05:32:44PM +0900, Hirokazu Honda wrote:
> 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?
>

But I need to call ....
> > > +{
> > > +     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();


.... validate here on the camera configuration :/

> > > +             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.

We would break bisection as this subtly introduces a behavioural change as
no RAW resolution is reported anymore. It's not a code breakages but
more a process one, so I don't think it's worth a comment if I got
your comment right. And I'll try to make sure this does not happen in
the next version of the patch.

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

Thanks
  j

> > >
> > > +             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