| Message ID | 20211023103302.152769-3-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: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@ideasonboard.com> > Signed-off-by: Umang Jain <umang.jain@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_;
Hi Umang, On Mon, Oct 25, 2021 at 1:58 PM Laurent Pinchart <laurent.pinchart@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@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@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
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@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. > > > Signed-off-by: Umang Jain <umang.jain@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_;
Hi Laurent, On Mon, Oct 25, 2021 at 2:38 PM Laurent Pinchart <laurent.pinchart@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@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@chromium.org> > > > > Signed-off-by: Umang Jain <umang.jain@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
Quoting Hirokazu Honda (2021-10-25 06:43:05) > Hi Laurent, > > On Mon, Oct 25, 2021 at 2:38 PM Laurent Pinchart > <laurent.pinchart@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@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. > > Yes, ASSERT means that something has happened which is supposed to be impossible, and I usually expect it to be used in the case where it might be possible that a developer could change something in the future, or incorrectly use an object... such that the ASSERT would fire and catch the attention of the developer ... and isn't something we'd expect end users to ever hit. > I agree to the rule. May I ask you to document somewhere? I guess this would go in the coding style somewhere? Maybe I assumed that my interpretation of assertions was generic to 'all' projects? Do other projects vary wildly in their expectations of using assertions? anyway, for this patch. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > Signed-off-by: Umang Jain <umang.jain@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
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_;
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. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/android/jpeg/post_processor_jpeg.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)