[libcamera-devel] [PATCH v6 2/7] android: post_processor_jpeg: Replace encoder_ nullptr check

[Laurent Pinchart]

Hi Hiro,

On Mon, Oct 25, 2021 at 02:18:29PM +0900, Hirokazu Honda wrote:
> On Mon, Oct 25, 2021 at 1:58 PM Laurent Pinchart wrote:
> > On Sat, Oct 23, 2021 at 04:02:57PM +0530, Umang Jain wrote:
> > > Instead of simply returning if encoder_ is nullptr, fail hard
> > > via an assertion. It is quite unlikely that encoder_ will be
> > > nullptr in PostProcessorJpeg::process() so, be loud about
> > > the failure.
> >
> > If it was only quite unlikely an assertion wouldn't be acceptable. It
> > has to be impossible. That's supposed to be the case, so the change is
> > fine, but I'd write the commit message as "encoder_ could only be null
> > as a result of a fatal bug in the code, so ...".
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> I think this causes a crash of a process calling libcamera.
> Is it acceptable?
> I would like to know the rule of using ASSERT against returning error
> in libcamera.

I don't think we have documented rules (and that should be fixed). My
personal preference at the moment is to use assertions to check for
conditions that are supposed to never happen, and indicate a clear bug
somewhere that we can't recover from. In this case, for instance,
encoder_ can only be null if configure() hasn't been called
successfully, which would mean that the CameraDevice implementation has
a fundamental bug. We can't recover from that, as we don't know what
other problems that bug could cause.

> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > >  src/android/jpeg/post_processor_jpeg.cpp | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > > index 699576ef..49483836 100644
> > > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > > @@ -102,9 +102,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > >                              CameraBuffer *destination,
> > >                              Camera3RequestDescriptor *request)
> > >  {
> > > -     if (!encoder_)
> > > -             return 0;
> > > -
> > > +     ASSERT(encoder_);
> > >       ASSERT(destination->numPlanes() == 1);
> > >
> > >       const CameraMetadata &requestMetadata = request->settings_;

-- 
Regards,

Laurent Pinchart