[libcamera-devel] [PATCH v6 1/7] android: camera_stream: Replace post-processor nullptr check

[Kieran Bingham]

Quoting Laurent Pinchart (2021-10-25 05:53:53)
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Sat, Oct 23, 2021 at 04:02:56PM +0530, Umang Jain wrote:
> > Instead of checking postProcessor for nullptr, replace this
> > check with an assertion that checks if the camera stream's
> > type is not Type::Direct. Since it makes no sense to call
> > CameraStream::process() on a Type::Direct camera stream.
> > 
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> >  src/android/camera_stream.cpp | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index f44a2717..ca25c4db 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -163,8 +163,7 @@ int CameraStream::process(const FrameBuffer &source,
> >               dest.fence = -1;
> >       }
> >  
> > -     if (!postProcessor_)
> > -             return 0;
> > +     ASSERT(type_ != Type::Direct);
> 
> I wonder if this is really equivalent. It depends what the goal of the
> check was. I've understood is as protecting against configure() not
> having been called successfully, but that's just my interpretation, it
> could have been there to protect against process() being called on
> direct streams. As both are equally unlikely, I'm fine with the change.
> Could you however move this to the beginning of the function ?
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

I would hazard a guess to think that the "if (!postProcessor)" was old
code that has followed refactoring that used to return early before
processing JPEG streams (this line in git-blame has derived from when we
had a JPEG encoder wired directly in the camera_device I think.

I think the assertion still makes sense though.

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

> 
> >  
> >       /*
> >        * \todo Buffer mapping and processing should be moved to a
> 
> -- 
> Regards,
> 
> Laurent Pinchart