| Message ID | 20210408031040.1388568-1-hiroh@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. 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@chromium.org> > Reviewed-by: Jacopo Mondi <jacopo@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 ? > + > + 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
On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch. > > 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@chromium.org> > > Reviewed-by: Jacopo Mondi <jacopo@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? -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
On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda <hiroh@chromium.org> wrote: > > On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Hiro, > > > > Thank you for the patch. > > > > 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@chromium.org> > > > Reviewed-by: Jacopo Mondi <jacopo@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? > > -Hiro Added https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340 -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
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@chromium.org> > > > > Reviewed-by: Jacopo Mondi <jacopo@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 ? > > > > + > > > > + 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
Hi Laurent, On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart <laurent.pinchart@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@chromium.org> > > > > > Reviewed-by: Jacopo Mondi <jacopo@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
Hi Hiro, On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote: > On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote: > > 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@chromium.org> > > > > > > Reviewed-by: Jacopo Mondi <jacopo@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 In the normal case the buffer handle should be valid. What I'm wondering is if there are valid use cases (even if they're rare) where the HAL could receive an invalid handle, or if that would be the consequence of a serious bug in the camera service. In the latter case, wouldn't it be better to add the check in the camera service just before calling .process_capture_request() ? That would cover all HALs in one go. > > > > > > + > > > > > > + 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
Ricky and Shik, what do you think? On Sun, Apr 18, 2021 at 7:50 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote: > > On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote: > > > 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@chromium.org> > > > > > > > Reviewed-by: Jacopo Mondi <jacopo@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 > > In the normal case the buffer handle should be valid. What I'm wondering > is if there are valid use cases (even if they're rare) where the HAL > could receive an invalid handle, or if that would be the consequence of > a serious bug in the camera service. In the latter case, wouldn't it be > better to add the check in the camera service just before calling > .process_capture_request() ? That would cover all HALs in one go. > Ricky and Shik, what do you think? > > > > > > > + > > > > > > > + 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
By discussion with Daniel offline, The test is required only by ChromeOS due to the fact that we don't trust other camera clients. Chrome shouldn't pass an invalid buffer, although we cannot guarantee ARC, Parallel, or other future clients does the same. For security, the minimum requirement is that the HAL won't crash or put a camera image into an unknown buffer. Since process_capture_request has to return -EINVAL if input is malformed, I think that's why we added this test as a subcase of the requirement. https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#3296 On Mon, Apr 19, 2021 at 11:14 AM Hirokazu Honda <hiroh@chromium.org> wrote: > > Ricky and Shik, what do you think? > > On Sun, Apr 18, 2021 at 7:50 AM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Hiro, > > > > On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote: > > > On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote: > > > > 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@chromium.org> > > > > > > > > Reviewed-by: Jacopo Mondi <jacopo@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 > > > > In the normal case the buffer handle should be valid. What I'm wondering > > is if there are valid use cases (even if they're rare) where the HAL > > could receive an invalid handle, or if that would be the consequence of > > a serious bug in the camera service. In the latter case, wouldn't it be > > better to add the check in the camera service just before calling > > .process_capture_request() ? That would cover all HALs in one go. > > > > Ricky and Shik, what do you think? > > > > > > > > > + > > > > > > > > + 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 > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel -- Cheers. Hanlin Chen
Hi Han-lin, On Tue, Apr 20, 2021 at 07:42:27PM +0800, Han-lin Chen wrote: > By discussion with Daniel offline, > The test is required only by ChromeOS due to the fact that we don't > trust other camera clients. > Chrome shouldn't pass an invalid buffer, although we cannot guarantee > ARC, Parallel, or other future clients does the same. > For security, the minimum requirement is that the HAL won't crash or > put a camera image into an unknown buffer. Do the other clients (ARC++, Parallel, ...) access the camera HAL directly, or do they go through the CrOS camera service ? > Since process_capture_request has to return -EINVAL if input is > malformed, I think that's why we added this test as a subcase of the > requirement. > https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#3296 > > On Mon, Apr 19, 2021 at 11:14 AM Hirokazu Honda wrote: > > > > Ricky and Shik, what do you think? > > > > On Sun, Apr 18, 2021 at 7:50 AM Laurent Pinchart wrote: > > > On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote: > > > > On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote: > > > > > 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@chromium.org> > > > > > > > > > Reviewed-by: Jacopo Mondi <jacopo@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 > > > > > > In the normal case the buffer handle should be valid. What I'm wondering > > > is if there are valid use cases (even if they're rare) where the HAL > > > could receive an invalid handle, or if that would be the consequence of > > > a serious bug in the camera service. In the latter case, wouldn't it be > > > better to add the check in the camera service just before calling > > > .process_capture_request() ? That would cover all HALs in one go. > > > > > > > Ricky and Shik, what do you think? > > > > > > > > > > > + > > > > > > > > > + 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
Hi Laurent and Hiro, On Wed, Apr 21, 2021 at 4:42 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Han-lin, > > On Tue, Apr 20, 2021 at 07:42:27PM +0800, Han-lin Chen wrote: > > By discussion with Daniel offline, > > The test is required only by ChromeOS due to the fact that we don't > > trust other camera clients. > > Chrome shouldn't pass an invalid buffer, although we cannot guarantee > > ARC, Parallel, or other future clients does the same. > > For security, the minimum requirement is that the HAL won't crash or > > put a camera image into an unknown buffer. > > Do the other clients (ARC++, Parallel, ...) access the camera HAL > directly, or do they go through the CrOS camera service ? Hmm, they do go through the camera service. I added patches to verify the buffer before passing buffers down to HAL, and remove the magic number test. In this case we don't need to call back CameraBufferManage::IsValid() to check the magic number. > > > Since process_capture_request has to return -EINVAL if input is > > malformed, I think that's why we added this test as a subcase of the > > requirement. > > https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#3296 > > > > On Mon, Apr 19, 2021 at 11:14 AM Hirokazu Honda wrote: > > > > > > Ricky and Shik, what do you think? > > > > > > On Sun, Apr 18, 2021 at 7:50 AM Laurent Pinchart wrote: > > > > On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote: > > > > > On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote: > > > > > > 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@chromium.org> > > > > > > > > > > Reviewed-by: Jacopo Mondi <jacopo@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 > > > > > > > > In the normal case the buffer handle should be valid. What I'm wondering > > > > is if there are valid use cases (even if they're rare) where the HAL > > > > could receive an invalid handle, or if that would be the consequence of > > > > a serious bug in the camera service. In the latter case, wouldn't it be > > > > better to add the check in the camera service just before calling > > > > .process_capture_request() ? That would cover all HALs in one go. > > > > > > > > > > Ricky and Shik, what do you think? > > > > > > > > > > > > > + > > > > > > > > > > + 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
Hi Han-lin, On Thu, Apr 22, 2021 at 04:27:35PM +0800, Han-lin Chen wrote: > On Wed, Apr 21, 2021 at 4:42 AM Laurent Pinchart wrote: > > On Tue, Apr 20, 2021 at 07:42:27PM +0800, Han-lin Chen wrote: > > > By discussion with Daniel offline, > > > The test is required only by ChromeOS due to the fact that we don't > > > trust other camera clients. > > > Chrome shouldn't pass an invalid buffer, although we cannot guarantee > > > ARC, Parallel, or other future clients does the same. > > > For security, the minimum requirement is that the HAL won't crash or > > > put a camera image into an unknown buffer. > > > > Do the other clients (ARC++, Parallel, ...) access the camera HAL > > directly, or do they go through the CrOS camera service ? > > Hmm, they do go through the camera service. > I added patches to verify the buffer before passing buffers down to > HAL, and remove the magic number test. > In this case we don't need to call back CameraBufferManage::IsValid() > to check the magic number. Thank you ! Now all the HAL implementations are protected :-) > > > Since process_capture_request has to return -EINVAL if input is > > > malformed, I think that's why we added this test as a subcase of the > > > requirement. > > > https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#3296 > > > > > > On Mon, Apr 19, 2021 at 11:14 AM Hirokazu Honda wrote: > > > > > > > > Ricky and Shik, what do you think? > > > > > > > > On Sun, Apr 18, 2021 at 7:50 AM Laurent Pinchart wrote: > > > > > On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote: > > > > > > On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote: > > > > > > > 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@chromium.org> > > > > > > > > > > > Reviewed-by: Jacopo Mondi <jacopo@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 > > > > > > > > > > In the normal case the buffer handle should be valid. What I'm wondering > > > > > is if there are valid use cases (even if they're rare) where the HAL > > > > > could receive an invalid handle, or if that would be the consequence of > > > > > a serious bug in the camera service. In the latter case, wouldn't it be > > > > > better to add the check in the camera service just before calling > > > > > .process_capture_request() ? That would cover all HALs in one go. > > > > > > > > > > > > > Ricky and Shik, what do you think? > > > > > > > > > > > > > > > + > > > > > > > > > > > + 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
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; + } + + bufferManager->Deregister(camera3Buffer); + + 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