mirror of
https://git.libcamera.org/libcamera/libcamera.git
synced 2025-07-13 15:29:45 +03:00
c50065c9eb
To enable the Simple Soft ISP and Soft IPA for simple pipeline handler configure the build with: -Dpipelines=simple -Dipas=simple Also using the Soft ISP for the particular hardware platform must be enabled in the supportedDevices[] table. It is currently enabled for and only for qcom-camss. If the pipeline uses Converter, Soft ISP and Soft IPA aren't available. Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s Tested-by: Pavel Machek <pavel@ucw.cz> Reviewed-by: Pavel Machek <pavel@ucw.cz> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
269 lines
9 KiB
Text
269 lines
9 KiB
Text
1. Setting F_SEAL_SHRINK and F_SEAL_GROW after ftruncate()
|
|
|
|
>> SharedMem::SharedMem(const std::string &name, std::size_t size)
|
|
>> : name_(name), size_(size), mem_(nullptr)
|
|
>>
|
|
>> ...
|
|
>>
|
|
>> if (ftruncate(fd_.get(), size_) < 0)
|
|
>> return;
|
|
>
|
|
> Should we set the GROW and SHRINK seals (in a separate patch) ?
|
|
|
|
Yes, this can be done.
|
|
Setting F_SEAL_SHRINK and F_SEAL_GROW after the ftruncate() call above could catch
|
|
some potential errors related to improper access to the shared memory allocated by
|
|
the SharedMemObject.
|
|
|
|
---
|
|
|
|
2. Reconsider stats sharing
|
|
|
|
>>> +void SwStatsCpu::finishFrame(void)
|
|
>>> +{
|
|
>>> + *sharedStats_ = stats_;
|
|
>>
|
|
>> Is it more efficient to copy the stats instead of operating directly on
|
|
>> the shared memory ?
|
|
>
|
|
> I inherited doing things this way from Andrey. I kept this because
|
|
> we don't really have any synchronization with the IPA reading this.
|
|
>
|
|
> So the idea is to only touch this when the next set of statistics
|
|
> is ready since we don't know when the IPA is done with accessing
|
|
> the previous set of statistics ...
|
|
>
|
|
> This is both something which seems mostly a theoretic problem,
|
|
> yet also definitely something which I think we need to fix.
|
|
>
|
|
> Maybe use a ringbuffer of stats buffers and pass the index into
|
|
> the ringbuffer to the emit signal ?
|
|
|
|
That would match how we deal with hardware ISPs, and I think that's a
|
|
good idea. It will help decoupling the processing side from the IPA.
|
|
|
|
---
|
|
|
|
3. Remove statsReady signal
|
|
|
|
> class SwStatsCpu
|
|
> {
|
|
> /**
|
|
> * \brief Signals that the statistics are ready
|
|
> */
|
|
> Signal<> statsReady;
|
|
|
|
But better, I wonder if the signal could be dropped completely. The
|
|
SwStatsCpu class does not operate asynchronously. Shouldn't whoever
|
|
calls the finishFrame() function then handle emitting the signal ?
|
|
|
|
Now, the trouble is that this would be the DebayerCpu class, whose name
|
|
doesn't indicate as a prime candidate to handle stats. However, it
|
|
already exposes a getStatsFD() function, so we're already calling for
|
|
trouble :-) Either that should be moved to somewhere else, or the class
|
|
should be renamed. Considering that the class applies colour gains in
|
|
addition to performing the interpolation, it may be more of a naming
|
|
issue.
|
|
|
|
Removing the signal and refactoring those classes doesn't have to be
|
|
addressed now, I think it would be part of a larger refactoring
|
|
(possibly also considering platforms that have no ISP but can produce
|
|
stats in hardware, such as the i.MX7), but please keep it on your radar.
|
|
|
|
---
|
|
|
|
4. Hide internal representation of gains from callers
|
|
|
|
> struct DebayerParams {
|
|
> static constexpr unsigned int kGain10 = 256;
|
|
|
|
Forcing the caller to deal with the internal representation of gains
|
|
isn't nice, especially given that it precludes implementing gains of
|
|
different precisions in different backend. Wouldn't it be better to pass
|
|
the values as floating point numbers, and convert them to the internal
|
|
representation in the implementation of process() before using them ?
|
|
|
|
---
|
|
|
|
5. Store ISP parameters in per-frame buffers
|
|
|
|
> /**
|
|
> * \fn void Debayer::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)
|
|
> * \brief Process the bayer data into the requested format.
|
|
> * \param[in] input The input buffer.
|
|
> * \param[in] output The output buffer.
|
|
> * \param[in] params The parameters to be used in debayering.
|
|
> *
|
|
> * \note DebayerParams is passed by value deliberately so that a copy is passed
|
|
> * when this is run in another thread by invokeMethod().
|
|
> */
|
|
|
|
Possibly something to address later, by storing ISP parameters in
|
|
per-frame buffers like we do for hardware ISPs.
|
|
|
|
---
|
|
|
|
6. Input buffer copying configuration
|
|
|
|
> DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
|
|
> : stats_(std::move(stats)), gammaCorrection_(1.0)
|
|
> {
|
|
> enableInputMemcpy_ = true;
|
|
|
|
Set this appropriately and/or make it configurable.
|
|
|
|
---
|
|
|
|
7. Performance measurement configuration
|
|
|
|
> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)
|
|
> /* Measure before emitting signals */
|
|
> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
|
|
> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
|
|
> timespec frameEndTime = {};
|
|
> clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);
|
|
> frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);
|
|
> if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {
|
|
> const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -
|
|
> DebayerCpu::kFramesToSkip;
|
|
> LOG(Debayer, Info)
|
|
> << "Processed " << measuredFrames
|
|
> << " frames in " << frameProcessTime_ / 1000 << "us, "
|
|
> << frameProcessTime_ / (1000 * measuredFrames)
|
|
> << " us/frame";
|
|
> }
|
|
> }
|
|
|
|
I wonder if there would be a way to control at runtime when/how to
|
|
perform those measurements. Maybe that's a bit overkill.
|
|
|
|
---
|
|
|
|
8. DebayerCpu cleanups
|
|
|
|
> >> class DebayerCpu : public Debayer, public Object
|
|
> >> const SharedFD &getStatsFD() { return stats_->getStatsFD(); }
|
|
> >
|
|
> > This,
|
|
>
|
|
> Note the statistics pass-through stuff is sort of a necessary evil
|
|
> since we want one main loop going over the data line by line and
|
|
> doing both debayering as well as stats while the line is still
|
|
> hot in the l2 cache. And things like the process2() and process4()
|
|
> loops are highly CPU debayering specific so I don't think we should
|
|
> move those out of the CpuDebayer code.
|
|
|
|
Yes, that I understood from the review. "necessary evil" is indeed the
|
|
right term :-) I expect it will take quite some design skills to balance
|
|
the need for performances and the need for a maintainable architecture.
|
|
|
|
> > plus the fact that this class handles colour gains and gamma,
|
|
> > makes me thing we have either a naming issue, or an architecture issue.
|
|
>
|
|
> I agree that this does a bit more then debayering, although
|
|
> the debayering really is the main thing it does.
|
|
>
|
|
> I guess the calculation of the rgb lookup tables which do the
|
|
> color gains and gamma could be moved outside of this class,
|
|
> that might even be beneficial for GPU based debayering assuming
|
|
> that that is going to use rgb lookup tables too (it could
|
|
> implement actual color gains + gamma correction in some different
|
|
> way).
|
|
>
|
|
> I think this falls under the lets wait until we have a GPU
|
|
> based SoftISP MVP/POC and then do some refactoring to see which
|
|
> bits should go where.
|
|
|
|
---
|
|
|
|
8. Decouple pipeline and IPA naming
|
|
|
|
> The current src/ipa/meson.build assumes the IPA name to match the
|
|
> pipeline name. For this reason "-Dipas=simple" is used for the
|
|
> Soft IPA module.
|
|
|
|
This should be addressed.
|
|
|
|
---
|
|
|
|
9. Doxyfile cleanup
|
|
|
|
>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
|
|
>> index a86ea6c1..2be8d47b 100644
|
|
>> --- a/Documentation/Doxyfile.in
|
|
>> +++ b/Documentation/Doxyfile.in
|
|
>> @@ -44,6 +44,7 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/base/span.h \
|
|
>> @TOP_SRCDIR@/src/libcamera/pipeline/ \
|
|
>> @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
|
|
>> @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
|
|
>> + @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \
|
|
> Why is this needed ?
|
|
>
|
|
>> @TOP_BUILDDIR@/src/libcamera/proxy/
|
|
>> EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
|
|
>> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
|
|
>> index f3b4881c..3352d08f 100644
|
|
>> --- a/include/libcamera/ipa/meson.build
|
|
>> +++ b/include/libcamera/ipa/meson.build
|
|
>> @@ -65,6 +65,7 @@ pipeline_ipa_mojom_mapping = {
|
|
>> 'ipu3': 'ipu3.mojom',
|
|
>> 'rkisp1': 'rkisp1.mojom',
|
|
>> 'rpi/vc4': 'raspberrypi.mojom',
|
|
>> + 'simple': 'soft.mojom',
|
|
>> 'vimc': 'vimc.mojom',
|
|
>> }
|
|
>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
|
|
>> new file mode 100644
|
|
>> index 00000000..c249bd75
|
|
>> --- /dev/null
|
|
>> +++ b/include/libcamera/ipa/soft.mojom
|
|
>> @@ -0,0 +1,28 @@
|
|
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
|
|
>> +
|
|
>> +/*
|
|
>> + * \todo Document the interface and remove the related EXCLUDE_PATTERNS entry.
|
|
> Ah that's why.
|
|
|
|
Yes, because, well... all the other IPAs were doing that...
|
|
|
|
> It doesn't have to be done before merging, but could you
|
|
> address this sooner than later ?
|
|
|
|
---
|
|
|
|
10. Switch to libipa/algorithm.h API in processStats
|
|
|
|
>> void IPASoftSimple::processStats(const ControlList &sensorControls)
|
|
>>
|
|
> Do you envision switching to the libipa/algorithm.h API at some point ?
|
|
|
|
At some point, yes.
|
|
|
|
---
|
|
|
|
11. Improve handling the sensor controls which take effect with a delay
|
|
|
|
> void IPASoftSimple::processStats(const ControlList &sensorControls)
|
|
> {
|
|
> ...
|
|
> /*
|
|
> * AE / AGC, use 2 frames delay to make sure that the exposure and
|
|
> * the gain set have applied to the camera sensor.
|
|
> */
|
|
> if (ignore_updates_ > 0) {
|
|
> --ignore_updates_;
|
|
> return;
|
|
> }
|
|
|
|
This could be handled better with DelayedControls.
|
|
|
|
---
|
|
|
|
12. Use DelayedControls class in ispStatsReady()
|
|
|
|
> void SimpleCameraData::ispStatsReady()
|
|
> {
|
|
> swIsp_->processStats(sensor_->getControls({ V4L2_CID_ANALOGUE_GAIN,
|
|
> V4L2_CID_EXPOSURE }));
|
|
|
|
You should use the DelayedControls class.
|