[libcamera-devel] [RFC PATCH 0/2] android: Fix the issue with Type::Mapped only request

[Jacopo Mondi]

Hi Hiro,

On Wed, Oct 13, 2021 at 01:46:50PM +0200, Jacopo Mondi wrote:
> Hi Hiro,
>
> On Mon, Sep 27, 2021 at 07:48:19PM +0900, Hirokazu Honda wrote:
> > We have the problem that no buffer is not provided to a native camera
> > if a request given in CameraDevice::processCaptureRequest() contains
> > only streams that are resolved as CameraStream::Type::Mapped in
> > CameraDevice::configureStreams(). See the example in below.
> >
> > ```
> > [0:27:06.863311142] [15203] DEBUG HAL camera_device.cpp:980
> > '\_SB_.PCI0.I2C2.CAM0': Queueing request 139090704005584 with 2
> > streams
> > [0:27:06.863389502] [15203] DEBUG HAL camera_device.cpp:1005
> > '\_SB_.PCI0.I2C2.CAM0': 0 - (320x240)[0x00000022] -> (320x240)[NV12]
> > (mapped)
> > [0:27:06.863436331] [15203] DEBUG HAL camera_device.cpp:1005
> > '\_SB_.PCI0.I2C2.CAM0': 1 - (320x240)[0x00000021] -> (320x240)[NV12]
> > (mapped)
> > [0:27:06.863487886] [15203] DEBUG HAL camera_device.cpp:1043
> > '\_SB_.PCI0.I2C2.CAM0': buffers=0
> > [0:27:06.863561912] [15218] ERROR Camera camera.cpp:1031 Request
> > contains no buffers
> > ```
> >
> > This patch series fixes the issue by looking up the stream
> > alternatively sent to a native camera. To do that, we have to
> > allocate a FrameBuffer for the stream. One possible way is to use
> > FrameBufferAllocator. However, FrameBufferAllocator::allocate must
> > be called when the state of Camera is Configured, but not Running.
> > So we have to allocate buffers during configureStreams(). But
> > allocating buffers for CameraStrems whose type is Direct wastes
> > memory as they tends to not be used at all.
> >
> > I would rather have a new class that allocates FrameBuffers in a
> > platform-dependent way, cros::CameraBufferManager on ChromeOS and
> > gralloc API on other platforms.
>
> Here is the reply to my previous question.
>
> And I actually think the contrary: this series fixes a bug it
> shouldn't introduce new features. I would make sure the minimum amount
> of changes to get there, instead of piling more stuff on top that
> requires more review, more test, more verifications and could
> potentially introduce new bugs.
>

What's your take on this ?

A possible way forward could be to separate the upstreaming of the
platform allocator, which seems in quite good shape already. It can be
used to replace the usage of FrameBufferAllocator in CameraStream and
verified with the current code base.

Then on top we can enable the correct use of the YUV post-processor
with your 2/2.

What do you think ?

Thanks
   j

> >
> > With this patch, android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces passed.
>
> Please add the full CTS results to the cover letters of series that
> touch the HAL in non trivial ways.
>
> I think we'll make this a requirement going forward.
>
> >
> > Hirokazu Honda (2):
> >   android: Introduce PlatformFrameBufferAllocator
> >   android: Send alternative stream configuration if no buffer to be sent
> >     exists
> >
> >  src/android/camera_device.cpp           |  50 ++++++++--
> >  src/android/camera_device.h             |   4 +
> >  src/android/camera_stream.cpp           |  37 +++++++-
> >  src/android/camera_stream.h             |  10 +-
> >  src/android/frame_buffer.h              |  53 +++++++++++
> >  src/android/mm/cros_frame_buffer.cpp    |  85 +++++++++++++++++
> >  src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++
> >  src/android/mm/meson.build              |   6 +-
> >  8 files changed, 344 insertions(+), 17 deletions(-)
> >  create mode 100644 src/android/frame_buffer.h
> >  create mode 100644 src/android/mm/cros_frame_buffer.cpp
> >  create mode 100644 src/android/mm/generic_frame_buffer.cpp
> >
> > --
> > 2.33.0.685.g46640cef36-goog