[libcamera-devel] android: Modify PostProcessor interface
diff mbox series

Message ID 20201019080323.2236548-1-hiroh@chromium.org
State Accepted
Headers show
Series
  • [libcamera-devel] android: Modify PostProcessor interface
Related show

Commit Message

Hirokazu Honda Oct. 19, 2020, 8:03 a.m. UTC 
This modifies PostProcessor interface and polishes the code
around the PostProcessor.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 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(-)

Comments

Umang Jain Oct. 19, 2020, 11:24 a.m. UTC | #1 
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@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.
> ---
>   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.
>   	  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() {}
>   
>   	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.
>   	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;
>   };
>
Hirokazu Honda Oct. 19, 2020, 11:44 a.m. UTC | #2 
Hi Umang,

On Mon, Oct 19, 2020 at 8:24 PM Umang Jain <email@uajain.com> 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@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.

Ack. I will do it in the next patch series.

> > ---
> >   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.

I expect moving here out of body of constructor doesn't make a
difference and thus the answer to your question should be true.

> >         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?

Since I change CameraDevice_ to const raw pointer, I need to pass
non-const raw pointer (cameraDevice, not cameraDevice"_") to
PostProcessorJpeg.
We can change PostProcessor and CameraStream constructors to receive
const raw pointer.

> >       }
> >
> >       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() {}
> >
> >       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.

Thanks. Kieran also told me that in another mail thread.
I will do so in the next patch series.

Regards,
-Hiro

> >       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;
> >   };
> >
>
Tomasz Figa Oct. 19, 2020, 11:55 a.m. UTC | #3 
On Mon, Oct 19, 2020 at 1:24 PM Umang Jain <email@uajain.com> 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@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.

+1

I'd also see rationale explained for each change.

> > ---
> >   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.
> >         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() {}
> >
> >       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.

I'd consider this to be governed by Android expectations.

Hiro, did you base on some kind of documented expectations when making
this change? If not, would you mind researching what's required for
compliance here? Thanks!

Best regards,
Tomasz
Laurent Pinchart Oct. 19, 2020, 12:57 p.m. UTC | #4 
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@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;
> >   };
> >
Hirokazu Honda Oct. 20, 2020, 6:51 a.m. UTC | #5 
Thanks Laurent and Tomasz,

On Mon, Oct 19, 2020 at 9:58 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@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.
>

I submitted a patch series of removing extra semicolons widely in libcamera.
I will not touch this in this patch therefore.

> > >
> > >     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.
>

Re Android standard, I saw in CaptureRequest documentation [1] that
the exif values affect the aspects of the captured image.
In fact, there is a CTS test to check exif metadata [2].
Searching the android codebase, a couple of implementations [3, 4]
handle the exif failure as a capture fatal BUT a few implementations
[5, 6] also ignore the failure.
I consider this should be treated as failure?

