[libcamera-devel] [PATCH v4 0/5] Raspberry Pi: Generalise statistics
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Feb 10 00:03:03 CET 2023
Quoting Laurent Pinchart (2023-02-09 19:34:49)
> On Thu, Feb 09, 2023 at 03:07:54PM +0000, Naushir Patuck via libcamera-devel wrote:
> > On Thu, 9 Feb 2023 at 14:20, Kieran Bingham wrote:
> > > Quoting Naushir Patuck (2023-02-09 11:47:23)
> > > > On Thu, 9 Feb 2023 at 11:38, Kieran Bingham via libcamera-devel wrote:
> > > > > Quoting Kieran Bingham (2023-02-09 10:37:22)
> > > > > > On 09/02/2023 08:31, Naushir Patuck via libcamera-devel wrote:
> > > > > > > Hi all,
> > > > > > >
> > > > > > > If there are no other review comments or feedback, I think this is
> > > > > > > ready to be merged.
> > > > > >
> > > > > > I'll apply this now.
> > > > > >
> > > > > > I wonder if it's worth using/trying pull requests when you have code
> > > > > > that is explicitly ready for merge from Raspberry Pi.
> > > > > >
> > > > > > - https://git-scm.com/docs/git-request-pull
> > > >
> > > > Interesting, I've never tried this before. Will investigate exactly
> > > > what it does.
> > >
> > > My integration scripts all work from a branch on a repository, so this
> > > would help me quite a bit as I could just directly pull the code you
> > > want to be merged.
> > >
> > > Speaking of which. This series is merged.
> >
> > Thanks Kieran!
> >
> > On the subject of git request-pull, what kind of process were you
> > thinking of? Do you think something like:
> >
> > - Post patches for review to the ML as normal.
> > - Once patches are reviewed and ready for submission, I create a
> > branch on github.
> > - Run git request-pull and copy/paste the output in an email to you?
>
> That e-mail should go to the list. Kieran can tell you if he prefers
> being CC'ed or if he will handle the pull request directly from the
> list.
Yes, To the list and cc me is fine. I'll get it from the list, but
having me as a To:/Cc: highlights it in my inbox too.
The main benefit is it removes the ambiguity of 'is this ready to merge
or not yet'. It's purely 'here is my branch, please merge it now'.
(which will then kick off my testing at least)
> > - You have scripts that parse the output and do the right thing(TM).
> >
> > might be useful?
>
> I don't know if Kieran has such scripts, but the rest of the procedure
> which details your tasks seems perfect to me. I'm sure we'll grow
> scripts if we don't have them yet :-)
At the moment I run scripts in the form:
./integration-tests.sh <branch> <repo>
So as long as I can extract the branch and repo - then yes this becomes
easy to integrate.
And it saves the hard part of having to manually apply patches, which
may be partially updated (as in this series) and ensures that what gets
applied, is exactly as you wish.
The only caveat is that if it's not based on master, it may need a
rebase. But otherwise - it would simplify things.
--
Kieran
>
> > > > > It failed compilation matrix I'm afraid:
> > > >
> > > > Argh. My clang-9 did not seem to complan.
> > > > Looks to be a trivial fix (remove std::move). I'll make the change
> > > > and re-submit.
> > > >
> > > > > Configuration: clang-11:clang++-11 (/home/kbingham/iob/libcamera/ci/integrator/logs/build-matrix-clang-11.log)
> > > > >
> > > > > ...
> > > > > [51/120] Compiling C++ object src/ipa/raspberrypi/ipa_rpi.so.p/raspberrypi.cpp.o
> > > > > FAILED: src/ipa/raspberrypi/ipa_rpi.so.p/raspberrypi.cpp.o
> > > > > clang++-11 -Isrc/ipa/raspberrypi/ipa_rpi.so.p -Isrc/ipa/raspberrypi -I../../../src/libcamera/src/ipa/raspberrypi -Iinclude -I../../../src/libcamera/include -Isrc/ipa -I../../../src/libcamera/src/ipa -I../../../src/libcamera/src/ipa/raspberrypi/controller -Iinclude/libcamera/ipa -Iinclude/libcamera -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -O0 -g -Wextra-semi -Wthread-safety -Wshadow -include /home/kbingham/iob/libcamera/ci/integrator/builds/build-matrix/clang-11/config.h -Wno-c99-designator -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/raspberrypi/ipa_rpi.so.p/raspberrypi.cpp.o -MF src/ipa/raspberrypi/ipa_rpi.so.p/raspberrypi.cpp.o.d -o src/ipa/raspberrypi/ipa_rpi.so.p/raspberrypi.cpp.o -c ../../../src/libcamera/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > ../../../src/libcamera/src/ipa/raspberrypi/raspberrypi.cpp:1378:22: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
> > > > > statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS));
> > > > > ^
> > > > > ../../../src/libcamera/src/ipa/raspberrypi/raspberrypi.cpp:1378:22: note: remove std::move call here
> > > > > statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS));
> > > > > ^~~~~~~~~~ ~
> > > > > 1 error generated.
> > > > > [52/120] Compiling C++ object src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_dpc.cpp.o
> > > > > [53/120] Compiling C++ object src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx477.cpp.o
> > > > > [54/120] Compiling C++ object src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_noise.cpp.o
> > > > > [55/120] Compiling C++ object src/android/libcamera-hal.so.p/camera_capabilities.cpp.o
> > > > > [56/120] Compiling C++ object src/ipa/raspberrypi/ipa_rpi.so.p/controller_pwl.cpp.o
> > > > > [57/120] Compiling C++ object src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx519.cpp.o
> > > > > [58/120] Compiling C++ object src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_sdn.cpp.o
> > > > > [59/120] Compiling C++ object src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o
> > > > > FAILED: src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o
> > > > > clang++-11 -Isrc/ipa/raspberrypi/ipa_rpi.so.p -Isrc/ipa/raspberrypi -I../../../src/libcamera/src/ipa/raspberrypi -Iinclude -I../../../src/libcamera/include -Isrc/ipa -I../../../src/libcamera/src/ipa -I../../../src/libcamera/src/ipa/raspberrypi/controller -Iinclude/libcamera/ipa -Iinclude/libcamera -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -O0 -g -Wextra-semi -Wthread-safety -Wshadow -include /home/kbingham/iob/libcamera/ci/integrator/builds/build-matrix/clang-11/config.h -Wno-c99-designator -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o -MF src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o.d -o src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o -c ../../../src/libcamera/src/ipa/raspberrypi/cam_helper_imx708.cpp
> > > > > ../../../src/libcamera/src/ipa/raspberrypi/cam_helper_imx708.cpp:315:18: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
> > > > > aeHistLinear_ = std::move(Histogram(hist, 128));
> > > > > ^
> > > > > ../../../src/libcamera/src/ipa/raspberrypi/cam_helper_imx708.cpp:315:18: note: remove std::move call here
> > > > > aeHistLinear_ = std::move(Histogram(hist, 128));
> > > > > ^~~~~~~~~~ ~
> > > > > 1 error generated.
> > > > > ...
> > > > >
> > > > > > > On Fri, 3 Feb 2023 at 09:17, Naushir Patuck <naush at raspberrypi.com> wrote:
> > > > > > >>
> > > > > > >> Hi,
> > > > > > >>
> > > > > > >> Version 5 rebases the changes on-top of master - particularly updating the
> > > > > > >> breakages in cam_helper_imx708.cpp in patch 4/5.
> > > > > > >>
> > > > > > >> Regards,
> > > > > > >> Naush
> > > > > > >>
> > > > > > >> Naushir Patuck (5):
> > > > > > >> ipa: raspberrypi: Generalise statistics
> > > > > > >> ipa: raspberrypi: histogram: Add a constructor for an empty histogram
> > > > > > >> ipa: raspberrypi: awb: Delay release of the statistics buffer
> > > > > > >> ipa: raspberrypi: Use the generic statistics structure in the
> > > > > > >> algorithms
> > > > > > >> ipa: raspberrypi: Normalise region sums to 16-bits
> > > > > > >>
> > > > > > >> src/ipa/raspberrypi/cam_helper_imx708.cpp | 26 ++--
> > > > > > >> src/ipa/raspberrypi/controller/controller.h | 4 +-
> > > > > > >> src/ipa/raspberrypi/controller/histogram.h | 5 +
> > > > > > >> src/ipa/raspberrypi/controller/region_stats.h | 123 ++++++++++++++++++
> > > > > > >> src/ipa/raspberrypi/controller/rpi/af.cpp | 10 +-
> > > > > > >> src/ipa/raspberrypi/controller/rpi/af.h | 8 +-
> > > > > > >> src/ipa/raspberrypi/controller/rpi/agc.cpp | 32 ++---
> > > > > > >> src/ipa/raspberrypi/controller/rpi/agc.h | 2 +-
> > > > > > >> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 32 ++---
> > > > > > >> src/ipa/raspberrypi/controller/rpi/alsc.h | 3 +-
> > > > > > >> src/ipa/raspberrypi/controller/rpi/awb.cpp | 30 ++---
> > > > > > >> src/ipa/raspberrypi/controller/rpi/awb.h | 1 +
> > > > > > >> .../raspberrypi/controller/rpi/contrast.cpp | 8 +-
> > > > > > >> src/ipa/raspberrypi/controller/rpi/focus.cpp | 7 +-
> > > > > > >> src/ipa/raspberrypi/controller/rpi/lux.cpp | 14 +-
> > > > > > >> src/ipa/raspberrypi/raspberrypi.cpp | 50 ++++++-
> > > > > > >> src/ipa/raspberrypi/statistics.h | 78 +++++++++++
> > > > > > >> 17 files changed, 338 insertions(+), 95 deletions(-)
> > > > > > >> create mode 100644 src/ipa/raspberrypi/controller/region_stats.h
> > > > > > >> create mode 100644 src/ipa/raspberrypi/statistics.h
> > > > > > >>
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list