[libcamera-devel] [RFC PATCH 8/8] RFC-Only: android: camera_device: Provide a MappedCamera3Buffer

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Mon, Jul 20, 2020 at 11:42:32PM +0100, Kieran Bingham wrote:
> Utilise the MappedBuffer interface to map each of the planes provided
> in the Camera3 buffer to facilitate use in software streams.
> 
> The buffers will be automatically unmapped when the object goes out of
> scope or is deleted.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> 
> This patch shows an addition of a new MappedBuffer type constructed from
> a camera3buffer. The base class deals with the move-constructors and
> destructor, and gives us a common interface to pass a set of mapped
> dmabufs around.
> 
> I had hoped to use this to pass in the camera3buffer for writing jpeg
> buffers to, giving the same interface for both the source and
> destination buffer - but for JPEG, I do also need to return the number
> of bytes actually consumed, so this ended up potentially not adding the
> benefits I hoped for.
> 
> Still, it might still provide some benefits, so I've included it here as
> something to talk about.

This, along with patch 4/8, is the only part of the series I'm not sure
to like :-S It feels quite like a ad-hoc solution, which is probably
normal, as that's what it is :-) It may not be that bad, but doesn't
qualify for the public API in my opinion. We could keep this as-is for
now as an internal helper, until we figure something better (if there's
ever a need to do so).

A random idea, would it make sense to convert buffer_handle_t to
FrameBuffer as early as possible in the HAL and operate on FrameBuffer ?
There's at least one drawback in that it would dup() the fds, which may
call for a rework of the FileDescriptor class to make borrowing fds
possible. Or maybe it's just a bad idea.

I'd like to see how this ends up being used for the JPEG encoding to get
a better feeling of the API. I thus can't guarantee at this point I'll
ack the idea as-is, but I think it's worth continuing the experiment, I
feel there's something good here.

>  src/android/camera_device.cpp | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 6212ccdd61ec..f78486117e9f 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -86,6 +86,39 @@ const std::map<int, const Camera3Format> camera3FormatsMap = {
>  
>  LOG_DECLARE_CATEGORY(HAL);
>  
> +class MappedCamera3Buffer : public MappedBuffer {
> +public:
> +	MappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags)
> +	{
> +		maps_.reserve(camera3buffer->numFds);
> +		error_ = 0;
> +
> +		for (int i = 0; i < camera3buffer->numFds; i++) {
> +			if (camera3buffer->data[i] == -1)
> +				continue;
> +
> +			off_t length = lseek(camera3buffer->data[i], 0, SEEK_END);
> +			if (length < 0) {
> +				error_  = errno;
> +				LOG(HAL, Error) << "Failed to query plane length";
> +				break;
> +			}
> +
> +			void *address = mmap(nullptr, length, flags, MAP_SHARED,
> +					camera3buffer->data[i], 0);
> +			if (address == MAP_FAILED) {
> +				error_ = errno;
> +				LOG(HAL, Error) << "Failed to mmap plane";
> +				break;
> +			}
> +
> +			maps_.push_back({address, static_cast<size_t>(length)});
> +		}
> +
> +		valid_ = error_ == 0;
> +	}
> +};
> +
>  /*
>   * \struct Camera3RequestDescriptor
>   *

-- 
Regards,

Laurent Pinchart