[PATCH v2 8/9] android: Add JpegExifMetadata to store tags setting into Exif
Cheng-Hao Yang
chenghaoyang at chromium.org
Fri Nov 29 10:17:30 CET 2024
Hi Jacopo,
On Fri, Nov 29, 2024 at 5:05 PM Cheng-Hao Yang
<chenghaoyang at chromium.org> wrote:
>
> Hi Jacopo,
>
> On Thu, Nov 28, 2024 at 11:35 PM Jacopo Mondi
> <jacopo.mondi at ideasonboard.com> wrote:
> >
> > Hi Harvey
> >
> > On Wed, Nov 27, 2024 at 09:25:58AM +0000, Harvey Yang wrote:
> > > From: Han-Lin Chen <hanlinchen at chromium.org>
> > >
> > > With partial result, some metadata, which needs to be added into Exif,
> > > may be sent back to framework earlier before Jpeg post-processing.
> > > Add a type JpegExifMetadata associated with StreamBuffer to store the values,
> > > so Jpeg post-processing doesn't need to reference to current metadata.
> > >
> > > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > ---
> > > src/android/camera_device.cpp | 26 ++++++++++++++++++++++++
> > > src/android/camera_device.h | 2 ++
> > > src/android/camera_request.h | 6 ++++++
> > > src/android/camera_stream.h | 4 ++++
> > > src/android/jpeg/post_processor_jpeg.cpp | 12 ++++++-----
> > > 5 files changed, 45 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 9fd851bc8..e085e18b2 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -1250,6 +1250,10 @@ void CameraDevice::requestComplete(Request *request)
> > > CameraStream *stream = iter->first;
> > > StreamBuffer *buffer = iter->second;
> > >
> > > + if (stream->isJpegStream()) {
> > > + generateJpegExifMetadata(descriptor, buffer);
> > > + }
> > > +
> >
> > no {} for single line statements
Done
> >
> > > FrameBuffer *src = request->findBuffer(stream->stream());
> > > if (!src) {
> > > LOG(HAL, Error) << "Failed to find a source stream buffer";
> > > @@ -1443,6 +1447,28 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> > > callbacks_->notify(callbacks_, ¬ify);
> > > }
> > >
> > > +/*
> > > + * Set jpeg metadata used to generate EXIF in the JPEG post processing.
> > > + */
> > > +void CameraDevice::generateJpegExifMetadata(Camera3RequestDescriptor *request,
> > > + StreamBuffer *buffer) const
> > > +{
> > > + const ControlList &metadata = request->request_->metadata();
> > > + auto &jpegExifMetadata = buffer->jpegExifMetadata;
> > > + jpegExifMetadata.emplace(StreamBuffer::JpegExifMetadata());
> > > +
> > > + const int64_t exposureTime = metadata.get(controls::ExposureTime).value_or(0);
> > > + jpegExifMetadata->sensorExposureTime = exposureTime;
> > > +
> > > + /*
> > > + * todo: Android Sensitivity should only include analog gain X digital
> >
> > \todo
done
> >
> > > + * gain from sensor. Digital gain on ISP shouldn't be included.
> >
> > mmm, I guess how the gain is split between analogue and digital on the
> > sensor is up to the IPA implementation, and currently I only see vc4
> > handling it and it sets it on the ISP.
> >
> > I wonder if you couldn't simply use AnalogueGain here
I think the comment here is assuming the further changes that use
AnalogueGain directly here, while might not be needed in this patch...
Removed.
I'll remember (hopefully) when we use AnalogueGain here in the
following patches.
> >
> > > + * Calculate sensitivity accordingly when we can differentiate
> > > + * the source of digital gains.
> > > + */
> > > + jpegExifMetadata->sensorSensitivityISO = 100;
> > > +}
> > > +
> > > /*
> > > * Produce a set of fixed result metadata.
> > > */
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index 815a695d1..3c46ff918 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -102,6 +102,8 @@ private:
> > > void sendCaptureResult(Camera3RequestDescriptor *request) const;
> > > void setBufferStatus(StreamBuffer &buffer,
> > > StreamBuffer::Status status);
> > > + void generateJpegExifMetadata(Camera3RequestDescriptor *request,
> > > + StreamBuffer *buffer) const;
> > > std::unique_ptr<CameraMetadata> getResultMetadata(
> > > const Camera3RequestDescriptor &descriptor) const;
> > >
> > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> > > index bd75d4595..bd87b36fd 100644
> > > --- a/src/android/camera_request.h
> > > +++ b/src/android/camera_request.h
> > > @@ -44,6 +44,11 @@ public:
> > > StreamBuffer(StreamBuffer &&);
> > > StreamBuffer &operator=(StreamBuffer &&);
> > >
> > > + struct JpegExifMetadata {
> > > + int64_t sensorExposureTime;
> > > + int32_t sensorSensitivityISO;
> > > + };
> > > +
> > > CameraStream *stream;
> > > buffer_handle_t *camera3Buffer;
> > > std::unique_ptr<HALFrameBuffer> frameBuffer;
> > > @@ -51,6 +56,7 @@ public:
> > > Status status = Status::Success;
> > > const libcamera::FrameBuffer *srcBuffer = nullptr;
> > > std::unique_ptr<CameraBuffer> dstBuffer;
> > > + std::optional<JpegExifMetadata> jpegExifMetadata;
> > > Camera3RequestDescriptor *request;
> > >
> > > private:
> > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > > index 30f64f690..47cd7ab85 100644
> > > --- a/src/android/camera_stream.h
> > > +++ b/src/android/camera_stream.h
> > > @@ -125,6 +125,10 @@ public:
> > > const libcamera::StreamConfiguration &configuration() const;
> > > libcamera::Stream *stream() const;
> > > CameraStream *sourceStream() const { return sourceStream_; }
> > > + bool isJpegStream() const
> > > + {
> > > + return camera3Stream_->format == HAL_PIXEL_FORMAT_BLOB;
> > > + }
> > >
> > > int configure();
> > > int process(StreamBuffer *streamBuffer);
> > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > > index f5a90785d..48782b574 100644
> > > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > > @@ -112,8 +112,11 @@ void PostProcessorJpeg::process(StreamBuffer *streamBuffer)
> > >
> > > const FrameBuffer &source = *streamBuffer->srcBuffer;
> > > CameraBuffer *destination = streamBuffer->dstBuffer.get();
> > > + const std::optional<StreamBuffer::JpegExifMetadata> &jpegExifMetadata =
> > > + streamBuffer->jpegExifMetadata;
> > >
> > > ASSERT(destination->numPlanes() == 1);
> > > + ASSERT(jpegExifMetadata.has_value());
> >
> > This means it's not optional, isn't it ?
Means it's not std::nullopt. Any suggestions?
BR,
Harvey
> >
> > >
> > > const CameraMetadata &requestMetadata = streamBuffer->request->settings_;
> > > CameraMetadata *resultMetadata = streamBuffer->request->resultMetadata_.get();
> > > @@ -139,15 +142,14 @@ void PostProcessorJpeg::process(StreamBuffer *streamBuffer)
> > > */
> > > exif.setTimestamp(std::time(nullptr), 0ms);
> > >
> > > - ret = resultMetadata->getEntry(ANDROID_SENSOR_EXPOSURE_TIME, &entry);
> > > - exif.setExposureTime(ret ? *entry.data.i64 : 0);
> > > + /* Exif requires nsec for exposure time */
> > > + exif.setExposureTime(jpegExifMetadata->sensorExposureTime * 1000);
> > > + exif.setISO(jpegExifMetadata->sensorSensitivityISO);
> > > +
> >
> > StreamBuffer has a pointer to the Camera3RequestDescriptor it belongs
> > to. From there you could get the Request metadata as you currently do
> > in CameraDevice::generateJpegExifMetadata().
>
> That's a very good question, only that I wonder how we can handle the
> threading issue properly.
>
> The current implementation of `Request::metadata()` [1] doesn't seem
> to consider race conditions, and our goal is to support partial results,
> which means that the post processor thread might try to access metadata
> when the request is still being processed in the pipeline handler, which
> might set further metadata tags.
>
> WDYT?
>
> [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/request.cpp#n530
>
> >
> > What is the advantage of caching the jpegExifMetadata at
> > CameraDevice::requestComplete() time ?
> >
> > Thanks
> > j
> >
> > > ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
> > > if (ret)
> > > exif.setAperture(*entry.data.f);
> > >
> > > - ret = resultMetadata->getEntry(ANDROID_SENSOR_SENSITIVITY, &entry);
> > > - exif.setISO(ret ? *entry.data.i32 : 100);
> > > -
> > > exif.setFlash(Exif::Flash::FlashNotPresent);
> > > exif.setWhiteBalance(Exif::WhiteBalance::Auto);
> > >
> > > --
> > > 2.47.0.338.g60cca15819-goog
> > >
More information about the libcamera-devel
mailing list