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

[Hirokazu Honda]

Hi Umang,

On Mon, Oct 25, 2021 at 1:54 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 think this is satisfied at this moment. Although we may have to
change this while we are supporting more post processing cases.

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>

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