[libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a static function to check a buffer validness

[Hirokazu Honda]

Hi Laurent,

On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Fri, Apr 16, 2021 at 10:43:33PM +0900, Hirokazu Honda wrote:
> > On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda wrote:
> > > On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart wrote:
> > > > On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:
> > > > > This adds a static function to CameraBuffer class that checks if
> > > > > a buffer_handle is valid with a platform dependent framework.
> > > > > For example, the function validates a buffer using
> > > > > cros::CameraBufferManager on ChromeOS.
> > > > >
> > > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > > ---
> > > > >  src/android/camera_buffer.h              |  6 ++++++
> > > > >  src/android/mm/cros_camera_buffer.cpp    | 18 ++++++++++++++++++
> > > > >  src/android/mm/generic_camera_buffer.cpp |  9 +++++++++
> > > > >  3 files changed, 33 insertions(+)
> > > > >
> > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> > > > > index 7e8970b4..ade082c3 100644
> > > > > --- a/src/android/camera_buffer.h
> > > > > +++ b/src/android/camera_buffer.h
> > > > > @@ -20,6 +20,8 @@ public:
> > > > >       CameraBuffer(buffer_handle_t camera3Buffer, int flags);
> > > > >       ~CameraBuffer();
> > > > >
> > > > > +     static bool isValidBuffer(buffer_hanlde_t camera3Buffer);
> > > > > +
> > > > >       bool isValid() const;
> > > > >
> > > > >       unsigned int numPlanes() const;
> > > > > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)       \
> > > > >  CameraBuffer::~CameraBuffer()                                                \
> > > > >  {                                                                    \
> > > > >  }                                                                    \
> > > > > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer)             \
> > > > > +{                                                                    \
> > > > > +     return Private::isValidBuffer(buffer)                           \
> > > > > +}                                                                    \
> > > > >  bool CameraBuffer::isValid() const                                   \
> > > > >  {                                                                    \
> > > > >       const Private *const d = LIBCAMERA_D_PTR();                     \
> > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> > > > > index 1a4fd5d1..f8449dfd 100644
> > > > > --- a/src/android/mm/cros_camera_buffer.cpp
> > > > > +++ b/src/android/mm/cros_camera_buffer.cpp
> > > > > @@ -24,6 +24,8 @@ public:
> > > > >               buffer_handle_t camera3Buffer, int flags);
> > > > >       ~Private();
> > > > >
> > > > > +     static bool isValidBuffer(buffer_handle_t camera3Buffer);
> > > > > +
> > > > >       bool isValid() const { return valid_; }
> > > > >
> > > > >       unsigned int numPlanes() const;
> > > > > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
> > > > >       return bufferManager_->GetPlaneSize(handle_, 0);
> > > > >  }
> > > > >
> > > > > +/* static */
> > > > > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)
> > > > > +{
> > > > > +     cros::CameraBufferManager *bufferManager =
> > > > > +             cros::CameraBufferManager::GetInstance();
> > > > > +
> > > > > +     int ret = bufferManager->Register(camera3Buffer);
> > > > > +     if (ret) {
> > > > > +             return false;
> > > > > +     }
> > > >
> > > > No need for braces.
> > > >
> > > > > +
> > > > > +     bufferManager->Deregister(camera3Buffer);
> > > >
> > > > This seems quite inefficient, as it will lead to registering all
> > > > buffers, even the ones we don't need to map to the CPU. For buffers that
> > > > we need to map to the CPU, we will register and unregister them here,
> > > > and then register them later.
> > > >
> > > > Could we do better than this ?
> > > >
> > >
> > > We may want to add verifyBuffer() to CameraBufferManagerInterface to
> > > do a cheap verification, basically only do
> > > camera_buffer_handle_t::FromBufferHandle().
> > > +Shik Chen +Ricky Liang +Tomasz Figa how do you think?
> >
> > Added https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340
>
> Could you explain what kind of errors you envision this will catch ?
> When would the camera HAL implementation receive an incorrect buffer
> handle in the first place ?
>

It checks a magic number of the structure is the correct one.
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_handle.h;l=82;drc=b45faaeee6b4a79906678746a2d619ccea361358

I have no idea about any situation in the product honestly; our test
tests that processCaptureRequest() fails when an invalid buffer is
passed.
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_frame_test.cc;l=1397;drc=34d10525925863b7dddb3db830d45365b2d7e25a

-Hiro
> > > > > +
> > > > > +     return true;
> > > > > +}
> > > > > +
> > > > >  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> > > > > index 929e078a..07a07372 100644
> > > > > --- a/src/android/mm/generic_camera_buffer.cpp
> > > > > +++ b/src/android/mm/generic_camera_buffer.cpp
> > > > > @@ -24,6 +24,8 @@ public:
> > > > >               buffer_handle_t camera3Buffer, int flags);
> > > > >       ~Private();
> > > > >
> > > > > +     static bool isValidBuffer(buffer_handle_t camera3Buffer);
> > > > > +
> > > > >       unsigned int numPlanes() const;
> > > > >
> > > > >       Span<uint8_t> plane(unsigned int plane);
> > > > > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
> > > > >                                     maxJpegBufferSize);
> > > > >  }
> > > > >
> > > > > +/* static */
> > > > > +bool CameraBuffer::Private::isValidBuffer(
> > > > > +     [[maybe_unused]] buffer_handle_t camera3Buffer)
> > > > > +{
> > > > > +     return true;
> > > > > +}
> > > > > +
> > > > >  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
>
> --
> Regards,
>
> Laurent Pinchart