| Message ID | 20261021080000.16624-2-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi, On 21.10.26 09:59, Milan Zamazal wrote: > Currently, the decision whether to compute stats for a given frame is > implemented in SwStatsCpu in several places and duplicated in > debayer_egl.cpp. Let's decide it solely in SwStatsCpu. This will also > help if a more flexible or complex update frequency is introduced, > e.g. to support stats for autofocus. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > .../internal/software_isp/swstats_cpu.h | 17 ++++++------- > src/libcamera/software_isp/debayer_egl.cpp | 24 ++++++++----------- > src/libcamera/software_isp/swstats_cpu.cpp | 16 ++++++++++--- > 3 files changed, 32 insertions(+), 25 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h > index 551870921..cd5303a51 100644 > --- a/include/libcamera/internal/software_isp/swstats_cpu.h > +++ b/include/libcamera/internal/software_isp/swstats_cpu.h > @@ -38,14 +38,6 @@ public: > SwStatsCpu(const CameraManager &cm); > ~SwStatsCpu() = default; > > - /* > - * The combination of pipeline + sensor delays means that > - * exposure changes can take up to 3 frames to get applied, > - * Run stats once every 4 frames to ensure any previous > - * exposure changes have been applied. > - */ > - static constexpr uint32_t kStatPerNumFrames = 4; > - > bool isValid() const { return sharedStats_.fd().isValid(); } > > const SharedFD &getStatsFD() { return sharedStats_.fd(); } > @@ -54,6 +46,7 @@ public: > > int configure(const StreamConfiguration &inputCfg, unsigned int statsBufferCount = 1); > void setWindow(const Rectangle &window); > + bool updating(uint32_t frame); > void startFrame(uint32_t frame); > void finishFrame(uint32_t frame, uint32_t bufferId); > void processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input); > @@ -88,6 +81,14 @@ private: > using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[], SwIspStats &stats); > using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in); > > + /* > + * The combination of pipeline + sensor delays means that > + * exposure changes can take up to 3 frames to get applied, > + * Run stats once every 4 frames to ensure any previous > + * exposure changes have been applied. > + */ > + static constexpr uint32_t kStatPerNumFrames = 4; > + > int setupStandardBayerOrder(BayerFormat::Order order); > /* Bayer 8 bpp unpacked */ > void statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats); > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > index 300822f9d..1a9dc1841 100644 > --- a/src/libcamera/software_isp/debayer_egl.cpp > +++ b/src/libcamera/software_isp/debayer_egl.cpp > @@ -630,21 +630,17 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output > metadata.planes()[0].bytesused = output->planes()[0].length; > > /* Calculate stats for the whole frame */ > - if (frame % SwStatsCpu::kStatPerNumFrames) { > - stats_->finishFrame(frame, 0); > - } else { > - if (!inMapped) { > - /* > - * The buffer was directly imported into EGL and thus > - * not mapped for texture upload. Do it now for the > - * CPU-based stats calculation. > - */ > - assert(!inDmaSyncer); > - inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read); > - inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read); > - } > - stats_->processFrame(frame, 0, inMapped.value()); > + if (stats_->updating(frame) && !inMapped) { > + /* > + * The buffer was directly imported into EGL and thus > + * not mapped for texture upload. Do it now for the > + * CPU-based stats calculation. > + */ > + assert(!inDmaSyncer); > + inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read); > + inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read); > } > + stats_->processFrame(frame, 0, inMapped.value()); This unfortunately doesn't work: in the "!stats_->updating(frame) && !inMapped" case - e.g. on the second frame if dmabuf import was successful - inMapped is not set and we crash. That's why I went with the somewhat ugly previous solution. > inDmaSyncer.reset(); > > egl_.syncOutput(); > diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp > index 7fb77ce7d..362fb6a86 100644 > --- a/src/libcamera/software_isp/swstats_cpu.cpp > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > @@ -383,7 +383,7 @@ void SwStatsCpu::statsGBRG12PLine0(const uint8_t *src[], SwIspStats &stats) > */ > void SwStatsCpu::startFrame(uint32_t frame) > { > - if (frame % kStatPerNumFrames) > + if (!updating(frame)) > return; > > if (window_.width == 0) > @@ -404,7 +404,7 @@ void SwStatsCpu::startFrame(uint32_t frame) > */ > void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) > { > - bool valid = frame % kStatPerNumFrames == 0; > + bool valid = updating(frame); > > if (valid) { > sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); > @@ -604,7 +604,7 @@ void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in) > */ > void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input) > { > - if (frame % kStatPerNumFrames) { > + if (!updating(frame)) { > finishFrame(frame, bufferId); > return; > } > @@ -616,4 +616,14 @@ void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuff > bench_.finishFrame(); > } > > +/** > + * \brief Determine whether stats update is required for the given frame > + * \param[in] frame The frame number > + * \return Whether the stats are updated for the given frame > + */ > +bool SwStatsCpu::updating(uint32_t frame) > +{ > + return frame % kStatPerNumFrames == 0; > +} > + > } /* namespace libcamera */
Hi Robert, thank you for review. Robert Mader <robert.mader@collabora.com> writes: > Hi, > > On 21.10.26 09:59, Milan Zamazal wrote: >> Currently, the decision whether to compute stats for a given frame is >> implemented in SwStatsCpu in several places and duplicated in >> debayer_egl.cpp. Let's decide it solely in SwStatsCpu. This will also >> help if a more flexible or complex update frequency is introduced, >> e.g. to support stats for autofocus. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> .../internal/software_isp/swstats_cpu.h | 17 ++++++------- >> src/libcamera/software_isp/debayer_egl.cpp | 24 ++++++++----------- >> src/libcamera/software_isp/swstats_cpu.cpp | 16 ++++++++++--- >> 3 files changed, 32 insertions(+), 25 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h >> index 551870921..cd5303a51 100644 >> --- a/include/libcamera/internal/software_isp/swstats_cpu.h >> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h >> @@ -38,14 +38,6 @@ public: >> SwStatsCpu(const CameraManager &cm); >> ~SwStatsCpu() = default; >> - /* >> - * The combination of pipeline + sensor delays means that >> - * exposure changes can take up to 3 frames to get applied, >> - * Run stats once every 4 frames to ensure any previous >> - * exposure changes have been applied. >> - */ >> - static constexpr uint32_t kStatPerNumFrames = 4; >> - >> bool isValid() const { return sharedStats_.fd().isValid(); } >> const SharedFD &getStatsFD() { return sharedStats_.fd(); } >> @@ -54,6 +46,7 @@ public: >> int configure(const StreamConfiguration &inputCfg, unsigned int >> statsBufferCount = 1); >> void setWindow(const Rectangle &window); >> + bool updating(uint32_t frame); >> void startFrame(uint32_t frame); >> void finishFrame(uint32_t frame, uint32_t bufferId); >> void processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input); >> @@ -88,6 +81,14 @@ private: >> using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[], SwIspStats &stats); >> using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in); >> + /* >> + * The combination of pipeline + sensor delays means that >> + * exposure changes can take up to 3 frames to get applied, >> + * Run stats once every 4 frames to ensure any previous >> + * exposure changes have been applied. >> + */ >> + static constexpr uint32_t kStatPerNumFrames = 4; >> + >> int setupStandardBayerOrder(BayerFormat::Order order); >> /* Bayer 8 bpp unpacked */ >> void statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats); >> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp >> index 300822f9d..1a9dc1841 100644 >> --- a/src/libcamera/software_isp/debayer_egl.cpp >> +++ b/src/libcamera/software_isp/debayer_egl.cpp >> @@ -630,21 +630,17 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >> metadata.planes()[0].bytesused = output->planes()[0].length; >> /* Calculate stats for the whole frame */ >> - if (frame % SwStatsCpu::kStatPerNumFrames) { >> - stats_->finishFrame(frame, 0); >> - } else { >> - if (!inMapped) { >> - /* >> - * The buffer was directly imported into EGL and thus >> - * not mapped for texture upload. Do it now for the >> - * CPU-based stats calculation. >> - */ >> - assert(!inDmaSyncer); >> - inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read); >> - inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read); >> - } >> - stats_->processFrame(frame, 0, inMapped.value()); >> + if (stats_->updating(frame) && !inMapped) { >> + /* >> + * The buffer was directly imported into EGL and thus >> + * not mapped for texture upload. Do it now for the >> + * CPU-based stats calculation. >> + */ >> + assert(!inDmaSyncer); >> + inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read); >> + inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read); >> } >> + stats_->processFrame(frame, 0, inMapped.value()); > > This unfortunately doesn't work: in the "!stats_->updating(frame) && !inMapped" > case - e.g. on the second frame if dmabuf import was successful - inMapped is > not set and we crash. > > That's why I went with the somewhat ugly previous solution. Oops, I'll fix it. BTW, in case you'd be keen on running it, this commit is what I use for now to get some primitive stats and autofocus actually doing anything: https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commit/a1b3135c4175bfb9e823166467418e24014c7fc8 > >> inDmaSyncer.reset(); >> egl_.syncOutput(); >> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp >> index 7fb77ce7d..362fb6a86 100644 >> --- a/src/libcamera/software_isp/swstats_cpu.cpp >> +++ b/src/libcamera/software_isp/swstats_cpu.cpp >> @@ -383,7 +383,7 @@ void SwStatsCpu::statsGBRG12PLine0(const uint8_t *src[], SwIspStats &stats) >> */ >> void SwStatsCpu::startFrame(uint32_t frame) >> { >> - if (frame % kStatPerNumFrames) >> + if (!updating(frame)) >> return; >> if (window_.width == 0) >> @@ -404,7 +404,7 @@ void SwStatsCpu::startFrame(uint32_t frame) >> */ >> void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) >> { >> - bool valid = frame % kStatPerNumFrames == 0; >> + bool valid = updating(frame); >> if (valid) { >> sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); >> @@ -604,7 +604,7 @@ void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in) >> */ >> void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input) >> { >> - if (frame % kStatPerNumFrames) { >> + if (!updating(frame)) { >> finishFrame(frame, bufferId); >> return; >> } >> @@ -616,4 +616,14 @@ void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuff >> bench_.finishFrame(); >> } >> +/** >> + * \brief Determine whether stats update is required for the given frame >> + * \param[in] frame The frame number >> + * \return Whether the stats are updated for the given frame >> + */ >> +bool SwStatsCpu::updating(uint32_t frame) >> +{ >> + return frame % kStatPerNumFrames == 0; >> +} >> + >> } /* namespace libcamera */
Hi, On 21.10.26 15:56, Milan Zamazal wrote: > Hi Robert, > > thank you for review. > > Robert Mader <robert.mader@collabora.com> writes: > >> Hi, >> >> On 21.10.26 09:59, Milan Zamazal wrote: >>> Currently, the decision whether to compute stats for a given frame is >>> implemented in SwStatsCpu in several places and duplicated in >>> debayer_egl.cpp. Let's decide it solely in SwStatsCpu. This will also >>> help if a more flexible or complex update frequency is introduced, >>> e.g. to support stats for autofocus. >>> >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> .../internal/software_isp/swstats_cpu.h | 17 ++++++------- >>> src/libcamera/software_isp/debayer_egl.cpp | 24 ++++++++----------- >>> src/libcamera/software_isp/swstats_cpu.cpp | 16 ++++++++++--- >>> 3 files changed, 32 insertions(+), 25 deletions(-) >>> >>> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h >>> index 551870921..cd5303a51 100644 >>> --- a/include/libcamera/internal/software_isp/swstats_cpu.h >>> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h >>> @@ -38,14 +38,6 @@ public: >>> SwStatsCpu(const CameraManager &cm); >>> ~SwStatsCpu() = default; >>> - /* >>> - * The combination of pipeline + sensor delays means that >>> - * exposure changes can take up to 3 frames to get applied, >>> - * Run stats once every 4 frames to ensure any previous >>> - * exposure changes have been applied. >>> - */ >>> - static constexpr uint32_t kStatPerNumFrames = 4; >>> - >>> bool isValid() const { return sharedStats_.fd().isValid(); } >>> const SharedFD &getStatsFD() { return sharedStats_.fd(); } >>> @@ -54,6 +46,7 @@ public: >>> int configure(const StreamConfiguration &inputCfg, unsigned int >>> statsBufferCount = 1); >>> void setWindow(const Rectangle &window); >>> + bool updating(uint32_t frame); >>> void startFrame(uint32_t frame); >>> void finishFrame(uint32_t frame, uint32_t bufferId); >>> void processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input); >>> @@ -88,6 +81,14 @@ private: >>> using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[], SwIspStats &stats); >>> using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in); >>> + /* >>> + * The combination of pipeline + sensor delays means that >>> + * exposure changes can take up to 3 frames to get applied, >>> + * Run stats once every 4 frames to ensure any previous >>> + * exposure changes have been applied. >>> + */ >>> + static constexpr uint32_t kStatPerNumFrames = 4; >>> + >>> int setupStandardBayerOrder(BayerFormat::Order order); >>> /* Bayer 8 bpp unpacked */ >>> void statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats); >>> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp >>> index 300822f9d..1a9dc1841 100644 >>> --- a/src/libcamera/software_isp/debayer_egl.cpp >>> +++ b/src/libcamera/software_isp/debayer_egl.cpp >>> @@ -630,21 +630,17 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >>> metadata.planes()[0].bytesused = output->planes()[0].length; >>> /* Calculate stats for the whole frame */ >>> - if (frame % SwStatsCpu::kStatPerNumFrames) { >>> - stats_->finishFrame(frame, 0); >>> - } else { >>> - if (!inMapped) { >>> - /* >>> - * The buffer was directly imported into EGL and thus >>> - * not mapped for texture upload. Do it now for the >>> - * CPU-based stats calculation. >>> - */ >>> - assert(!inDmaSyncer); >>> - inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read); >>> - inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read); >>> - } >>> - stats_->processFrame(frame, 0, inMapped.value()); >>> + if (stats_->updating(frame) && !inMapped) { >>> + /* >>> + * The buffer was directly imported into EGL and thus >>> + * not mapped for texture upload. Do it now for the >>> + * CPU-based stats calculation. >>> + */ >>> + assert(!inDmaSyncer); >>> + inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read); >>> + inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read); >>> } >>> + stats_->processFrame(frame, 0, inMapped.value()); >> This unfortunately doesn't work: in the "!stats_->updating(frame) && !inMapped" >> case - e.g. on the second frame if dmabuf import was successful - inMapped is >> not set and we crash. >> >> That's why I went with the somewhat ugly previous solution. > Oops, I'll fix it. > > BTW, in case you'd be keen on running it, this commit is what I use for > now to get some primitive stats and autofocus actually doing anything: > https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commit/a1b3135c4175bfb9e823166467418e24014c7fc8 Nice, thanks! Just gave this a quick go on some phones: FairPhone 5, OnePlus 6, Pixel 3a and the Librem 5 (with Mesa 26.3, bringing GLES 3.0 support and thus making GTK apps like Snapshot usable again) - all on Nura (formerly postmarketOS). The AF works on each of them, meaning all have at least one rear camera with actuator properly wired up - and can thus be used for further testing. And while there's still a lot to be optimized, the current code already makes the devices much more useful - e.g. for stuff like QR-code scanning. Let's try to get it in for 0.9! :) > >>> inDmaSyncer.reset(); >>> egl_.syncOutput(); >>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp >>> index 7fb77ce7d..362fb6a86 100644 >>> --- a/src/libcamera/software_isp/swstats_cpu.cpp >>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp >>> @@ -383,7 +383,7 @@ void SwStatsCpu::statsGBRG12PLine0(const uint8_t *src[], SwIspStats &stats) >>> */ >>> void SwStatsCpu::startFrame(uint32_t frame) >>> { >>> - if (frame % kStatPerNumFrames) >>> + if (!updating(frame)) >>> return; >>> if (window_.width == 0) >>> @@ -404,7 +404,7 @@ void SwStatsCpu::startFrame(uint32_t frame) >>> */ >>> void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) >>> { >>> - bool valid = frame % kStatPerNumFrames == 0; >>> + bool valid = updating(frame); >>> if (valid) { >>> sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); >>> @@ -604,7 +604,7 @@ void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in) >>> */ >>> void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input) >>> { >>> - if (frame % kStatPerNumFrames) { >>> + if (!updating(frame)) { >>> finishFrame(frame, bufferId); >>> return; >>> } >>> @@ -616,4 +616,14 @@ void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuff >>> bench_.finishFrame(); >>> } >>> +/** >>> + * \brief Determine whether stats update is required for the given frame >>> + * \param[in] frame The frame number >>> + * \return Whether the stats are updated for the given frame >>> + */ >>> +bool SwStatsCpu::updating(uint32_t frame) >>> +{ >>> + return frame % kStatPerNumFrames == 0; >>> +} >>> + >>> } /* namespace libcamera */
Robert Mader <robert.mader@collabora.com> writes: > Hi, > > On 21.10.26 15:56, Milan Zamazal wrote: >> Hi Robert, >> >> thank you for review. >> >> Robert Mader <robert.mader@collabora.com> writes: >> >>> Hi, >>> >>> On 21.10.26 09:59, Milan Zamazal wrote: >>>> Currently, the decision whether to compute stats for a given frame is >>>> implemented in SwStatsCpu in several places and duplicated in >>>> debayer_egl.cpp. Let's decide it solely in SwStatsCpu. This will also >>>> help if a more flexible or complex update frequency is introduced, >>>> e.g. to support stats for autofocus. >>>> >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> .../internal/software_isp/swstats_cpu.h | 17 ++++++------- >>>> src/libcamera/software_isp/debayer_egl.cpp | 24 ++++++++----------- >>>> src/libcamera/software_isp/swstats_cpu.cpp | 16 ++++++++++--- >>>> 3 files changed, 32 insertions(+), 25 deletions(-) >>>> >>>> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h >>>> index 551870921..cd5303a51 100644 >>>> --- a/include/libcamera/internal/software_isp/swstats_cpu.h >>>> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h >>>> @@ -38,14 +38,6 @@ public: >>>> SwStatsCpu(const CameraManager &cm); >>>> ~SwStatsCpu() = default; >>>> - /* >>>> - * The combination of pipeline + sensor delays means that >>>> - * exposure changes can take up to 3 frames to get applied, >>>> - * Run stats once every 4 frames to ensure any previous >>>> - * exposure changes have been applied. >>>> - */ >>>> - static constexpr uint32_t kStatPerNumFrames = 4; >>>> - >>>> bool isValid() const { return sharedStats_.fd().isValid(); } >>>> const SharedFD &getStatsFD() { return sharedStats_.fd(); } >>>> @@ -54,6 +46,7 @@ public: >>>> int configure(const StreamConfiguration &inputCfg, unsigned int >>>> statsBufferCount = 1); >>>> void setWindow(const Rectangle &window); >>>> + bool updating(uint32_t frame); >>>> void startFrame(uint32_t frame); >>>> void finishFrame(uint32_t frame, uint32_t bufferId); >>>> void processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input); >>>> @@ -88,6 +81,14 @@ private: >>>> using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[], SwIspStats &stats); >>>> using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in); >>>> + /* >>>> + * The combination of pipeline + sensor delays means that >>>> + * exposure changes can take up to 3 frames to get applied, >>>> + * Run stats once every 4 frames to ensure any previous >>>> + * exposure changes have been applied. >>>> + */ >>>> + static constexpr uint32_t kStatPerNumFrames = 4; >>>> + >>>> int setupStandardBayerOrder(BayerFormat::Order order); >>>> /* Bayer 8 bpp unpacked */ >>>> void statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats); >>>> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp >>>> index 300822f9d..1a9dc1841 100644 >>>> --- a/src/libcamera/software_isp/debayer_egl.cpp >>>> +++ b/src/libcamera/software_isp/debayer_egl.cpp >>>> @@ -630,21 +630,17 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >>>> metadata.planes()[0].bytesused = output->planes()[0].length; >>>> /* Calculate stats for the whole frame */ >>>> - if (frame % SwStatsCpu::kStatPerNumFrames) { >>>> - stats_->finishFrame(frame, 0); >>>> - } else { >>>> - if (!inMapped) { >>>> - /* >>>> - * The buffer was directly imported into EGL and thus >>>> - * not mapped for texture upload. Do it now for the >>>> - * CPU-based stats calculation. >>>> - */ >>>> - assert(!inDmaSyncer); >>>> - inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read); >>>> - inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read); >>>> - } >>>> - stats_->processFrame(frame, 0, inMapped.value()); >>>> + if (stats_->updating(frame) && !inMapped) { >>>> + /* >>>> + * The buffer was directly imported into EGL and thus >>>> + * not mapped for texture upload. Do it now for the >>>> + * CPU-based stats calculation. >>>> + */ >>>> + assert(!inDmaSyncer); >>>> + inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read); >>>> + inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read); >>>> } >>>> + stats_->processFrame(frame, 0, inMapped.value()); >>> This unfortunately doesn't work: in the "!stats_->updating(frame) && !inMapped" >>> case - e.g. on the second frame if dmabuf import was successful - inMapped is >>> not set and we crash. >>> >>> That's why I went with the somewhat ugly previous solution. >> Oops, I'll fix it. >> >> BTW, in case you'd be keen on running it, this commit is what I use for >> now to get some primitive stats and autofocus actually doing anything: >> https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commit/a1b3135c4175bfb9e823166467418e24014c7fc8 > > Nice, thanks! Just gave this a quick go on some phones: FairPhone 5, OnePlus 6, > Pixel 3a and the Librem 5 (with Mesa 26.3, bringing GLES 3.0 support and thus > making GTK apps like Snapshot usable again) - all on Nura (formerly > postmarketOS). > > The AF works on each of them, meaning all have at least one rear camera with > actuator properly wired up - and can thus be used for further testing. And while > there's still a lot to be optimized, the current code already makes the devices > much more useful - e.g. for stuff like QR-code scanning. Nice to hear and better than what I'd expect. Thanks for testing! > Let's try to get it in for 0.9! :) Yes, let's try. > >> >>>> inDmaSyncer.reset(); >>>> egl_.syncOutput(); >>>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp >>>> index 7fb77ce7d..362fb6a86 100644 >>>> --- a/src/libcamera/software_isp/swstats_cpu.cpp >>>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp >>>> @@ -383,7 +383,7 @@ void SwStatsCpu::statsGBRG12PLine0(const uint8_t *src[], SwIspStats &stats) >>>> */ >>>> void SwStatsCpu::startFrame(uint32_t frame) >>>> { >>>> - if (frame % kStatPerNumFrames) >>>> + if (!updating(frame)) >>>> return; >>>> if (window_.width == 0) >>>> @@ -404,7 +404,7 @@ void SwStatsCpu::startFrame(uint32_t frame) >>>> */ >>>> void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) >>>> { >>>> - bool valid = frame % kStatPerNumFrames == 0; >>>> + bool valid = updating(frame); >>>> if (valid) { >>>> sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); >>>> @@ -604,7 +604,7 @@ void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in) >>>> */ >>>> void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input) >>>> { >>>> - if (frame % kStatPerNumFrames) { >>>> + if (!updating(frame)) { >>>> finishFrame(frame, bufferId); >>>> return; >>>> } >>>> @@ -616,4 +616,14 @@ void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuff >>>> bench_.finishFrame(); >>>> } >>>> +/** >>>> + * \brief Determine whether stats update is required for the given frame >>>> + * \param[in] frame The frame number >>>> + * \return Whether the stats are updated for the given frame >>>> + */ >>>> +bool SwStatsCpu::updating(uint32_t frame) >>>> +{ >>>> + return frame % kStatPerNumFrames == 0; >>>> +} >>>> + >>>> } /* namespace libcamera */
diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h index 551870921..cd5303a51 100644 --- a/include/libcamera/internal/software_isp/swstats_cpu.h +++ b/include/libcamera/internal/software_isp/swstats_cpu.h @@ -38,14 +38,6 @@ public: SwStatsCpu(const CameraManager &cm); ~SwStatsCpu() = default; - /* - * The combination of pipeline + sensor delays means that - * exposure changes can take up to 3 frames to get applied, - * Run stats once every 4 frames to ensure any previous - * exposure changes have been applied. - */ - static constexpr uint32_t kStatPerNumFrames = 4; - bool isValid() const { return sharedStats_.fd().isValid(); } const SharedFD &getStatsFD() { return sharedStats_.fd(); } @@ -54,6 +46,7 @@ public: int configure(const StreamConfiguration &inputCfg, unsigned int statsBufferCount = 1); void setWindow(const Rectangle &window); + bool updating(uint32_t frame); void startFrame(uint32_t frame); void finishFrame(uint32_t frame, uint32_t bufferId); void processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input); @@ -88,6 +81,14 @@ private: using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[], SwIspStats &stats); using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in); + /* + * The combination of pipeline + sensor delays means that + * exposure changes can take up to 3 frames to get applied, + * Run stats once every 4 frames to ensure any previous + * exposure changes have been applied. + */ + static constexpr uint32_t kStatPerNumFrames = 4; + int setupStandardBayerOrder(BayerFormat::Order order); /* Bayer 8 bpp unpacked */ void statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats); diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp index 300822f9d..1a9dc1841 100644 --- a/src/libcamera/software_isp/debayer_egl.cpp +++ b/src/libcamera/software_isp/debayer_egl.cpp @@ -630,21 +630,17 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output metadata.planes()[0].bytesused = output->planes()[0].length; /* Calculate stats for the whole frame */ - if (frame % SwStatsCpu::kStatPerNumFrames) { - stats_->finishFrame(frame, 0); - } else { - if (!inMapped) { - /* - * The buffer was directly imported into EGL and thus - * not mapped for texture upload. Do it now for the - * CPU-based stats calculation. - */ - assert(!inDmaSyncer); - inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read); - inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read); - } - stats_->processFrame(frame, 0, inMapped.value()); + if (stats_->updating(frame) && !inMapped) { + /* + * The buffer was directly imported into EGL and thus + * not mapped for texture upload. Do it now for the + * CPU-based stats calculation. + */ + assert(!inDmaSyncer); + inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read); + inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read); } + stats_->processFrame(frame, 0, inMapped.value()); inDmaSyncer.reset(); egl_.syncOutput(); diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp index 7fb77ce7d..362fb6a86 100644 --- a/src/libcamera/software_isp/swstats_cpu.cpp +++ b/src/libcamera/software_isp/swstats_cpu.cpp @@ -383,7 +383,7 @@ void SwStatsCpu::statsGBRG12PLine0(const uint8_t *src[], SwIspStats &stats) */ void SwStatsCpu::startFrame(uint32_t frame) { - if (frame % kStatPerNumFrames) + if (!updating(frame)) return; if (window_.width == 0) @@ -404,7 +404,7 @@ void SwStatsCpu::startFrame(uint32_t frame) */ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) { - bool valid = frame % kStatPerNumFrames == 0; + bool valid = updating(frame); if (valid) { sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); @@ -604,7 +604,7 @@ void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in) */ void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input) { - if (frame % kStatPerNumFrames) { + if (!updating(frame)) { finishFrame(frame, bufferId); return; } @@ -616,4 +616,14 @@ void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuff bench_.finishFrame(); } +/** + * \brief Determine whether stats update is required for the given frame + * \param[in] frame The frame number + * \return Whether the stats are updated for the given frame + */ +bool SwStatsCpu::updating(uint32_t frame) +{ + return frame % kStatPerNumFrames == 0; +} + } /* namespace libcamera */
Currently, the decision whether to compute stats for a given frame is implemented in SwStatsCpu in several places and duplicated in debayer_egl.cpp. Let's decide it solely in SwStatsCpu. This will also help if a more flexible or complex update frequency is introduced, e.g. to support stats for autofocus. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- .../internal/software_isp/swstats_cpu.h | 17 ++++++------- src/libcamera/software_isp/debayer_egl.cpp | 24 ++++++++----------- src/libcamera/software_isp/swstats_cpu.cpp | 16 ++++++++++--- 3 files changed, 32 insertions(+), 25 deletions(-)