[libcamera-devel] [PATCH 3/3] android: camera_device: Configure one stream for identical stream requests

[Hirokazu Honda]

Hi Jacopo, thank you for reviewing.

On Wed, Aug 18, 2021 at 8:20 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Hiro,
>
> On Thu, Aug 05, 2021 at 10:45:30PM +0900, Hirokazu Honda wrote:
> > An Android HAL client may request identical stream requests. It is
> > redundant that a native camera device produces a separate stream for
> > each of the identical requests.
> > Configure camera one stream configuration for the identical stream
> > requests.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/android/camera_device.cpp | 27 ++++++++++++++++++++++-----
> >  src/android/camera_device.h   |  3 +--
> >  src/android/camera_stream.cpp |  3 ++-
> >  3 files changed, 25 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 2466122d..68b2840c 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -618,14 +618,32 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >                       continue;
> >               }
> >
> > +             /* This stream will be produced by hardware. */
> > +             stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
> > +
> > +             /*
> > +              * If they are the same capture request, associate with the same
> > +              * CameraStream.
>
>                 /*
>                  * If a CameraStream with the same size and format of
>                  * the current stream has already been requested
>                  * associate the two.
>                  */
>
> > +              */
> > +             auto iter = std::find_if(
> > +                     streamConfigs.begin(), streamConfigs.end(),
> > +                     [size, format](const Camera3StreamConfig &streamConfig) {
> > +                             return streamConfig.config.size == size &&
> > +                                    streamConfig.config.pixelFormat == format;
> > +                     });
>
> So this creates a mapped stream only if both the format AND the size
> are identical. Shouldn't we use the YUV post-processor to downscale
> NV12-to-NV12 as well ?

Right but using YUV-post processor for down-scaling is not a goal of
this patch series.
I would not do it in this patch series.

> Even if not part of this patch, the logic here
> implemented won't work, unless the camera3_streams are presented to
> the libcamera HAL ordered by size, doens't it ?

I don't understand why this won't work unless that.
Could you explain me more?

>
> > +             if (iter != streamConfigs.end()) {
> > +                     /* Add usage to copy the buffer in streams[0] to stream. */
> > +                     iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;
> > +                     stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;
>
> Should the GRALLOC_USAGE_HW_CAMERA_WRITE be kept or should it be
> overridden ?
>
>
> > +                     iter->streams.push_back({ stream, CameraStream::Type::Mapped });
> > +                     continue;
> > +             }
> > +
> >               Camera3StreamConfig streamConfig;
> >               streamConfig.streams = { { stream, CameraStream::Type::Direct } };
> >               streamConfig.config.size = size;
> >               streamConfig.config.pixelFormat = format;
> >               streamConfigs.push_back(std::move(streamConfig));
> > -
> > -             /* This stream will be produced by hardware. */
> > -             stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
>
> Depending on the answer to the previous question, should this be
> moved ?
>

If I drop this usage, a buffer fails to be mapped because
cros::CameraBufferManager requires the usage upon gbm_bo_import().
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=667;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e
I think the usage is mandatory.

> >       }
> >
> >       /* Now handle the MJPEG streams, adding a new stream if required. */
> > @@ -1095,12 +1113,11 @@ void CameraDevice::requestComplete(Request *request)
> >               resultMetadata = std::make_unique<CameraMetadata>(0, 0);
> >       }
> >
> > -     /* Handle any JPEG compression. */
>
> I would keep the comment as something like
>
>         /* Handle post-processing. */
>
> >       for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> >               CameraStream *cameraStream =
> >                       static_cast<CameraStream *>(buffer.stream->priv);
> >
> > -             if (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)
> > +             if (cameraStream->type() == CameraStream::Type::Direct)
>
> This will accept Type::Internal. Is this intended ? Shouldn't this be
>                 if (cameraStream->type() != CameraStream::Type::Mapped)
>

JPEG processing case is Internal. We need to handle Mapped and Internal.


Best Regards,
-Hiro


> >                       continue;
> >
> >               FrameBuffer *src = request->findBuffer(cameraStream->stream());
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index cbc71be4..c5f96d32 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -48,6 +48,7 @@ public:
> >
> >       unsigned int id() const { return id_; }
> >       camera3_device_t *camera3Device() { return &camera3Device_; }
> > +     const CameraCapabilities *capabilties() const { return &capabilities_; }
>
> Should be moved to 1/3
>
> >       const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; }
> >
> >       const std::string &maker() const { return maker_; }
> > @@ -63,8 +64,6 @@ public:
> >       int processCaptureRequest(camera3_capture_request_t *request);
> >       void requestComplete(libcamera::Request *request);
> >
> > -     libcamera::PixelFormat toPixelFormat(int format) const;
> > -
>
> And this should be dropped from 1/3
>
> >  protected:
> >       std::string logPrefix() const override;
> >
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 8c02cb43..d880cc18 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -8,6 +8,7 @@
> >  #include "camera_stream.h"
> >
> >  #include "camera_buffer.h"
> > +#include "camera_capabilities.h"
> >  #include "camera_device.h"
> >  #include "camera_metadata.h"
> >  #include "jpeg/post_processor_jpeg.h"
> > @@ -62,7 +63,7 @@ int CameraStream::configure()
> >  {
> >       if (type_ == Type::Internal || type_ == Type::Mapped) {
> >               const PixelFormat outFormat =
> > -                     cameraDevice_->toPixelFormat(camera3Stream_->format);
> > +                     cameraDevice_->capabilties()->toPixelFormat(camera3Stream_->format);
> >               StreamConfiguration output = configuration();
> >               output.pixelFormat = outFormat;
> >               switch (outFormat) {
>
> Overall a very nice step in the right direction!
>
> Thanks
>    j
>
> > --
> > 2.32.0.554.ge1b32706d8-goog
> >