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

[Jacopo Mondi]

Hi Laurent, Nicholas

On Mon, Dec 19, 2022 at 03:38:05AM +0200, Laurent Pinchart via libcamera-devel wrote:
> 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 ?
>

I concur with the observation that CameraBuffer should be abstracting
platform-specific implementation details from the rest of the Camera
HAL implementation, possibily with something like
CameraBuffer::createFrameBuffer(const buffer_handle_t handle,..)

I'm now looking at the ChromeOS CameraBufferManager implementation
which abstracts away from us the need to set the right usages flags
when, in example, importing buffers:
https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/camera_buffer_manager_impl.cc#671

Standard Android platforms will not have the luxury of having a
generic buffer manager as ChromeOS does, and each implemenation, if my
understanding is correct, will need to encode in the Camera HAL
knowledge of the platform specificities (such as buffers layout) and
specific requirements of the gralloc implementation (usage flags).

We right now have a cros buffer manager backend and a generic one
which is, well, generic and can't be expected to work on all platforms
as it is. I presume the direction to take would be to specialize that
class to adapt it the platform the HAL is running on.

Let me use the platform Nicholas' working on as an example:

We know we could use [minigbm + rockchip kernel] for allocating NV12
buffers on the display. Let's assume the PinephonePro Waydroid build
is instrumented to use such component. Should we expect to have an
src/android/mm/rockchip_minigbm_camera_buffer.c implementation that
adapts to the requirements of the Rockchip's minigbm implementation ?


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