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

[Kieran Bingham]

Quoting Hirokazu Honda (2021-10-25 06:43:05)
> Hi Laurent,
> 
> On Mon, Oct 25, 2021 at 2:38 PM Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > 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.
> >

Yes, ASSERT means that something has happened which is supposed to be
impossible, and I usually expect it to be used in the case where it
might be possible that a developer could change something in the future,
or incorrectly use an object... such that the ASSERT would fire and
catch the attention of the developer ... and isn't something we'd expect
end users to ever hit.

> I agree to the rule. May I ask you to document somewhere?

I guess this would go in the coding style somewhere? Maybe I assumed
that my interpretation of assertions was generic to 'all' projects?

Do other projects vary wildly in their expectations of using assertions?

anyway, for this patch.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> 
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> 
> > > > > 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