[1] https://developer.android.com/reference/android/hardware/camera2/CaptureRequest
[2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/src/android/hardware/camera2/cts/StillCaptureTest.java;l=690;drc=master
[3] https://cs.android.com/android/platform/superproject/+/master:hardware/libhardware/modules/camera/3_4/arc/image_processor.cpp;l=381;drc=9dae907ea07692118698a242fe4a7427283bd10a
[4] https://cs.android.com/android/platform/superproject/+/master:frameworks/av/services/camera/libcameraservice/api2/HeicCompositeStream.cpp;l=999;drc=991b7b672203d79b66ba7fcb672cb885d7947ae1
[5] https://cs.android.com/android/platform/superproject/+/master:frameworks/av/services/camera/libcameraservice/common/DepthPhotoProcessor.cpp;l=211;drc=29e9ec182d20f44ee2e8a15de1940cb4ef29663e
[6] https://cs.android.com/android/platform/superproject/+/master:hardware/google/camera/devices/EmulatedCamera/hwl/JpegCompressor.cpp;l=314;drc=master

Best Regards,
-Hiro
> > >     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
Laurent Pinchart Oct. 22, 2020, 4:15 a.m. UTC | #6 
Hi Hiro-san,

On Tue, Oct 20, 2020 at 03:51:25PM +0900, Hirokazu Honda wrote:
> On Mon, Oct 19, 2020 at 9:58 PM Laurent Pinchart wrote:
> >
> > 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@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.
> >
> 
> I submitted a patch series of removing extra semicolons widely in libcamera.
> I will not touch this in this patch therefore.
> 
> > > >
> > > >     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.
> 
> Re Android standard, I saw in CaptureRequest documentation [1] that
> the exif values affect the aspects of the captured image.
> In fact, there is a CTS test to check exif metadata [2].
> Searching the android codebase, a couple of implementations [3, 4]
> handle the exif failure as a capture fatal BUT a few implementations
> [5, 6] also ignore the failure.
> I consider this should be treated as failure?

This shouldn't fail in normal circumstances, so I don't really see a
need for a degraded mode without Exif data as different parts of the
Android stack may choke on this. On the other hand, as it's not more
costly for us to handle the failure gracefully, and as it may still
work, we could consider this as non-fatal.  I'm fine with either option.

> [1] https://developer.android.com/reference/android/hardware/camera2/CaptureRequest
> [2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/src/android/hardware/camera2/cts/StillCaptureTest.java;l=690;drc=master
> [3] https://cs.android.com/android/platform/superproject/+/master:hardware/libhardware/modules/camera/3_4/arc/image_processor.cpp;l=381;drc=9dae907ea07692118698a242fe4a7427283bd10a
> [4] https://cs.android.com/android/platform/superproject/+/master:frameworks/av/services/camera/libcameraservice/api2/HeicCompositeStream.cpp;l=999;drc=991b7b672203d79b66ba7fcb672cb885d7947ae1
> [5] https://cs.android.com/android/platform/superproject/+/master:frameworks/av/services/camera/libcameraservice/common/DepthPhotoProcessor.cpp;l=211;drc=29e9ec182d20f44ee2e8a15de1940cb4ef29663e
> [6] https://cs.android.com/android/platform/superproject/+/master:hardware/google/camera/devices/EmulatedCamera/hwl/JpegCompressor.cpp;l=314;drc=master
> 
> > > >     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;
> > > >   };
> > > >
Hirokazu Honda Oct. 22, 2020, 4:51 a.m. UTC | #7 
On Thu, Oct 22, 2020 at 1:15 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro-san,
>
> On Tue, Oct 20, 2020 at 03:51:25PM +0900, Hirokazu Honda wrote:
> > On Mon, Oct 19, 2020 at 9:58 PM Laurent Pinchart wrote:
> > >
> > > 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@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.
> > >
> >
> > I submitted a patch series of removing extra semicolons widely in libcamera.
> > I will not touch this in this patch therefore.
> >
> > > > >
> > > > >     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.
> >
> > Re Android standard, I saw in CaptureRequest documentation [1] that
> > the exif values affect the aspects of the captured image.
> > In fact, there is a CTS test to check exif metadata [2].
> > Searching the android codebase, a couple of implementations [3, 4]
> > handle the exif failure as a capture fatal BUT a few implementations
> > [5, 6] also ignore the failure.
> > I consider this should be treated as failure?
>
> This shouldn't fail in normal circumstances, so I don't really see a
> need for a degraded mode without Exif data as different parts of the
> Android stack may choke on this. On the other hand, as it's not more
> costly for us to handle the failure gracefully, and as it may still
> work, we could consider this as non-fatal.  I'm fine with either option.
>

I got it.
I don't treat it as fatal.

Best Regards,
-Hiro
> > [1] https://developer.android.com/reference/android/hardware/camera2/CaptureRequest
> > [2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/src/android/hardware/camera2/cts/StillCaptureTest.java;l=690;drc=master
> > [3] https://cs.android.com/android/platform/superproject/+/master:hardware/libhardware/modules/camera/3_4/arc/image_processor.cpp;l=381;drc=9dae907ea07692118698a242fe4a7427283bd10a
> > [4] https://cs.android.com/android/platform/superproject/+/master:frameworks/av/services/camera/libcameraservice/api2/HeicCompositeStream.cpp;l=999;drc=991b7b672203d79b66ba7fcb672cb885d7947ae1
> > [5] https://cs.android.com/android/platform/superproject/+/master:frameworks/av/services/camera/libcameraservice/common/DepthPhotoProcessor.cpp;l=211;drc=29e9ec182d20f44ee2e8a15de1940cb4ef29663e
> > [6] https://cs.android.com/android/platform/superproject/+/master:hardware/google/camera/devices/EmulatedCamera/hwl/JpegCompressor.cpp;l=314;drc=master
> >
> > > > >     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

Patch
diff mbox series 

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),
 	  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);
 	}
 
 	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() {}
 
 	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;
+	}
 
 	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;
 };