| Message ID | 20201021013955.301790-1-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Commit | b8dd5ce944eca4d17df585bb059595729905f7ec |
| Headers | show |
| Series |
|
| Related | show |
Hi all, I have re-reviewed the series and changes look good to me. Patches 3/4 and 4/4 will require changing const pointer style to use prefix-ed const. Apart from that, I don't think anything is blocking for merging. Thanks! On 10/21/20 7:09 AM, Hirokazu Honda wrote: > In PostProcessor::process(), the |source| argument doesn't have > to be a pointer. This replaces its type, const pointer, with > const reference as the latter is preferred to the former. > libcamera::Span is cheap to construct/copy/move. We should deal > with the type as pass-by-value parameter. Therefore this also > drops the const reference in the |destination| argument. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > Reviewed-by: Umang Jain <email@uajain.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/camera_stream.cpp | 2 +- > src/android/jpeg/post_processor_jpeg.cpp | 6 +++--- > src/android/jpeg/post_processor_jpeg.h | 4 ++-- > src/android/post_processor.h | 4 ++-- > 4 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index eae451e..3e5d6be 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -102,7 +102,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source, > if (!postProcessor_) > return 0; > > - return postProcessor_->process(&source, dest->maps()[0], metadata); > + return postProcessor_->process(source, dest->maps()[0], metadata); > } > > FrameBuffer *CameraStream::getBuffer() > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 9d452b7..90bf10d 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg, > return encoder_->configure(inCfg); > } > > -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, > - const libcamera::Span<uint8_t> &destination, > +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, > + libcamera::Span<uint8_t> destination, > CameraMetadata *metadata) > { > if (!encoder_) > @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, > if (exif.generate() != 0) > LOG(JPEG, Error) << "Failed to generate valid EXIF data"; > > - int jpeg_size = encoder_->encode(source, destination, exif.data()); > + int jpeg_size = encoder_->encode(&source, destination, exif.data()); > if (jpeg_size < 0) { > LOG(JPEG, Error) << "Failed to encode stream image"; > return jpeg_size; > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h > index 62c8650..ae636ff 100644 > --- a/src/android/jpeg/post_processor_jpeg.h > +++ b/src/android/jpeg/post_processor_jpeg.h > @@ -23,8 +23,8 @@ public: > > int configure(const libcamera::StreamConfiguration &incfg, > const libcamera::StreamConfiguration &outcfg) override; > - int process(const libcamera::FrameBuffer *source, > - const libcamera::Span<uint8_t> &destination, > + int process(const libcamera::FrameBuffer &source, > + libcamera::Span<uint8_t> destination, > CameraMetadata *metadata) override; > > private: > diff --git a/src/android/post_processor.h b/src/android/post_processor.h > index a891c43..5f87a5d 100644 > --- a/src/android/post_processor.h > +++ b/src/android/post_processor.h > @@ -20,8 +20,8 @@ public: > > virtual int configure(const libcamera::StreamConfiguration &inCfg, > const libcamera::StreamConfiguration &outCfg) = 0; > - virtual int process(const libcamera::FrameBuffer *source, > - const libcamera::Span<uint8_t> &destination, > + virtual int process(const libcamera::FrameBuffer &source, > + libcamera::Span<uint8_t> destination, > CameraMetadata *metadata) = 0; > }; >
Hi Umang, / Hiro, On 22/10/2020 08:46, Umang Jain wrote: > Hi all, > > I have re-reviewed the series and changes look good to me. > > Patches 3/4 and 4/4 will require changing const pointer style to use > prefix-ed const. Apart from that, I don't think anything is blocking for > merging. Thanks, but these patches went in yesterday. And now I take a second look, I am more dubious about the const conditions on some of those entries. In locations where I think it looks like the intent was to stop the object being modified, indeed, the const declarations are only stopping the 'pointer' from being modified... Hiro - Could you take another look please and evaluate if the code matches your intentions? I find this to be a really useful tool in translating C sometimes ;-) https://cdecl.org/ For example: const class CameraDevice *cameraDevice_; > declare cameraDevice_ as pointer to const class CameraDevice (https://cdecl.org/?q=const+class+CameraDevice+*cameraDevice_%3B) vs class CameraDevice *const cameraDevice_; > declare cameraDevice_ as const pointer to class CameraDevice (https://cdecl.org/?q=class+CameraDevice+*const+cameraDevice_%3B+) Which while likely a true statement, stating that we can't modify the pointer isn't as protective as stating that we can't modify the class instance itself. -- Regards Kieran > > Thanks! > > On 10/21/20 7:09 AM, Hirokazu Honda wrote: >> In PostProcessor::process(), the |source| argument doesn't have >> to be a pointer. This replaces its type, const pointer, with >> const reference as the latter is preferred to the former. >> libcamera::Span is cheap to construct/copy/move. We should deal >> with the type as pass-by-value parameter. Therefore this also >> drops the const reference in the |destination| argument. >> >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> >> Reviewed-by: Umang Jain <email@uajain.com> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> src/android/camera_stream.cpp | 2 +- >> src/android/jpeg/post_processor_jpeg.cpp | 6 +++--- >> src/android/jpeg/post_processor_jpeg.h | 4 ++-- >> src/android/post_processor.h | 4 ++-- >> 4 files changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/src/android/camera_stream.cpp >> b/src/android/camera_stream.cpp >> index eae451e..3e5d6be 100644 >> --- a/src/android/camera_stream.cpp >> +++ b/src/android/camera_stream.cpp >> @@ -102,7 +102,7 @@ int CameraStream::process(const >> libcamera::FrameBuffer &source, >> if (!postProcessor_) >> return 0; >> - return postProcessor_->process(&source, dest->maps()[0], >> metadata); >> + return postProcessor_->process(source, dest->maps()[0], metadata); >> } >> FrameBuffer *CameraStream::getBuffer() >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp >> b/src/android/jpeg/post_processor_jpeg.cpp >> index 9d452b7..90bf10d 100644 >> --- a/src/android/jpeg/post_processor_jpeg.cpp >> +++ b/src/android/jpeg/post_processor_jpeg.cpp >> @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const >> StreamConfiguration &inCfg, >> return encoder_->configure(inCfg); >> } >> -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, >> - const libcamera::Span<uint8_t> &destination, >> +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, >> + libcamera::Span<uint8_t> destination, >> CameraMetadata *metadata) >> { >> if (!encoder_) >> @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const >> libcamera::FrameBuffer *source, >> if (exif.generate() != 0) >> LOG(JPEG, Error) << "Failed to generate valid EXIF data"; >> - int jpeg_size = encoder_->encode(source, destination, >> exif.data()); >> + int jpeg_size = encoder_->encode(&source, destination, exif.data()); >> if (jpeg_size < 0) { >> LOG(JPEG, Error) << "Failed to encode stream image"; >> return jpeg_size; >> diff --git a/src/android/jpeg/post_processor_jpeg.h >> b/src/android/jpeg/post_processor_jpeg.h >> index 62c8650..ae636ff 100644 >> --- a/src/android/jpeg/post_processor_jpeg.h >> +++ b/src/android/jpeg/post_processor_jpeg.h >> @@ -23,8 +23,8 @@ public: >> int configure(const libcamera::StreamConfiguration &incfg, >> const libcamera::StreamConfiguration &outcfg) override; >> - int process(const libcamera::FrameBuffer *source, >> - const libcamera::Span<uint8_t> &destination, >> + int process(const libcamera::FrameBuffer &source, >> + libcamera::Span<uint8_t> destination, >> CameraMetadata *metadata) override; >> private: >> diff --git a/src/android/post_processor.h b/src/android/post_processor.h >> index a891c43..5f87a5d 100644 >> --- a/src/android/post_processor.h >> +++ b/src/android/post_processor.h >> @@ -20,8 +20,8 @@ public: >> virtual int configure(const libcamera::StreamConfiguration >> &inCfg, >> const libcamera::StreamConfiguration &outCfg) = 0; >> - virtual int process(const libcamera::FrameBuffer *source, >> - const libcamera::Span<uint8_t> &destination, >> + virtual int process(const libcamera::FrameBuffer &source, >> + libcamera::Span<uint8_t> destination, >> CameraMetadata *metadata) = 0; >> }; >> >
On Thu, Oct 22, 2020 at 6:27 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Umang, / Hiro, > > On 22/10/2020 08:46, Umang Jain wrote: > > Hi all, > > > > I have re-reviewed the series and changes look good to me. > > > > Patches 3/4 and 4/4 will require changing const pointer style to use > > prefix-ed const. Apart from that, I don't think anything is blocking for > > merging. > > Thanks, but these patches went in yesterday. And now I take a second > look, I am more dubious about the const conditions on some of those entries. > > In locations where I think it looks like the intent was to stop the > object being modified, indeed, the const declarations are only stopping > the 'pointer' from being modified... > > Hiro - Could you take another look please and evaluate if the code > matches your intentions? > I checked the patches. Yes, they were what I intended. i.e. a const pointer to a class, e.g., CameraDevice *const CameraDevice. For instance, A class "X" is constructed with being given a class pointer of "Z" but doesn't have the ownership of Z. The X unlikely changes the pointer "Z" but should still be able to modify the state of "Z". Does it make sense? Best Regards, -Hiro > I find this to be a really useful tool in translating C sometimes ;-) > > https://cdecl.org/ > > For example: > > const class CameraDevice *cameraDevice_; > > declare cameraDevice_ as pointer to const class CameraDevice > > (https://cdecl.org/?q=const+class+CameraDevice+*cameraDevice_%3B) > > vs > > class CameraDevice *const cameraDevice_; > > declare cameraDevice_ as const pointer to class CameraDevice > > (https://cdecl.org/?q=class+CameraDevice+*const+cameraDevice_%3B+) > > Which while likely a true statement, stating that we can't modify the > pointer isn't as protective as stating that we can't modify the class > instance itself. > > -- > Regards > > Kieran > > > > > > Thanks! > > > > On 10/21/20 7:09 AM, Hirokazu Honda wrote: > >> In PostProcessor::process(), the |source| argument doesn't have > >> to be a pointer. This replaces its type, const pointer, with > >> const reference as the latter is preferred to the former. > >> libcamera::Span is cheap to construct/copy/move. We should deal > >> with the type as pass-by-value parameter. Therefore this also > >> drops the const reference in the |destination| argument. > >> > >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > >> Reviewed-by: Umang Jain <email@uajain.com> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- > >> src/android/camera_stream.cpp | 2 +- > >> src/android/jpeg/post_processor_jpeg.cpp | 6 +++--- > >> src/android/jpeg/post_processor_jpeg.h | 4 ++-- > >> src/android/post_processor.h | 4 ++-- > >> 4 files changed, 8 insertions(+), 8 deletions(-) > >> > >> diff --git a/src/android/camera_stream.cpp > >> b/src/android/camera_stream.cpp > >> index eae451e..3e5d6be 100644 > >> --- a/src/android/camera_stream.cpp > >> +++ b/src/android/camera_stream.cpp > >> @@ -102,7 +102,7 @@ int CameraStream::process(const > >> libcamera::FrameBuffer &source, > >> if (!postProcessor_) > >> return 0; > >> - return postProcessor_->process(&source, dest->maps()[0], > >> metadata); > >> + return postProcessor_->process(source, dest->maps()[0], metadata); > >> } > >> FrameBuffer *CameraStream::getBuffer() > >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp > >> b/src/android/jpeg/post_processor_jpeg.cpp > >> index 9d452b7..90bf10d 100644 > >> --- a/src/android/jpeg/post_processor_jpeg.cpp > >> +++ b/src/android/jpeg/post_processor_jpeg.cpp > >> @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const > >> StreamConfiguration &inCfg, > >> return encoder_->configure(inCfg); > >> } > >> -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, > >> - const libcamera::Span<uint8_t> &destination, > >> +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, > >> + libcamera::Span<uint8_t> destination, > >> CameraMetadata *metadata) > >> { > >> if (!encoder_) > >> @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const > >> libcamera::FrameBuffer *source, > >> if (exif.generate() != 0) > >> LOG(JPEG, Error) << "Failed to generate valid EXIF data"; > >> - int jpeg_size = encoder_->encode(source, destination, > >> exif.data()); > >> + int jpeg_size = encoder_->encode(&source, destination, exif.data()); > >> if (jpeg_size < 0) { > >> LOG(JPEG, Error) << "Failed to encode stream image"; > >> return jpeg_size; > >> diff --git a/src/android/jpeg/post_processor_jpeg.h > >> b/src/android/jpeg/post_processor_jpeg.h > >> index 62c8650..ae636ff 100644 > >> --- a/src/android/jpeg/post_processor_jpeg.h > >> +++ b/src/android/jpeg/post_processor_jpeg.h > >> @@ -23,8 +23,8 @@ public: > >> int configure(const libcamera::StreamConfiguration &incfg, > >> const libcamera::StreamConfiguration &outcfg) override; > >> - int process(const libcamera::FrameBuffer *source, > >> - const libcamera::Span<uint8_t> &destination, > >> + int process(const libcamera::FrameBuffer &source, > >> + libcamera::Span<uint8_t> destination, > >> CameraMetadata *metadata) override; > >> private: > >> diff --git a/src/android/post_processor.h b/src/android/post_processor.h > >> index a891c43..5f87a5d 100644 > >> --- a/src/android/post_processor.h > >> +++ b/src/android/post_processor.h > >> @@ -20,8 +20,8 @@ public: > >> virtual int configure(const libcamera::StreamConfiguration > >> &inCfg, > >> const libcamera::StreamConfiguration &outCfg) = 0; > >> - virtual int process(const libcamera::FrameBuffer *source, > >> - const libcamera::Span<uint8_t> &destination, > >> + virtual int process(const libcamera::FrameBuffer &source, > >> + libcamera::Span<uint8_t> destination, > >> CameraMetadata *metadata) = 0; > >> }; > >> > > > > -- > Regards > -- > Kieran
Hi Hiro / Kieran, On 10/22/20 4:28 PM, Hirokazu Honda wrote: > On Thu, Oct 22, 2020 at 6:27 PM Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: >> Hi Umang, / Hiro, >> >> On 22/10/2020 08:46, Umang Jain wrote: >>> Hi all, >>> >>> I have re-reviewed the series and changes look good to me. >>> >>> Patches 3/4 and 4/4 will require changing const pointer style to use >>> prefix-ed const. Apart from that, I don't think anything is blocking for >>> merging. >> Thanks, but these patches went in yesterday. And now I take a second >> look, I am more dubious about the const conditions on some of those entries. >> >> In locations where I think it looks like the intent was to stop the >> object being modified, indeed, the const declarations are only stopping >> the 'pointer' from being modified... >> >> Hiro - Could you take another look please and evaluate if the code >> matches your intentions? >> > I checked the patches. > Yes, they were what I intended. i.e. a const pointer to a class, e.g., > CameraDevice *const CameraDevice. I think I would digress here. I think what we needed here from the start was: `const X *p` i.e. pointer to an X that is constant. https://isocpp.org/wiki/faq/const-correctness#ptr-to-const > For instance, A class "X" is constructed with being given a class > pointer of "Z" but doesn't have the ownership of Z. > The X unlikely changes the pointer "Z" but should still be able to > modify the state of "Z". > Does it make sense? I don't think we need the ability to modify "Z", in this case (patch 3/4 and 4/4), Z is the cameraDevice_. looking at both CameraStream and PostProcessorJpeg on how they use CameraDevice API, one can make out the both (CameraStream and PostProcessor) does not _intent_ to change the state of cameraDevice_ (you can also double-check via how all getters in camera_device.h are declared `const`). Does this reasoning makes sense? -- uajain > > Best Regards, > -Hiro > >> I find this to be a really useful tool in translating C sometimes ;-) >> >> https://cdecl.org/ >> >> For example: >> >> const class CameraDevice *cameraDevice_; >> > declare cameraDevice_ as pointer to const class CameraDevice >> >> (https://cdecl.org/?q=const+class+CameraDevice+*cameraDevice_%3B) >> >> vs >> >> class CameraDevice *const cameraDevice_; >> > declare cameraDevice_ as const pointer to class CameraDevice >> >> (https://cdecl.org/?q=class+CameraDevice+*const+cameraDevice_%3B+) >> >> Which while likely a true statement, stating that we can't modify the >> pointer isn't as protective as stating that we can't modify the class >> instance itself. >> >> -- >> Regards >> >> Kieran >> >> >>> Thanks! >>> >>> On 10/21/20 7:09 AM, Hirokazu Honda wrote: >>>> In PostProcessor::process(), the |source| argument doesn't have >>>> to be a pointer. This replaces its type, const pointer, with >>>> const reference as the latter is preferred to the former. >>>> libcamera::Span is cheap to construct/copy/move. We should deal >>>> with the type as pass-by-value parameter. Therefore this also >>>> drops the const reference in the |destination| argument. >>>> >>>> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> >>>> Reviewed-by: Umang Jain <email@uajain.com> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> --- >>>> src/android/camera_stream.cpp | 2 +- >>>> src/android/jpeg/post_processor_jpeg.cpp | 6 +++--- >>>> src/android/jpeg/post_processor_jpeg.h | 4 ++-- >>>> src/android/post_processor.h | 4 ++-- >>>> 4 files changed, 8 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/src/android/camera_stream.cpp >>>> b/src/android/camera_stream.cpp >>>> index eae451e..3e5d6be 100644 >>>> --- a/src/android/camera_stream.cpp >>>> +++ b/src/android/camera_stream.cpp >>>> @@ -102,7 +102,7 @@ int CameraStream::process(const >>>> libcamera::FrameBuffer &source, >>>> if (!postProcessor_) >>>> return 0; >>>> - return postProcessor_->process(&source, dest->maps()[0], >>>> metadata); >>>> + return postProcessor_->process(source, dest->maps()[0], metadata); >>>> } >>>> FrameBuffer *CameraStream::getBuffer() >>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp >>>> b/src/android/jpeg/post_processor_jpeg.cpp >>>> index 9d452b7..90bf10d 100644 >>>> --- a/src/android/jpeg/post_processor_jpeg.cpp >>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp >>>> @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const >>>> StreamConfiguration &inCfg, >>>> return encoder_->configure(inCfg); >>>> } >>>> -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, >>>> - const libcamera::Span<uint8_t> &destination, >>>> +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, >>>> + libcamera::Span<uint8_t> destination, >>>> CameraMetadata *metadata) >>>> { >>>> if (!encoder_) >>>> @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const >>>> libcamera::FrameBuffer *source, >>>> if (exif.generate() != 0) >>>> LOG(JPEG, Error) << "Failed to generate valid EXIF data"; >>>> - int jpeg_size = encoder_->encode(source, destination, >>>> exif.data()); >>>> + int jpeg_size = encoder_->encode(&source, destination, exif.data()); >>>> if (jpeg_size < 0) { >>>> LOG(JPEG, Error) << "Failed to encode stream image"; >>>> return jpeg_size; >>>> diff --git a/src/android/jpeg/post_processor_jpeg.h >>>> b/src/android/jpeg/post_processor_jpeg.h >>>> index 62c8650..ae636ff 100644 >>>> --- a/src/android/jpeg/post_processor_jpeg.h >>>> +++ b/src/android/jpeg/post_processor_jpeg.h >>>> @@ -23,8 +23,8 @@ public: >>>> int configure(const libcamera::StreamConfiguration &incfg, >>>> const libcamera::StreamConfiguration &outcfg) override; >>>> - int process(const libcamera::FrameBuffer *source, >>>> - const libcamera::Span<uint8_t> &destination, >>>> + int process(const libcamera::FrameBuffer &source, >>>> + libcamera::Span<uint8_t> destination, >>>> CameraMetadata *metadata) override; >>>> private: >>>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h >>>> index a891c43..5f87a5d 100644 >>>> --- a/src/android/post_processor.h >>>> +++ b/src/android/post_processor.h >>>> @@ -20,8 +20,8 @@ public: >>>> virtual int configure(const libcamera::StreamConfiguration >>>> &inCfg, >>>> const libcamera::StreamConfiguration &outCfg) = 0; >>>> - virtual int process(const libcamera::FrameBuffer *source, >>>> - const libcamera::Span<uint8_t> &destination, >>>> + virtual int process(const libcamera::FrameBuffer &source, >>>> + libcamera::Span<uint8_t> destination, >>>> CameraMetadata *metadata) = 0; >>>> }; >>>> >> -- >> Regards >> -- >> Kieran
On Thu, Oct 22, 2020 at 8:59 PM Umang Jain <email@uajain.com> wrote: > > Hi Hiro / Kieran, > > On 10/22/20 4:28 PM, Hirokazu Honda wrote: > > On Thu, Oct 22, 2020 at 6:27 PM Kieran Bingham > > <kieran.bingham@ideasonboard.com> wrote: > >> Hi Umang, / Hiro, > >> > >> On 22/10/2020 08:46, Umang Jain wrote: > >>> Hi all, > >>> > >>> I have re-reviewed the series and changes look good to me. > >>> > >>> Patches 3/4 and 4/4 will require changing const pointer style to use > >>> prefix-ed const. Apart from that, I don't think anything is blocking for > >>> merging. > >> Thanks, but these patches went in yesterday. And now I take a second > >> look, I am more dubious about the const conditions on some of those entries. > >> > >> In locations where I think it looks like the intent was to stop the > >> object being modified, indeed, the const declarations are only stopping > >> the 'pointer' from being modified... > >> > >> Hiro - Could you take another look please and evaluate if the code > >> matches your intentions? > >> > > I checked the patches. > > Yes, they were what I intended. i.e. a const pointer to a class, e.g., > > CameraDevice *const CameraDevice. > I think I would digress here. I think what we needed here from the start > was: `const X *p` i.e. pointer to an X that is constant. > https://isocpp.org/wiki/faq/const-correctness#ptr-to-const > > For instance, A class "X" is constructed with being given a class > > pointer of "Z" but doesn't have the ownership of Z. > > The X unlikely changes the pointer "Z" but should still be able to > > modify the state of "Z". > > Does it make sense? > > I don't think we need the ability to modify "Z", in this case (patch 3/4 > and 4/4), Z is the cameraDevice_. looking at both CameraStream and > PostProcessorJpeg on how they use CameraDevice API, one can make out the > both (CameraStream and PostProcessor) does not _intent_ to change the > state of cameraDevice_ (you can also double-check via how all getters in > camera_device.h are declared `const`). Does this reasoning makes sense? > I am fine to change it to so. It is aligned with the current usage. Best Regards, -Hiro > -- > uajain > > > > > > Best Regards, > > -Hiro > > > >> I find this to be a really useful tool in translating C sometimes ;-) > >> > >> https://cdecl.org/ > >> > >> For example: > >> > >> const class CameraDevice *cameraDevice_; > >> > declare cameraDevice_ as pointer to const class CameraDevice > >> > >> (https://cdecl.org/?q=const+class+CameraDevice+*cameraDevice_%3B) > >> > >> vs > >> > >> class CameraDevice *const cameraDevice_; > >> > declare cameraDevice_ as const pointer to class CameraDevice > >> > >> (https://cdecl.org/?q=class+CameraDevice+*const+cameraDevice_%3B+) > >> > >> Which while likely a true statement, stating that we can't modify the > >> pointer isn't as protective as stating that we can't modify the class > >> instance itself. > >> > >> -- > >> Regards > >> > >> Kieran > >> > >> > >>> Thanks! > >>> > >>> On 10/21/20 7:09 AM, Hirokazu Honda wrote: > >>>> In PostProcessor::process(), the |source| argument doesn't have > >>>> to be a pointer. This replaces its type, const pointer, with > >>>> const reference as the latter is preferred to the former. > >>>> libcamera::Span is cheap to construct/copy/move. We should deal > >>>> with the type as pass-by-value parameter. Therefore this also > >>>> drops the const reference in the |destination| argument. > >>>> > >>>> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > >>>> Reviewed-by: Umang Jain <email@uajain.com> > >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> --- > >>>> src/android/camera_stream.cpp | 2 +- > >>>> src/android/jpeg/post_processor_jpeg.cpp | 6 +++--- > >>>> src/android/jpeg/post_processor_jpeg.h | 4 ++-- > >>>> src/android/post_processor.h | 4 ++-- > >>>> 4 files changed, 8 insertions(+), 8 deletions(-) > >>>> > >>>> diff --git a/src/android/camera_stream.cpp > >>>> b/src/android/camera_stream.cpp > >>>> index eae451e..3e5d6be 100644 > >>>> --- a/src/android/camera_stream.cpp > >>>> +++ b/src/android/camera_stream.cpp > >>>> @@ -102,7 +102,7 @@ int CameraStream::process(const > >>>> libcamera::FrameBuffer &source, > >>>> if (!postProcessor_) > >>>> return 0; > >>>> - return postProcessor_->process(&source, dest->maps()[0], > >>>> metadata); > >>>> + return postProcessor_->process(source, dest->maps()[0], metadata); > >>>> } > >>>> FrameBuffer *CameraStream::getBuffer() > >>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp > >>>> b/src/android/jpeg/post_processor_jpeg.cpp > >>>> index 9d452b7..90bf10d 100644 > >>>> --- a/src/android/jpeg/post_processor_jpeg.cpp > >>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp > >>>> @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const > >>>> StreamConfiguration &inCfg, > >>>> return encoder_->configure(inCfg); > >>>> } > >>>> -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, > >>>> - const libcamera::Span<uint8_t> &destination, > >>>> +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, > >>>> + libcamera::Span<uint8_t> destination, > >>>> CameraMetadata *metadata) > >>>> { > >>>> if (!encoder_) > >>>> @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const > >>>> libcamera::FrameBuffer *source, > >>>> if (exif.generate() != 0) > >>>> LOG(JPEG, Error) << "Failed to generate valid EXIF data"; > >>>> - int jpeg_size = encoder_->encode(source, destination, > >>>> exif.data()); > >>>> + int jpeg_size = encoder_->encode(&source, destination, exif.data()); > >>>> if (jpeg_size < 0) { > >>>> LOG(JPEG, Error) << "Failed to encode stream image"; > >>>> return jpeg_size; > >>>> diff --git a/src/android/jpeg/post_processor_jpeg.h > >>>> b/src/android/jpeg/post_processor_jpeg.h > >>>> index 62c8650..ae636ff 100644 > >>>> --- a/src/android/jpeg/post_processor_jpeg.h > >>>> +++ b/src/android/jpeg/post_processor_jpeg.h > >>>> @@ -23,8 +23,8 @@ public: > >>>> int configure(const libcamera::StreamConfiguration &incfg, > >>>> const libcamera::StreamConfiguration &outcfg) override; > >>>> - int process(const libcamera::FrameBuffer *source, > >>>> - const libcamera::Span<uint8_t> &destination, > >>>> + int process(const libcamera::FrameBuffer &source, > >>>> + libcamera::Span<uint8_t> destination, > >>>> CameraMetadata *metadata) override; > >>>> private: > >>>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h > >>>> index a891c43..5f87a5d 100644 > >>>> --- a/src/android/post_processor.h > >>>> +++ b/src/android/post_processor.h > >>>> @@ -20,8 +20,8 @@ public: > >>>> virtual int configure(const libcamera::StreamConfiguration > >>>> &inCfg, > >>>> const libcamera::StreamConfiguration &outCfg) = 0; > >>>> - virtual int process(const libcamera::FrameBuffer *source, > >>>> - const libcamera::Span<uint8_t> &destination, > >>>> + virtual int process(const libcamera::FrameBuffer &source, > >>>> + libcamera::Span<uint8_t> destination, > >>>> CameraMetadata *metadata) = 0; > >>>> }; > >>>> > >> -- > >> Regards > >> -- > >> Kieran >
Hello everybody, On Thu, Oct 22, 2020 at 10:40:51PM +0900, Hirokazu Honda wrote: > On Thu, Oct 22, 2020 at 8:59 PM Umang Jain wrote: > > On 10/22/20 4:28 PM, Hirokazu Honda wrote: > > > On Thu, Oct 22, 2020 at 6:27 PM Kieran Bingham wrote: > > >> On 22/10/2020 08:46, Umang Jain wrote: > > >>> Hi all, > > >>> > > >>> I have re-reviewed the series and changes look good to me. > > >>> > > >>> Patches 3/4 and 4/4 will require changing const pointer style to use > > >>> prefix-ed const. Apart from that, I don't think anything is blocking for > > >>> merging. > > >> > > >> Thanks, but these patches went in yesterday. And now I take a second > > >> look, I am more dubious about the const conditions on some of those entries. > > >> > > >> In locations where I think it looks like the intent was to stop the > > >> object being modified, indeed, the const declarations are only stopping > > >> the 'pointer' from being modified... > > >> > > >> Hiro - Could you take another look please and evaluate if the code > > >> matches your intentions? > > > > > > I checked the patches. > > > Yes, they were what I intended. i.e. a const pointer to a class, e.g., > > > CameraDevice *const CameraDevice. > > > > I think I would digress here. I think what we needed here from the start > > was: `const X *p` i.e. pointer to an X that is constant. > > https://isocpp.org/wiki/faq/const-correctness#ptr-to-const I agree with Hiro on this. For class members that are meant to be set at construction time and not modified afterwards, T *const member_; is correct. It captures the fact that if code in the class tries to modify member_, it's an error We may or may not also need to qualify the object pointed to as const, that is const T *const member_; depending on whether the class should be able to call non-const functions of the member. That's an orthogonal issue. And it's certainly also valid to have const T *member_; when the pointer is meant to point to different instances of T during its lifetime. In this specific case, we should likely use const T* const cameraDevice_; as neither the pointer nor the object pointed to should be modified. > > > For instance, A class "X" is constructed with being given a class > > > pointer of "Z" but doesn't have the ownership of Z. > > > The X unlikely changes the pointer "Z" but should still be able to > > > modify the state of "Z". > > > Does it make sense? > > > > I don't think we need the ability to modify "Z", in this case (patch 3/4 > > and 4/4), Z is the cameraDevice_. looking at both CameraStream and > > PostProcessorJpeg on how they use CameraDevice API, one can make out the > > both (CameraStream and PostProcessor) does not _intent_ to change the > > state of cameraDevice_ (you can also double-check via how all getters in > > camera_device.h are declared `const`). Does this reasoning makes sense? > > I am fine to change it to so. It is aligned with the current usage. > > > >> I find this to be a really useful tool in translating C sometimes ;-) > > >> > > >> https://cdecl.org/ > > >> > > >> For example: > > >> > > >> const class CameraDevice *cameraDevice_; > > >> > declare cameraDevice_ as pointer to const class CameraDevice > > >> > > >> (https://cdecl.org/?q=const+class+CameraDevice+*cameraDevice_%3B) > > >> > > >> vs > > >> > > >> class CameraDevice *const cameraDevice_; > > >> > declare cameraDevice_ as const pointer to class CameraDevice > > >> > > >> (https://cdecl.org/?q=class+CameraDevice+*const+cameraDevice_%3B+) > > >> > > >> Which while likely a true statement, stating that we can't modify the > > >> pointer isn't as protective as stating that we can't modify the class > > >> instance itself. > > >> > > >> -- > > >> Regards > > >> > > >> Kieran > > >> > > >> > > >>> Thanks! > > >>> > > >>> On 10/21/20 7:09 AM, Hirokazu Honda wrote: > > >>>> In PostProcessor::process(), the |source| argument doesn't have > > >>>> to be a pointer. This replaces its type, const pointer, with > > >>>> const reference as the latter is preferred to the former. > > >>>> libcamera::Span is cheap to construct/copy/move. We should deal > > >>>> with the type as pass-by-value parameter. Therefore this also > > >>>> drops the const reference in the |destination| argument. > > >>>> > > >>>> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > >>>> Reviewed-by: Umang Jain <email@uajain.com> > > >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >>>> --- > > >>>> src/android/camera_stream.cpp | 2 +- > > >>>> src/android/jpeg/post_processor_jpeg.cpp | 6 +++--- > > >>>> src/android/jpeg/post_processor_jpeg.h | 4 ++-- > > >>>> src/android/post_processor.h | 4 ++-- > > >>>> 4 files changed, 8 insertions(+), 8 deletions(-) > > >>>> > > >>>> diff --git a/src/android/camera_stream.cpp > > >>>> b/src/android/camera_stream.cpp > > >>>> index eae451e..3e5d6be 100644 > > >>>> --- a/src/android/camera_stream.cpp > > >>>> +++ b/src/android/camera_stream.cpp > > >>>> @@ -102,7 +102,7 @@ int CameraStream::process(const > > >>>> libcamera::FrameBuffer &source, > > >>>> if (!postProcessor_) > > >>>> return 0; > > >>>> - return postProcessor_->process(&source, dest->maps()[0], > > >>>> metadata); > > >>>> + return postProcessor_->process(source, dest->maps()[0], metadata); > > >>>> } > > >>>> FrameBuffer *CameraStream::getBuffer() > > >>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp > > >>>> b/src/android/jpeg/post_processor_jpeg.cpp > > >>>> index 9d452b7..90bf10d 100644 > > >>>> --- a/src/android/jpeg/post_processor_jpeg.cpp > > >>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp > > >>>> @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const > > >>>> StreamConfiguration &inCfg, > > >>>> return encoder_->configure(inCfg); > > >>>> } > > >>>> -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, > > >>>> - const libcamera::Span<uint8_t> &destination, > > >>>> +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, > > >>>> + libcamera::Span<uint8_t> destination, > > >>>> CameraMetadata *metadata) > > >>>> { > > >>>> if (!encoder_) > > >>>> @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const > > >>>> libcamera::FrameBuffer *source, > > >>>> if (exif.generate() != 0) > > >>>> LOG(JPEG, Error) << "Failed to generate valid EXIF data"; > > >>>> - int jpeg_size = encoder_->encode(source, destination, > > >>>> exif.data()); > > >>>> + int jpeg_size = encoder_->encode(&source, destination, exif.data()); > > >>>> if (jpeg_size < 0) { > > >>>> LOG(JPEG, Error) << "Failed to encode stream image"; > > >>>> return jpeg_size; > > >>>> diff --git a/src/android/jpeg/post_processor_jpeg.h > > >>>> b/src/android/jpeg/post_processor_jpeg.h > > >>>> index 62c8650..ae636ff 100644 > > >>>> --- a/src/android/jpeg/post_processor_jpeg.h > > >>>> +++ b/src/android/jpeg/post_processor_jpeg.h > > >>>> @@ -23,8 +23,8 @@ public: > > >>>> int configure(const libcamera::StreamConfiguration &incfg, > > >>>> const libcamera::StreamConfiguration &outcfg) override; > > >>>> - int process(const libcamera::FrameBuffer *source, > > >>>> - const libcamera::Span<uint8_t> &destination, > > >>>> + int process(const libcamera::FrameBuffer &source, > > >>>> + libcamera::Span<uint8_t> destination, > > >>>> CameraMetadata *metadata) override; > > >>>> private: > > >>>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h > > >>>> index a891c43..5f87a5d 100644 > > >>>> --- a/src/android/post_processor.h > > >>>> +++ b/src/android/post_processor.h > > >>>> @@ -20,8 +20,8 @@ public: > > >>>> virtual int configure(const libcamera::StreamConfiguration > > >>>> &inCfg, > > >>>> const libcamera::StreamConfiguration &outCfg) = 0; > > >>>> - virtual int process(const libcamera::FrameBuffer *source, > > >>>> - const libcamera::Span<uint8_t> &destination, > > >>>> + virtual int process(const libcamera::FrameBuffer &source, > > >>>> + libcamera::Span<uint8_t> destination, > > >>>> CameraMetadata *metadata) = 0; > > >>>> }; > > >>>>
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index eae451e..3e5d6be 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -102,7 +102,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source, if (!postProcessor_) return 0; - return postProcessor_->process(&source, dest->maps()[0], metadata); + return postProcessor_->process(source, dest->maps()[0], metadata); } FrameBuffer *CameraStream::getBuffer() diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index 9d452b7..90bf10d 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg, return encoder_->configure(inCfg); } -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, - const libcamera::Span<uint8_t> &destination, +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, + libcamera::Span<uint8_t> destination, CameraMetadata *metadata) { if (!encoder_) @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, if (exif.generate() != 0) LOG(JPEG, Error) << "Failed to generate valid EXIF data"; - int jpeg_size = encoder_->encode(source, destination, exif.data()); + int jpeg_size = encoder_->encode(&source, destination, exif.data()); if (jpeg_size < 0) { LOG(JPEG, Error) << "Failed to encode stream image"; return jpeg_size; diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h index 62c8650..ae636ff 100644 --- a/src/android/jpeg/post_processor_jpeg.h +++ b/src/android/jpeg/post_processor_jpeg.h @@ -23,8 +23,8 @@ public: int configure(const libcamera::StreamConfiguration &incfg, const libcamera::StreamConfiguration &outcfg) override; - int process(const libcamera::FrameBuffer *source, - const libcamera::Span<uint8_t> &destination, + int process(const libcamera::FrameBuffer &source, + libcamera::Span<uint8_t> destination, CameraMetadata *metadata) override; private: diff --git a/src/android/post_processor.h b/src/android/post_processor.h index a891c43..5f87a5d 100644 --- a/src/android/post_processor.h +++ b/src/android/post_processor.h @@ -20,8 +20,8 @@ public: virtual int configure(const libcamera::StreamConfiguration &inCfg, const libcamera::StreamConfiguration &outCfg) = 0; - virtual int process(const libcamera::FrameBuffer *source, - const libcamera::Span<uint8_t> &destination, + virtual int process(const libcamera::FrameBuffer &source, + libcamera::Span<uint8_t> destination, CameraMetadata *metadata) = 0; };