[libcamera-devel] [PATCH] android: Modify PostProcessor interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Oct 22 06:15:07 CEST 2020
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 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?
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;
> > > > };
> > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list