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

[Hirokazu Honda]

Hi Jacopo,

The way has a problem about the number of allocated buffers, here.
> + LOG(HAL, Error) << "getBuffer@@@@@ " << numBuffers;
> + // numBuffers is 4 (=config.bufferCount) is not enough sadly...
> + // Should we dynamically allocate?

I am going to allocate every buffer by FrameBufferAllocator.
That is, allocate() returns std::unique_ptr<FrameBuffer>.
I have to declare a class deriving FrameBufferAllocator so that I can
let the additional resource be released when the returned FrameBuffer
is released.
But I cannot because FrameBuffer is marked final.
I am waiting for Laurent's answer if I can drop it.
https://patchwork.libcamera.org/patch/13944/

Thanks,
-Hiro

On Tue, Oct 26, 2021 at 6:18 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> 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