[libcamera-devel] [PATCH v3 3/4] libcamera: ipa: raspberrypi: ALSC: Resample luminance table
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 5 01:47:14 CEST 2020
Hi Naush,
Thanks for the reviews. Do you plan to review 4/4 too ?
On Tue, Aug 04, 2020 at 09:49:17AM +0100, Naushir Patuck wrote:
> Hi David,
>
> Thank you for the patch, LGTM.
>
> On Sat, 1 Aug 2020 at 09:02, 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_).
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Reviewed-by: Naushir Patuck <naush at raspberrypi.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 12359dc..4df9934 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()
> > +{
> > + if (async_started_) {
> > + async_started_ = false;
> > + std::unique_lock<std::mutex> lock(mutex_);
> > + sync_signal_.wait(lock, [&] {
> > + return async_finished_;
> > + });
> > + 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_started_ = true;
> > {
> > std::lock_guard<std::mutex> lock(mutex_);
> > @@ -672,9 +685,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;
> > @@ -697,7 +710,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