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

[Hirokazu Honda]

Hi Umang,

On Mon, Oct 25, 2021 at 1:58 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Umang,
>
> Thank you for the patch.
>
> 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.

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