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

[Hirokazu Honda]

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.
>

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

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