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

[Laurent Pinchart]

Hi Kieran,

On Wed, Jul 29, 2020 at 03:37:16PM +0100, Kieran Bingham wrote:
> On 29/07/2020 15:27, Laurent Pinchart wrote:
> > On Wed, Jul 29, 2020 at 03:19:24PM +0100, Kieran Bingham wrote:
> >> On 24/07/2020 18:24, Laurent Pinchart wrote:
> >>> 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).
> > 
> > I may also have been confused. I think I was referring to the idea of
> > creating a base class that can be inherited from, that part is in the
> > public API. Making it private should be fine, but exposing that design
> > externally would I think require a bit more work first.
> > 
> >>> 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.
> > 
> > We really want to avoid unnecessary dup() calls as much as possible, as
> > that's expensive. Do you think it would make sense to extend
> > FileDescriptor to allow borrowing fds, or is that too complicated, or
> > too hackish ?
> 
> I don't think it's too hackish necessarily, but I don't know how that
> could be expressed.
> 
> And indeed it could make things complicated for lifetimes as suddenly
> what happens if the other owner closes them etc.. (ok so you could
> document the requirements for borrowing etc)...
> 
> I presume we'd have to have some sort of a flag to determine the fd was
> borrowed and prevent the associated closes on destructor ... it feels a
> bit awkward ... :S

Yes, that's what would be needed, and it would be a bit awkward indeed.

> But I have two paths for 'now':
> 
> Make a MappedBuffer generic base that can map from a buffer_t
> 
> or
> 
> Map all buffer_t's to a FrameBuffer, then make a MappedFrameBuffer from
> that ...
> 
> Do you have a particular preference on either?
> 
> If I construct a FrameBuffer for each buffer_t, we can always look to
> improve performance later...

As we agreed it will be an internal API for now, I would avoid dup()s
already, at the expense of a future refactoring of the API.

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