[libcamera-devel] [PATCH] android: Fix improper file descriptor enumeration

[Laurent Pinchart]

Hi Nicholas,

(CC'ing Han-Lin and Harvey for the Chrome OS side, as well as Jacopo)

On Sun, Dec 18, 2022 at 02:00:02PM -0600, Nicholas Roth wrote:
> > I've replied to the bug report in
> > https://bugs.libcamera.org/show_bug.cgi?id=172#c5. I'd like to
> > investigate this further, as numFds != buf.numPlanes() doesn't sound
> > right.
> 
> My understanding from the differences between
> android/mm/generic_camera_buffer.cpp (Android) and
> android/mm/cros_camera_buffer.cpp (ChromeOS) is that camera3 on
> Android each handle buffers differently. See also the comment in
> generic_camera_buffer.cpp:
> ```
> /*
> * As Android doesn't offer an API to query buffer layouts, assume for
> * now that the buffer is backed by a single dmabuf, with planes being
> * stored contiguously.
> */
> ```

I didn't interpret this to mean that android would give us a buffer with
numFds == 1 even for multiplanar formats, but that all the file
descriptors would point to the same dmabuf instance. However, looking at
the gralloc implementations for Qualcomm platforms available at
https://android.googlesource.com/platform/hardware/qcom/display/, for
example the MSM8996 gralloc (https://android.googlesource.com/platform/hardware/qcom/display/+/refs/heads/master/msm8996/libgralloc/gralloc_priv.h#247),
the situation is more complicated. numFds is set to 2, with the first fd
storing the image buffer file descriptor (allocated from ION), and the
second fd storing a metadata buffer file descriptor (not entirely sure
what it's used for).

The only reasonable conclusion is that Android is a mess.

> I can't easily find "cros-camera/camera_buffer_manager.h" on the
> public internet so I can't comment about that here.

It's available at
https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/include/cros-camera/camera_buffer_manager.h.

> > This will break operation on Chrome OS.
>
> In that case, would you be open to the following?
> pseudocode:
> ```
> if (camera3buffer->numFds == buf.numPlanes()) {
>   // Act as if there's one buffer per plane (old behavior / ChromeOS)
> } else if (camera3buffer->numFds == 1) {
>   // Act as if planes are stored contiguously in a single dmabuf (Android)
> } else {
>   // Log Fatal: Unsupported state: number of FDs
> {camera3buffer->numFds} is not 1 (contiguous planes) and does not
> match number of planes {buf.numPlanes()}. Failed to infer buffer
> layout.
> }
> ```

This won't work on Qualcomm platforms, but that's likely something we
can ignore for now. I'm also worried about Android platforms that would
provide one dmabuf instance (and thus one fd) per plane, which is also
something that we could ignore for now I suppose.

Looking at CameraDevice::createFrameBuffer(), we create a CameraBuffer
instance, which abstracts platform-specific buffer management. I would
like to use that to provides the fds we need to create the FrameBuffer,
instead of implementing platform-specific logic in createFrameBuffer()
directly. We could either extend the class to provides the fds, or
possibly better, to create a FrameBuffer instance.

Any opinion ?

> Let me know!
> 
> Thanks,
> -Nicholas
> 
> P.S. I'm moving, and the initial plan is short-term rentals. I may not
> have access to my PC for awhile. I've tried to put the steps required
> to replicate my development environment in
> https://docs.google.com/document/d/1Sly_VH3w6wFIdE72WXijQegoHZh8IxEnJ9m0hH7GodU/edit#
> and will continue to be active here. I'm also trying to use an IP-KVM
> to maintain access to my desktop but can't guarantee that will work.

Thank you for putting those notes together and making them public. It's
useful.

-- 
Regards,

Laurent Pinchart