[libcamera-devel] [PATCH] android: Modify PostProcessor interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Oct 19 14:57:43 CEST 2020
Hello everybody,
Hiro-san, thank you for the patch.
On Mon, Oct 19, 2020 at 04:54:27PM +0530, Umang Jain wrote:
> Hi Hiro,
>
> On 10/19/20 1:33 PM, Hirokazu Honda wrote:
> > This modifies PostProcessor interface and polishes the code
> > around the PostProcessor.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
>
> Changes looks good, albeit a few comments. Also, I think the patch could
> also be split down further to address the per Span pass-by-value, per
> *-type reference to &-type, etc.
Agreed, an explanation of the rationale is useful. It may appear
straightforward today, but when we will read the code in the future we
may not remember the context, so it should be captured in the commit
message.
> > ---
> > src/android/camera_stream.cpp | 9 ++++-----
> > src/android/camera_stream.h | 8 ++++----
> > src/android/jpeg/encoder.h | 6 +++---
> > src/android/jpeg/encoder_libjpeg.cpp | 6 +++---
> > src/android/jpeg/encoder_libjpeg.h | 4 ++--
> > src/android/jpeg/post_processor_jpeg.cpp | 8 +++++---
> > src/android/jpeg/post_processor_jpeg.h | 6 +++---
> > src/android/post_processor.h | 4 ++--
> > 8 files changed, 26 insertions(+), 25 deletions(-)
> >
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 1b8afa8..cc8691d 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -40,11 +40,10 @@ LOG_DECLARE_CATEGORY(HAL);
> >
> > CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
> > camera3_stream_t *camera3Stream, unsigned int index)
> > - : cameraDevice_(cameraDevice), type_(type),
> > + : cameraDevice_(cameraDevice),
> > + config_(cameraDevice_->cameraConfiguration()), type_(type),
>
> Will cameraDevice_ will be initialized by this point? I am not sure.
C++ initializes fields in the order they are defined in the class. The
compiler warns if the initialization list here is in a different order,
so the two are guaranteed to match. cameraDevice_ is thus guaranteed to
be initialized here.
> > camera3Stream_(camera3Stream), index_(index)
> > {
> > - config_ = cameraDevice_->cameraConfiguration();
> > -
> > if (type_ == Type::Internal || type_ == Type::Mapped) {
> > /*
> > * \todo There might be multiple post-processors. The logic
> > @@ -52,7 +51,7 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
> > * future. For now, we only have PostProcessorJpeg and that
> > * is what we instantiate here.
> > */
> > - postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
> > + postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice);
>
> What does this change achieve?
>
> > }
> >
> > if (type == Type::Internal) {
> > @@ -102,7 +101,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/camera_stream.h b/src/android/camera_stream.h
> > index c367a5f..24e66bb 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -124,11 +124,11 @@ public:
> > void putBuffer(libcamera::FrameBuffer *buffer);
> >
> > private:
> > - CameraDevice *cameraDevice_;
> > - libcamera::CameraConfiguration *config_;
> > - Type type_;
> > + CameraDevice const *cameraDevice_;
> > + const libcamera::CameraConfiguration *config_;
> > + const Type type_;
> > camera3_stream_t *camera3Stream_;
> > - unsigned int index_;
> > + const unsigned int index_;
> >
> > std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
> > std::vector<libcamera::FrameBuffer *> buffers_;
> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> > index 4483153..270ea60 100644
> > --- a/src/android/jpeg/encoder.h
> > +++ b/src/android/jpeg/encoder.h
> > @@ -14,11 +14,11 @@
> > class Encoder
> > {
> > public:
> > - virtual ~Encoder() {};
> > + virtual ~Encoder() {}
This should also be split in a separate patch, it's an easy one.
> >
> > virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
> > - virtual int encode(const libcamera::FrameBuffer *source,
> > - const libcamera::Span<uint8_t> &destination,
> > + virtual int encode(const libcamera::FrameBuffer &source,
> > + libcamera::Span<uint8_t> destination,
> > const libcamera::Span<const uint8_t> &exifData) = 0;
> > };
> >
> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> > index 8995ba5..4236c9d 100644
> > --- a/src/android/jpeg/encoder_libjpeg.cpp
> > +++ b/src/android/jpeg/encoder_libjpeg.cpp
> > @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
> > }
> > }
> >
> > -int EncoderLibJpeg::encode(const FrameBuffer *source,
> > - const libcamera::Span<uint8_t> &dest,
> > +int EncoderLibJpeg::encode(const FrameBuffer &source,
> > + libcamera::Span<uint8_t> dest,
> > const libcamera::Span<const uint8_t> &exifData)
> > {
> > - MappedFrameBuffer frame(source, PROT_READ);
> > + MappedFrameBuffer frame(&source, PROT_READ);
> > if (!frame.isValid()) {
> > LOG(JPEG, Error) << "Failed to map FrameBuffer : "
> > << strerror(frame.error());
> > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> > index 934caef..391a53c 100644
> > --- a/src/android/jpeg/encoder_libjpeg.h
> > +++ b/src/android/jpeg/encoder_libjpeg.h
> > @@ -21,8 +21,8 @@ public:
> > ~EncoderLibJpeg();
> >
> > int configure(const libcamera::StreamConfiguration &cfg) override;
> > - int encode(const libcamera::FrameBuffer *source,
> > - const libcamera::Span<uint8_t> &destination,
> > + int encode(const libcamera::FrameBuffer &source,
> > + libcamera::Span<uint8_t> destination,
> > const libcamera::Span<const uint8_t> &exifData) override;
> >
> > private:
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > index 753c28e..4ded3e3 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_)
> > @@ -64,8 +64,10 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
> > * second, it is good enough.
> > */
> > exif.setTimestamp(std::time(nullptr));
> > - if (exif.generate() != 0)
> > + if (exif.generate() != 0) {
> > LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> > + return -EINVAL;
> > + }
> >
>
> Personally speaking, I don't think we should return FAIL here. So my
> preference is to skip this change.
Tomasz' recommendation of aligning this with Android's requirements
makes sense I think.
> > int jpeg_size = encoder_->encode(source, destination, exif.data());
> > if (jpeg_size < 0) {
> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > index 62c8650..4d8edf5 100644
> > --- a/src/android/jpeg/post_processor_jpeg.h
> > +++ b/src/android/jpeg/post_processor_jpeg.h
> > @@ -23,12 +23,12 @@ 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:
> > - CameraDevice *cameraDevice_;
> > + CameraDevice const *cameraDevice_;
> > std::unique_ptr<Encoder> encoder_;
> > libcamera::Size streamSize_;
> > };
> > 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