[libcamera-devel] [PATCH v3 1/4] android: Modify PostProcessor interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Oct 22 16:29:34 CEST 2020
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 at chromium.org>
> > >>>> Reviewed-by: Umang Jain <email at uajain.com>
> > >>>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list