[libcamera-devel] [PATCH] android: Modify PostProcessor interface
Hirokazu Honda
hiroh at chromium.org
Tue Oct 20 08:51:25 CEST 2020
Thanks Laurent and Tomasz,
On Mon, Oct 19, 2020 at 9:58 PM Laurent Pinchart
<laurent.pinchart at 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 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.
>
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
More information about the libcamera-devel
mailing list