[libcamera-devel] [PATCH] android: Modify PostProcessor interface
Hirokazu Honda
hiroh at chromium.org
Thu Oct 22 06:51:24 CEST 2020
On Thu, Oct 22, 2020 at 1:15 PM Laurent Pinchart
<laurent.pinchart at 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 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.
>
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
More information about the libcamera-devel
mailing list