[libcamera-devel] [RFC PATCH 09/10] android: camera_device: Fill offset and right length in CreateFrameBuffer()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 18 03:51:54 CEST 2021
Hi Hiro,
Thank you for the patch.
On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote:
> CameraDevice::CreateFrameBuffer() fills the length of the buffer to
> each FrameBuffer::Plane::length. It should rather be the length of
> plane. This also changes CreateFrameBuffer() to fill offset of
> FrameBuffer::Plane.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
> src/android/camera_device.cpp | 38 ++++++++++++++++++-----------------
> 1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a69b687a..1ded5cb1 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -12,6 +12,7 @@
>
> #include <algorithm>
> #include <fstream>
> +#include <sys/mman.h>
> #include <unistd.h>
> #include <vector>
>
> @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>
> FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer)
> {
> - std::vector<FrameBuffer::Plane> planes;
> + FileDescriptor fd;
> + /* This assumes all the planes are in the same buffer. */
Should we ASSERT() if that's not the case ? Also, does this assumption
always hold ?
> for (int i = 0; i < camera3buffer->numFds; i++) {
> - /* Skip unused planes. */
> - if (camera3buffer->data[i] == -1)
> + if (camera3buffer->data[i] != -1) {
> + fd = FileDescriptor(camera3buffer->data[i]);
> break;
> -
> - FrameBuffer::Plane plane;
> - plane.fd = FileDescriptor(camera3buffer->data[i]);
> - if (!plane.fd.isValid()) {
> - LOG(HAL, Error) << "Failed to obtain FileDescriptor ("
> - << camera3buffer->data[i] << ") "
> - << " on plane " << i;
> - return nullptr;
> }
> + }
Blank line.
> + if (!fd.isValid()) {
> + LOG(HAL, Fatal) << "No valid fd";
> + return nullptr;
> + }
>
> - off_t length = lseek(plane.fd.fd(), 0, SEEK_END);
> - if (length == -1) {
> - LOG(HAL, Error) << "Failed to query plane length";
> - return nullptr;
> - }
> + CameraBuffer buf(camera3buffer, PROT_READ);
I suppose there's no other way than using the cros::CameraBufferManager
to get the necessary information. I'm annoyed that this involves mapping
the buffer to the CPU just to retrieve sizes, as that's an expensive
operation :-( It's of course not a prerequisite for this series, but I'm
wondering if the cros::CameraBufferManager API could be improved to
separate retrieving plane information from mapping the buffer. What do
you think ?
Also, should generic_camera_buffer.cpp be updated to provide the correct
information ?
> + if (!buf.isValid()) {
> + LOG(HAL, Fatal) << "Failed mapping buffer";
> + return nullptr;
> + }
>
> - plane.length = length;
> - planes.push_back(std::move(plane));
> + std::vector<FrameBuffer::Plane> planes(buf.numPlanes());
> + for (size_t i = 0; i < buf.numPlanes(); ++i) {
> + planes[i].fd = fd;
> + planes[i].offset = buf.plane(i).data() - buf.plane(0).data();
> + planes[i].length = buf.plane(i).size();
> }
>
> return new FrameBuffer(std::move(planes));
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list