[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: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'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