[PATCH v2 8/9] android: Add JpegExifMetadata to store tags setting into Exif
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Dec 2 18:04:10 CET 2024
Hi Harvey
On Fri, Nov 29, 2024 at 05:17:30PM +0800, Cheng-Hao Yang wrote:
> 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'm not sure I get this comment in full. I was suggesting to use
AnalogueGain directly here, but if it isn't required, I'm fine with
keeping a \todo
>
> 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?
yeah, what I meant was "if you ASSERT()" you expect it to be always
populated, so std::optional<> doesn't bring any value. But you
probably want to make sure that jpegExifMetadata has been populated
when this function is called, so feel free to keep std::optional<>
>
> 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.
Is this an issue about threading or about the order in which metadata
and buffer completes ?
I see in the next patch you call generateJpegExifMetadata() on a
((Mapped|Internal) && Jpeg) bufferComplete event.
https://gitlab.freedesktop.org/camera/libcamera/-/commit/b731fe488badef2861da914913290e16afb716c8#88faf21e943da09c94a5b31cd420d91c35371290_1193_1227
And immediately after, if such a buffer has been completed, you call
process() on it
https://gitlab.freedesktop.org/camera/libcamera/-/commit/b731fe488badef2861da914913290e16afb716c8#88faf21e943da09c94a5b31cd420d91c35371290_1193_1251
Is this right ?
So when the buffer completes, you inspect the so-far-completed
metadata and extract ExposureTime (and AnalogueGain eventually).
1) Why you don't do that at metadataAvailable time ?
2) What does guarantee that the pipeline has populated ExposureTime at
the time the buffer to process has completed ?
> >
> > WDYT?
> >
> > [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/request.cpp#n530
> >
I might have missed why this is related :)
> > >
> > > 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