| Message ID | 20210805134530.825065-4-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? 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 ? > + 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 ? > } > > /* 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) > 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 >
Hi Jacopo, thank you for reviewing. On Wed, Aug 18, 2021 at 8:20 PM Jacopo Mondi <jacopo@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@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 > >
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@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@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 > > >
HI Jacopo, thank you for reply. On Fri, Aug 20, 2021 at 10:53 PM Jacopo Mondi <jacopo@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@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@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 > > > >
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. + */ + 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. */ @@ -1095,12 +1113,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; 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_; } 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; - 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) {
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@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(-)