| Message ID | 20211023103302.152769-2-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@ideasonboard.com> > > /* > * \todo Buffer mapping and processing should be moved to a
Hi Umang, On Mon, Oct 25, 2021 at 1:54 PM Laurent Pinchart <laurent.pinchart@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@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@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@chromium.org> > > > > /* > > * \todo Buffer mapping and processing should be moved to a > > -- > Regards, > > Laurent Pinchart
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@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@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@ideasonboard.com> > > > > > /* > > * \todo Buffer mapping and processing should be moved to a > > -- > Regards, > > Laurent Pinchart
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); /* * \todo Buffer mapping and processing should be moved to a
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@ideasonboard.com> --- src/android/camera_stream.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)