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

[Jacopo Mondi]

Hi Hiro,

On Mon, Aug 23, 2021 at 05:19:13PM +0900, Hirokazu Honda wrote:
> Hi Jacopo,
>
> On Fri, Aug 20, 2021 at 10:57 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi Hiro,
> >
> > On Fri, Aug 20, 2021 at 05:12:14AM +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 | 28 +++++++++++++++++++++++-----
> > >  1 file changed, 23 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index a69b687a..88cfd117 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -617,14 +617,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 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;
> > > +                     });
> > > +             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;
> > > +                     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;
> > >       }
> > >
> > >       /* Now handle the MJPEG streams, adding a new stream if required. */
> > > @@ -1077,6 +1095,7 @@ void CameraDevice::requestComplete(Request *request)
> > >       camera3_capture_result_t captureResult = {};
> > >       captureResult.frame_number = descriptor.frameNumber_;
> > >       captureResult.num_output_buffers = descriptor.buffers_.size();
> > > +     /* Handle post-processing. */
> >
> > Is this the right place where to move this comment should this replace
>
> Sorry, I don't get your suggestion. Can you rephrase?

I would have replaced

-     /* Handle any JPEG compression. */
+     /* Handle post-processing. */

with
>
> Regards,
> -Hiro
> > -       /* Handle any JPEG compression. */
> >
> > >       for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> > >               buffer.acquire_fence = -1;
> > >               buffer.release_fence = -1;
> > > @@ -1132,12 +1151,11 @@ void CameraDevice::requestComplete(Request *request)
> > >               resultMetadata = std::make_unique<CameraMetadata>(0, 0);
> > >       }
> > >
> > > -     /* Handle any JPEG compression. */
> > >       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)
> > >                       continue;
> >
> > Excellent. Minor nit apart
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > Thanks
> >    j
> >
> > >
> > >               FrameBuffer *src = request->findBuffer(cameraStream->stream());
> > > --
> > > 2.33.0.rc2.250.ged5fa647cd-goog
> > >