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

Message ID 20210927104821.2526508-1-hiroh@chromium.org
Headers show
Series
  • android: Fix the issue with Type::Mapped only request
Related show

Message

Hirokazu Honda Sept. 27, 2021, 10:48 a.m. UTC 
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.

With this patch, android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces passed.

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

Comments

Jacopo Mondi Oct. 13, 2021, 11:46 a.m. UTC | #1 
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.

>
> 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
Jacopo Mondi Oct. 26, 2021, 9:19 a.m. UTC | #2 
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
Hirokazu Honda Oct. 26, 2021, 9:25 a.m. UTC | #3 
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@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