[libcamera-devel] [PATCH 3/3] android: camera_device: Configure one stream for identical stream requests
Hirokazu Honda
hiroh at chromium.org
Mon Aug 23 09:50:33 CEST 2021
HI Jacopo, thank you for reply.
On Fri, Aug 20, 2021 at 10:53 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> 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 ?
>
Note that sorting is done in sortCamera3StreamConfigs().
Searching for a stream to be down-scaled should not be done here.
It is because a native camera may be able to produce the multiple streams.
This search for the best configured streams is achievable with a
reprocessing configuration, which will be done by Laurent.
It is questionable whether squashing the streams should also be done
in the reprocessing process.
I asked it in IRC, and I was answered that it should be done
beforehand regardless of the reprocessing configuration.
+Laurent Pinchart what do you think?
Best Regards,
-Hiro
> > >
> > > > + 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