[libcamera-devel] [PATCH 2/3] libcamera: ipa: raspberrypi: ALSC: Resample luminance table
David Plowman
david.plowman at raspberrypi.com
Fri Jul 31 14:27:42 CEST 2020
Hi again
Thanks for that. Let me circulate a second version of these patches,
and we can also wait for Naush to review next week.
David
On Fri, 31 Jul 2020 at 13:01, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> On Fri, Jul 31, 2020 at 09:03:35AM +0100, David Plowman wrote:
> > On Thu, 30 Jul 2020 at 23:57, Laurent Pinchart wrote:
> > > On Thu, Jul 30, 2020 at 12:11:33PM +0100, David Plowman wrote:
> > > > This fixes a bug where the luminance correction table was not being
> > > > resampled according to the camera mode, in the same way as the colour
> > > > tables. This could be noticeable if any camera modes crop
> > > > aggressively.
> > > >
> > > > This resampling can be done "up front" in the SwitchMode, as we have
> > > > only a single fixed luminance table. In order to protect the
> > > > recalculation of the table from the asynchronous thread (which reads
> > > > it) I've elected to wait for that thread to go idle (though I doubt it
> > > > would have mattered much). As a by-product of stopping the thread, it
> > > > no longer needs its own copy of the camera mode (async_camera_mode_).
> > >
> > > This looks good to me. There are however a couple of potential issues
> > > I've noticed when reading the ALSC code and that you may want to check:
> > >
> > > - In Alsc::restartAsync(), async_start_ and async_started_ are set
> > > without holding the mutex_. They could thus potentially be reordered,
> > > possibly causing a race condition.
> >
> > async_started_ is used only by the main thread, so all we have to
> > worry about is async_start_.
>
> Oops, indeed, my bad.
>
> > Presumably this is safe assuming updating bools is atomic. But I have
> > some recollection that nothing is guaranteed atomic if you don't mark
> > it explicitly, so it probably wants wrapping in std::atomic, is that
> > right?
>
> If it's used in a single thread there's no issue, atomicity isn't
> required.
>
> > Same comments probably true about async_abort_. If that's atomic (and
> > marked explicitly), no need for the lock in the destructor.
>
> async_abort_ is used in the ALSC thread, so for that we need a lock.
> It's not just about atomicity, it's also about ensuring that memory
> accesses are not reordered. If you write
>
> async_abort_ = true;
> async_signal_.notify_one();
>
> without taking a lock, there is no guarantee that the async_abort_ write
> will be visible to the reader before it gets woken up by
> async_signal_.notify_one().
>
> The documentation of std::condition_variable::notify_one() states
>
> "The effects of notify_one()/notify_all() and each of the three atomic
> parts of wait()/wait_for()/wait_until() (unlock+wait, wakeup, and lock)
> take place in a single total order that can be viewed as modification
> order of an atomic variable: the order is specific to this individual
> condition_variable. This makes it impossible for notify_one() to, for
> example, be delayed and unblock a thread that started waiting just after
> the call to notify_one() was made."
>
> This doesn't offer any memory order guarantee (as in
> https://en.cppreference.com/w/cpp/atomic/memory_order) for the
> modification of the shared variable.
>
> The std::condition_variable documentation state
>
> "Even if the shared variable is atomic, it must be modified under the
> mutex in order to correctly publish the modification to the waiting
> thread."
>
> The lock is thus needed, but the notify_one() call can be moved after
> unlocking the mutex.
>
> > > - In Alsc::asyncFunc(), sync_signal_.notify_one() is called with the
> > > mutex_ held. The function will cause the thread waiting on the
> > > sync_signal_ condition variable to wake up, to then go back to sleep
> > > immediatelly when it tries to lock the mutex_. The notify_one() call
> > > should be moved after the locked scope.
> >
> > Here too, presumably the lock is in fact redundant if we believe the
> > update to async_finished_ is atomic (though again, the correct thing
> > would be to wrap it in std::atomic)
>
> See above :-) Memory ordering is a tricky topic.
>
> > > - sync_signal_ is actually never used :-) Or rather was never used, as
> > > this patch now waits on it in Alsc::waitForAysncThread(). The
> > > sync_signal_ in the AWB algorithm is however completely unused.
> >
> > Yes indeed. All this code was ported from somewhere where you could
> > force these algorithms to run synchronously (i.e. wait for the
> > calculations to finish and use them immediately), so these other
> > signals were indeed being used. I removed that for the libcamera/VC4
> > platform and never did anything about this signal. Though I'm quite
> > happy to have left it in now! I'm not sure about the AWB one, I can't
> > think of any case where waiting for that thread to stop would be
> > useful like it is here, so maybe that will be one to remove.
> >
> > Thanks for all the comments!
> >
> > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > >
> > > For this patch,
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >
> > > > ---
> > > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 39 ++++++++++++++-------
> > > > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 3 +-
> > > > 2 files changed, 28 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> > > > index 76e2f04..8f7720b 100644
> > > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> > > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> > > > @@ -173,19 +173,34 @@ void Alsc::Initialise()
> > > > lambda_r_[i] = lambda_b_[i] = 1.0;
> > > > }
> > > >
> > > > +void Alsc::waitForAysncThread()
> > > > +{
> > > > + std::unique_lock<std::mutex> lock(mutex_);
> > > > + if (async_started_) {
> > > > + sync_signal_.wait(lock, [&] {
> > > > + return async_finished_;
> > > > + });
> > > > + async_started_ = false;
> > > > + async_finished_ = false;
> > > > + }
> > > > +}
> > > > +
> > > > void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> > > > {
> > > > (void)metadata;
> > > >
> > > > + // Ensure the other thread isn't running while we do this.
> > > > + waitForAysncThread();
> > > > +
> > > > // There's a bit of a question what we should do if the "crop" of the
> > > > - // camera mode has changed. Any calculation currently in flight would
> > > > - // not be useful to the new mode, so arguably we should abort it, and
> > > > - // generate a new table (like the "first_time" code already here). When
> > > > - // the crop doesn't change, we can presumably just leave things
> > > > - // alone. For now, I think we'll just wait and see. When the crop does
> > > > - // change, any effects should be transient, and if they're not transient
> > > > - // enough, we'll revisit the question then.
> > > > + // camera mode has changed. Should we effectively reset the algorithm
> > > > + // and start over?
> > > > camera_mode_ = camera_mode;
> > > > +
> > > > + // We must resample the luminance table like we do the others, but it's
> > > > + // fixed so we can simply do it up front here.
> > > > + resample_cal_table(config_.luminance_lut, camera_mode_, luminance_table_);
> > > > +
> > > > if (first_time_) {
> > > > // On the first time, arrange for something sensible in the
> > > > // initial tables. Construct the tables for some default colour
> > > > @@ -201,7 +216,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> > > > compensate_lambdas_for_cal(cal_table_b, lambda_b_,
> > > > async_lambda_b_);
> > > > add_luminance_to_tables(sync_results_, async_lambda_r_, 1.0,
> > > > - async_lambda_b_, config_.luminance_lut,
> > > > + async_lambda_b_, luminance_table_,
> > > > config_.luminance_strength);
> > > > memcpy(prev_sync_results_, sync_results_,
> > > > sizeof(prev_sync_results_));
> > > > @@ -266,8 +281,6 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)
> > > > }
> > > > copy_stats(statistics_, stats, alsc_status);
> > > > frame_phase_ = 0;
> > > > - // copy the camera mode so it won't change during the calculations
> > > > - async_camera_mode_ = camera_mode_;
> > > > async_start_ = true;
> > > > async_started_ = true;
> > > > async_signal_.notify_one();
> > > > @@ -670,9 +683,9 @@ void Alsc::doAlsc()
> > > > // Fetch the new calibrations (if any) for this CT. Resample them in
> > > > // case the camera mode is not full-frame.
> > > > get_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);
> > > > - resample_cal_table(cal_table_tmp, async_camera_mode_, cal_table_r);
> > > > + resample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);
> > > > get_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);
> > > > - resample_cal_table(cal_table_tmp, async_camera_mode_, cal_table_b);
> > > > + resample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);
> > > > // You could print out the cal tables for this image here, if you're
> > > > // tuning the algorithm...
> > > > (void)print_cal_table;
> > > > @@ -695,7 +708,7 @@ void Alsc::doAlsc()
> > > > compensate_lambdas_for_cal(cal_table_b, lambda_b_, async_lambda_b_);
> > > > // Fold in the luminance table at the appropriate strength.
> > > > add_luminance_to_tables(async_results_, async_lambda_r_, 1.0,
> > > > - async_lambda_b_, config_.luminance_lut,
> > > > + async_lambda_b_, luminance_table_,
> > > > config_.luminance_strength);
> > > > }
> > > >
> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp
> > > > index e895913..95572af 100644
> > > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp
> > > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp
> > > > @@ -60,10 +60,10 @@ private:
> > > > AlscConfig config_;
> > > > bool first_time_;
> > > > CameraMode camera_mode_;
> > > > + double luminance_table_[ALSC_CELLS_X * ALSC_CELLS_Y];
> > > > std::thread async_thread_;
> > > > void asyncFunc(); // asynchronous thread function
> > > > std::mutex mutex_;
> > > > - CameraMode async_camera_mode_;
> > > > // condvar for async thread to wait on
> > > > std::condition_variable async_signal_;
> > > > // condvar for synchronous thread to wait on
> > > > @@ -86,6 +86,7 @@ private:
> > > > int frame_count2_;
> > > > double sync_results_[3][ALSC_CELLS_Y][ALSC_CELLS_X];
> > > > double prev_sync_results_[3][ALSC_CELLS_Y][ALSC_CELLS_X];
> > > > + void waitForAysncThread();
> > > > // The following are for the asynchronous thread to use, though the main
> > > > // thread can set/reset them if the async thread is known to be idle:
> > > > void restartAsync(StatisticsPtr &stats, Metadata *image_metadata);
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list