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

[Kieran Bingham]

Hi Laurent,

On 24/07/2020 18:24, Laurent Pinchart wrote:
> 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).


This *isn't* public api - this is under src/android/camera_device.

Perhaps you mean the MappedBuffer API shouldn't be public (Though even
if the base is still internal, the Android layer can still use this).


> 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 thnik it was the unnecessary dup's required to construct a FrameBuffer
that stopped me, but perhaps it might make some sense still.



> 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.


The idea was to make sure that the Mapped buffer object was the same
regardless of whether it came from a FrameBuffer, or from a buffer_handle_t.

Of course mapping the MappedFrameBuffer(FrameBuffer(buffer_handle_t)) is
a solution to that, it just wastes/duplicates more fds.

See:

[RFC PATCH 6/6] android: camera_device: Provide a MappedCamera3Buffer

(Ugh ,sorry same patch name, this was split from there)

for how this got used to remove the aribtrary mapAndroidBlobBuffer() and
unmapAndroidBlobBuffer() fucntions that I had created earlier on.

(Kept separate for exactly these discussions).

I think for now, I'm going to end up mapping a FrameBuffer from every
buffer_handle_t() for consistency instead.

--
Kieran


>>  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
--
Kieran