| Message ID | 20201020074229.119151-3-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thanks for your work. On 10/20/20 1:12 PM, Hirokazu Honda wrote: > PostProcessorJpeg doesn't have the ownership of CameraDevice given > in the constructor. To clarify it, this makes the member variable > constant. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/android/jpeg/post_processor_jpeg.cpp | 2 +- > src/android/jpeg/post_processor_jpeg.h | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 6f33631..f895d1f 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -20,7 +20,7 @@ using namespace libcamera; > > LOG_DEFINE_CATEGORY(JPEG); > > -PostProcessorJpeg::PostProcessorJpeg(CameraDevice *device) > +PostProcessorJpeg::PostProcessorJpeg(CameraDevice const *device) > : cameraDevice_(device) > { > } > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h > index ae636ff..8e25b29 100644 > --- a/src/android/jpeg/post_processor_jpeg.h > +++ b/src/android/jpeg/post_processor_jpeg.h > @@ -19,7 +19,7 @@ class CameraDevice; > class PostProcessorJpeg : public PostProcessor > { > public: > - PostProcessorJpeg(CameraDevice *device); > + PostProcessorJpeg(CameraDevice const *device); > > int configure(const libcamera::StreamConfiguration &incfg, > const libcamera::StreamConfiguration &outcfg) override; > @@ -28,7 +28,7 @@ public: > CameraMetadata *metadata) override; > > private: > - CameraDevice *cameraDevice_; > + CameraDevice const *cameraDevice_; > std::unique_ptr<Encoder> encoder_; > libcamera::Size streamSize_; > }; Not a hard-blocker but I think libcamera uses the other style for `const` definition - `const class *ptr_;` I would refer to overlords to use their discretion here before pushing. Reviewed-by: Umang Jain <email@uajain.com>
Hi Hiro, On 20/10/2020 11:45, Umang Jain wrote: > Hi Hiro, > > Thanks for your work. > > On 10/20/20 1:12 PM, Hirokazu Honda wrote: >> PostProcessorJpeg doesn't have the ownership of CameraDevice given >> in the constructor. To clarify it, this makes the member variable >> constant. >> >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> >> --- >> src/android/jpeg/post_processor_jpeg.cpp | 2 +- >> src/android/jpeg/post_processor_jpeg.h | 4 ++-- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp >> b/src/android/jpeg/post_processor_jpeg.cpp >> index 6f33631..f895d1f 100644 >> --- a/src/android/jpeg/post_processor_jpeg.cpp >> +++ b/src/android/jpeg/post_processor_jpeg.cpp >> @@ -20,7 +20,7 @@ using namespace libcamera; >> LOG_DEFINE_CATEGORY(JPEG); >> -PostProcessorJpeg::PostProcessorJpeg(CameraDevice *device) >> +PostProcessorJpeg::PostProcessorJpeg(CameraDevice const *device) >> : cameraDevice_(device) >> { >> } >> diff --git a/src/android/jpeg/post_processor_jpeg.h >> b/src/android/jpeg/post_processor_jpeg.h >> index ae636ff..8e25b29 100644 >> --- a/src/android/jpeg/post_processor_jpeg.h >> +++ b/src/android/jpeg/post_processor_jpeg.h >> @@ -19,7 +19,7 @@ class CameraDevice; >> class PostProcessorJpeg : public PostProcessor >> { >> public: >> - PostProcessorJpeg(CameraDevice *device); >> + PostProcessorJpeg(CameraDevice const *device); >> int configure(const libcamera::StreamConfiguration &incfg, >> const libcamera::StreamConfiguration &outcfg) override; >> @@ -28,7 +28,7 @@ public: >> CameraMetadata *metadata) override; >> private: >> - CameraDevice *cameraDevice_; >> + CameraDevice const *cameraDevice_; >> std::unique_ptr<Encoder> encoder_; >> libcamera::Size streamSize_; >> }; > Not a hard-blocker but I think libcamera uses the other style for > `const` definition - `const class *ptr_;` I would refer to overlords to > use their discretion here before pushing. Indeed, i think this should be a prefixed const. I can update locally, assuming there's nothing else critical. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Umang Jain <email@uajain.com> > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Wed, Oct 21, 2020 at 12:08 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Hiro, > > On 20/10/2020 11:45, Umang Jain wrote: > > Hi Hiro, > > > > Thanks for your work. > > > > On 10/20/20 1:12 PM, Hirokazu Honda wrote: > >> PostProcessorJpeg doesn't have the ownership of CameraDevice given > >> in the constructor. To clarify it, this makes the member variable > >> constant. > >> > >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > >> --- > >> src/android/jpeg/post_processor_jpeg.cpp | 2 +- > >> src/android/jpeg/post_processor_jpeg.h | 4 ++-- > >> 2 files changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp > >> b/src/android/jpeg/post_processor_jpeg.cpp > >> index 6f33631..f895d1f 100644 > >> --- a/src/android/jpeg/post_processor_jpeg.cpp > >> +++ b/src/android/jpeg/post_processor_jpeg.cpp > >> @@ -20,7 +20,7 @@ using namespace libcamera; > >> LOG_DEFINE_CATEGORY(JPEG); > >> -PostProcessorJpeg::PostProcessorJpeg(CameraDevice *device) > >> +PostProcessorJpeg::PostProcessorJpeg(CameraDevice const *device) > >> : cameraDevice_(device) > >> { > >> } > >> diff --git a/src/android/jpeg/post_processor_jpeg.h > >> b/src/android/jpeg/post_processor_jpeg.h > >> index ae636ff..8e25b29 100644 > >> --- a/src/android/jpeg/post_processor_jpeg.h > >> +++ b/src/android/jpeg/post_processor_jpeg.h > >> @@ -19,7 +19,7 @@ class CameraDevice; > >> class PostProcessorJpeg : public PostProcessor > >> { > >> public: > >> - PostProcessorJpeg(CameraDevice *device); > >> + PostProcessorJpeg(CameraDevice const *device); > >> int configure(const libcamera::StreamConfiguration &incfg, > >> const libcamera::StreamConfiguration &outcfg) override; > >> @@ -28,7 +28,7 @@ public: > >> CameraMetadata *metadata) override; > >> private: > >> - CameraDevice *cameraDevice_; > >> + CameraDevice const *cameraDevice_; > >> std::unique_ptr<Encoder> encoder_; > >> libcamera::Size streamSize_; > >> }; > > Not a hard-blocker but I think libcamera uses the other style for > > `const` definition - `const class *ptr_;` I would refer to overlords to > > use their discretion here before pushing. > > Indeed, i think this should be a prefixed const. > Oops, I made a mistake here. I intended to change it to `class *const ptr_`, which means the ptr value (not value pointed by the ptr) cannot be changed. (The mistake is because I have been writing for it as `class* const ptr_` in my life.) I changed so in the next patch. > I can update locally, assuming there's nothing else critical. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Reviewed-by: Umang Jain <email@uajain.com> > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards > -- > Kieran
diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index 6f33631..f895d1f 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -20,7 +20,7 @@ using namespace libcamera; LOG_DEFINE_CATEGORY(JPEG); -PostProcessorJpeg::PostProcessorJpeg(CameraDevice *device) +PostProcessorJpeg::PostProcessorJpeg(CameraDevice const *device) : cameraDevice_(device) { } diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h index ae636ff..8e25b29 100644 --- a/src/android/jpeg/post_processor_jpeg.h +++ b/src/android/jpeg/post_processor_jpeg.h @@ -19,7 +19,7 @@ class CameraDevice; class PostProcessorJpeg : public PostProcessor { public: - PostProcessorJpeg(CameraDevice *device); + PostProcessorJpeg(CameraDevice const *device); int configure(const libcamera::StreamConfiguration &incfg, const libcamera::StreamConfiguration &outcfg) override; @@ -28,7 +28,7 @@ public: CameraMetadata *metadata) override; private: - CameraDevice *cameraDevice_; + CameraDevice const *cameraDevice_; std::unique_ptr<Encoder> encoder_; libcamera::Size streamSize_; };
PostProcessorJpeg doesn't have the ownership of CameraDevice given in the constructor. To clarify it, this makes the member variable constant. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/android/jpeg/post_processor_jpeg.cpp | 2 +- src/android/jpeg/post_processor_jpeg.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)