[libcamera-devel] [PATCH 3/3] android: camera_device: Configure one stream for identical stream requests
Jacopo Mondi
jacopo at jmondi.org
Fri Aug 20 15:54:14 CEST 2021
Hi Hiro,
On Fri, Aug 20, 2021 at 05:10:03AM +0900, Hirokazu Honda wrote:
> 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.
That's fine! I was not asking for this, but it was related to my
question below.
>
> > 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 the streams requested by the HAL client are not ordered from the
bigger to the smaller it won't be possible to here identify what
stream can be generated by downscaling another one.
In other words: if we sort streams by size, will it be possible here
to get YUV downscaling by simply searching for a YUV stream which is
bigger in size and map the current on to it ?
> >
> > > + 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.
Fine with me, I was asking to check if this was intentional or not.
>
> > > }
> > >
> > > /* 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.
You're right, it can be internal and this is the same check which is
now performed in CameraStream::configure().
>
>
> 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
> > >
More information about the libcamera-devel
mailing list