[libcamera-devel] [PATCH v2] android: CameraBuffer: Mark as invalid if cros::CameraBufferManager::Register() fails

[Laurent Pinchart]

Hi Hiro,

On Sat, Apr 03, 2021 at 07:49:51PM +0900, Hirokazu Honda wrote:
> On Sat, Apr 3, 2021 at 9:06 AM Laurent Pinchart wrote:
> >
> > Hi Hiro,
> >
> > Thank you for the patch.
> >
> > I'd write the subject line as "android: mm: cros: Handle buffer
> > registration failure" to shorten it.
> >
> > On Fri, Apr 02, 2021 at 01:03:40PM +0900, Hirokazu Honda wrote:
> > > cros::CameraBufferManager::Register() fails if a buffer handle
> > > is invalid. We should mark CameraBuffer as invalid on the failure
> > > of Register().
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > ---
> > >  src/android/mm/cros_camera_buffer.cpp | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> > > index 7df4f47c..5f98ee44 100644
> > > --- a/src/android/mm/cros_camera_buffer.cpp
> > > +++ b/src/android/mm/cros_camera_buffer.cpp
> > > @@ -53,12 +53,16 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> > >  {
> > >       bufferManager_ = cros::CameraBufferManager::GetInstance();
> > >
> > > -     bufferManager_->Register(camera3Buffer);
> > > +     int ret = bufferManager_->Register(camera3Buffer);
> > > +     if (ret) {
> > > +             LOG(HAL, Error) << "Failed registering a buffer: " << ret;
> >
> > s/a buffer/buffer/
> >
> > This looks good,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > I'll apply the patch if you're OK with the proposed changes.
> 
> I'm fine indeed.
> 
> > One additional question though. The CameraBuffer::Private destructors
> > calls
> >
> >         bufferManager_->Unlock(handle_);
> >         bufferManager_->Deregister(handle_);
> >
> > unconditionally. Is that safe to do if the buffer hasn't been registered
> > and locked properly ?
> 
> I looked the implementation of CameraBufferManagerImpl. [1]
> Looks like both functions return -EINVAL if Register() and
> Lock/LockYCbCr() are not successful.
> Well, it should be safer to check here, but we add the if-condition check here.
> Shall I upload the new patch with them?

Both will log an error in those cases, so it's best to avoid that. Also,
the API documentation doesn't explicitly state that calling those
functions on buffers that haven't been registered and/or locked is safe,
so even if it is today, the implementation may change later. It's best
to be defensive (or get the CameraBufferManager API to explicitly state
that this is fine, and drop the error messages).

> [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc
>
> > > +             return;
> > > +     }
> > >
> > >       numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
> > >       switch (numPlanes_) {
> > >       case 1: {
> > > -             int ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> > > +             ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> > >               if (ret) {
> > >                       LOG(HAL, Error) << "Single plane buffer mapping failed";
> > >                       return;
> > > @@ -67,8 +71,8 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> > >       }
> > >       case 2:
> > >       case 3: {
> > > -             int ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> > > -                                                 &mem.ycbcr);
> > > +             ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> > > +                                             &mem.ycbcr);
> > >               if (ret) {
> > >                       LOG(HAL, Error) << "YCbCr buffer mapping failed";
> > >                       return;

-- 
Regards,

Laurent Pinchart