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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Dec 19, 2022 at 10:15:50AM +0100, Jacopo Mondi wrote:
> 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).

Until Android sort out their mess, I think that's right. I won't hold my
breath, one of the big design principles of the operating system is that
platform-specific components are meant to be developed together, and
share common knowledge through hardcoding assumptions, without any
standard API to communicate them. I doubt they will change that anytime
soon.

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

I believe so, yes. We may not have to specialize it for every single
Android platform though, there may be some platform-specific data that
we could also obtain from the HAL configuration file.

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

It depends what those requirements would be. One thing we'll likely have
to specialize CameraBuffer for is the usage of the fds in the
native_handle_t structure. If multiple platform use the same layout,
possibly with other platform-specific differences that could be encoded
in a configuration file (such as, for example, which pixel format
IMPLEMENTATION_DEFINED is mapped to, assuming it doesn't depend too much
on the usage flags), then a single CameraBuffer implementation could be
used for all those platforms.

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