| Message ID | 20260120170701.400944-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, thank you for the patch. Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > When calling `Debayer::process()` from `SoftwareIsp::process()`, the > `DebayerParams` object is copied multiple time: > > (1) call of `BoundMethodMember<...>::activate()` > inside `Object::invokeMethod()` > (2) constructor of `BoundMethodArgs<...>` > inside `BoundMethodMember<...>::activate()` > (3) call of `BoundMethodMember<...>::invoke()` > inside `BoundMethodArgs::invokePack()` > (4) call of the actual pointer to member function > inside `BoundMethodMember::invoke()` > > Given that the type has a size of 5696 bytes at the moment on x86-64, > while the size is expected to shrink considerably and compilers might > avoid one or two of the above copies, this is still not ideal. By making > `Debayer::process()` take the parameter object by const lvalue reference, > only the copy in the `BoundMethodArgs` constructor remains. So do that. Why RFC? Looks like harmless cleanup. Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/libcamera/software_isp/debayer.cpp | 2 +- > src/libcamera/software_isp/debayer.h | 4 ++-- > src/libcamera/software_isp/debayer_cpu.cpp | 2 +- > src/libcamera/software_isp/debayer_cpu.h | 2 +- > src/libcamera/software_isp/debayer_egl.cpp | 6 +++--- > src/libcamera/software_isp/debayer_egl.h | 6 +++--- > 6 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp > index 65a1762dd..3e7702525 100644 > --- a/src/libcamera/software_isp/debayer.cpp > +++ b/src/libcamera/software_isp/debayer.cpp > @@ -401,7 +401,7 @@ Debayer::~Debayer() > * \brief Select the bayer params to use for the next frame debayer > * \param[in] params The parameters to be used in debayering > */ > -void Debayer::setParams(DebayerParams ¶ms) > +void Debayer::setParams(const DebayerParams ¶ms) > { > green_ = params.green; > greenCcm_ = params.greenCcm; > diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h > index cd2db9930..99e22ed85 100644 > --- a/src/libcamera/software_isp/debayer.h > +++ b/src/libcamera/software_isp/debayer.h > @@ -47,7 +47,7 @@ public: > virtual std::tuple<unsigned int, unsigned int> > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; > > - virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; > + virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) = 0; > virtual int start() { return 0; } > virtual void stop() {} > > @@ -92,7 +92,7 @@ private: > virtual Size patternSize(PixelFormat inputFormat) = 0; > > protected: > - void setParams(DebayerParams ¶ms); > + void setParams(const DebayerParams ¶ms); > void dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *input, FrameBuffer *output); > static bool isStandardBayerOrder(BayerFormat::Order order); > }; > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index 00738c56b..b52f4d5c7 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -740,7 +740,7 @@ void DebayerCpu::process4(uint32_t frame, const uint8_t *src, uint8_t *dst) > } > } > > -void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) > +void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) > { > bench_.startFrame(); > > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > index 67df2b93a..27eff021d 100644 > --- a/src/libcamera/software_isp/debayer_cpu.h > +++ b/src/libcamera/software_isp/debayer_cpu.h > @@ -37,7 +37,7 @@ public: > std::vector<PixelFormat> formats(PixelFormat input); > std::tuple<unsigned int, unsigned int> > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); > - void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); > + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms); > SizeRange sizes(PixelFormat inputFormat, const Size &inputSize); > const SharedFD &getStatsFD() { return stats_->getStatsFD(); } > > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > index 9693d7252..1ac162427 100644 > --- a/src/libcamera/software_isp/debayer_egl.cpp > +++ b/src/libcamera/software_isp/debayer_egl.cpp > @@ -386,7 +386,7 @@ DebayerEGL::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size > return std::make_tuple(stride, stride * size.height); > } > > -void DebayerEGL::setShaderVariableValues(DebayerParams ¶ms) > +void DebayerEGL::setShaderVariableValues(const DebayerParams ¶ms) > { > /* > * Raw Bayer 8-bit, and packed raw Bayer 10-bit/12-bit formats > @@ -509,7 +509,7 @@ void DebayerEGL::setShaderVariableValues(DebayerParams ¶ms) > return; > } > > -int DebayerEGL::debayerGPU(MappedFrameBuffer &in, int out_fd, DebayerParams ¶ms) > +int DebayerEGL::debayerGPU(MappedFrameBuffer &in, int out_fd, const DebayerParams ¶ms) > { > /* eGL context switch */ > egl_.makeCurrent(); > @@ -536,7 +536,7 @@ int DebayerEGL::debayerGPU(MappedFrameBuffer &in, int out_fd, DebayerParams &par > return 0; > } > > -void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) > +void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) > { > bench_.startFrame(); > > diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h > index a5033bc63..790348249 100644 > --- a/src/libcamera/software_isp/debayer_egl.h > +++ b/src/libcamera/software_isp/debayer_egl.h > @@ -50,7 +50,7 @@ public: > std::vector<PixelFormat> formats(PixelFormat input); > std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); > > - void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); > + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms); > int start(); > void stop(); > > @@ -72,9 +72,9 @@ private: > std::vector<std::string> shaderEnv); > int linkShaderProgram(void); > int getShaderVariableLocations(); > - void setShaderVariableValues(DebayerParams ¶ms); > + void setShaderVariableValues(const DebayerParams ¶ms); > void configureTexture(GLuint &texture); > - int debayerGPU(MappedFrameBuffer &in, int out_fd, DebayerParams ¶ms); > + int debayerGPU(MappedFrameBuffer &in, int out_fd, const DebayerParams ¶ms); > > /* Shader program identifiers */ > GLuint vertexShaderId_ = 0;
Milan Zamazal <mzamazal@redhat.com> writes: > Hi Barnabás, > > thank you for the patch. > > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> When calling `Debayer::process()` from `SoftwareIsp::process()`, the >> `DebayerParams` object is copied multiple time: >> >> (1) call of `BoundMethodMember<...>::activate()` >> inside `Object::invokeMethod()` >> (2) constructor of `BoundMethodArgs<...>` >> inside `BoundMethodMember<...>::activate()` >> (3) call of `BoundMethodMember<...>::invoke()` >> inside `BoundMethodArgs::invokePack()` >> (4) call of the actual pointer to member function >> inside `BoundMethodMember::invoke()` >> >> Given that the type has a size of 5696 bytes at the moment on x86-64, >> while the size is expected to shrink considerably and compilers might >> avoid one or two of the above copies, this is still not ideal. By making >> `Debayer::process()` take the parameter object by const lvalue reference, >> only the copy in the `BoundMethodArgs` constructor remains. So do that. > > Why RFC? Looks like harmless cleanup. > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/libcamera/software_isp/debayer.cpp | 2 +- >> src/libcamera/software_isp/debayer.h | 4 ++-- >> src/libcamera/software_isp/debayer_cpu.cpp | 2 +- >> src/libcamera/software_isp/debayer_cpu.h | 2 +- >> src/libcamera/software_isp/debayer_egl.cpp | 6 +++--- >> src/libcamera/software_isp/debayer_egl.h | 6 +++--- >> 6 files changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp >> index 65a1762dd..3e7702525 100644 >> --- a/src/libcamera/software_isp/debayer.cpp >> +++ b/src/libcamera/software_isp/debayer.cpp >> @@ -401,7 +401,7 @@ Debayer::~Debayer() >> * \brief Select the bayer params to use for the next frame debayer >> * \param[in] params The parameters to be used in debayering >> */ >> -void Debayer::setParams(DebayerParams ¶ms) >> +void Debayer::setParams(const DebayerParams ¶ms) >> { >> green_ = params.green; >> greenCcm_ = params.greenCcm; >> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h >> index cd2db9930..99e22ed85 100644 >> --- a/src/libcamera/software_isp/debayer.h >> +++ b/src/libcamera/software_isp/debayer.h >> @@ -47,7 +47,7 @@ public: >> virtual std::tuple<unsigned int, unsigned int> >> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; >> >> - virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; >> + virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) = 0; Well, this looks risky and I think it's better to avoid. >> virtual int start() { return 0; } >> virtual void stop() {} >> >> @@ -92,7 +92,7 @@ private: >> virtual Size patternSize(PixelFormat inputFormat) = 0; >> >> protected: >> - void setParams(DebayerParams ¶ms); >> + void setParams(const DebayerParams ¶ms); >> void dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *input, FrameBuffer *output); >> static bool isStandardBayerOrder(BayerFormat::Order order); >> }; >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >> index 00738c56b..b52f4d5c7 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.cpp >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >> @@ -740,7 +740,7 @@ void DebayerCpu::process4(uint32_t frame, const uint8_t *src, uint8_t *dst) >> } >> } >> >> -void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) >> +void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) >> { >> bench_.startFrame(); >> >> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h >> index 67df2b93a..27eff021d 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.h >> +++ b/src/libcamera/software_isp/debayer_cpu.h >> @@ -37,7 +37,7 @@ public: >> std::vector<PixelFormat> formats(PixelFormat input); >> std::tuple<unsigned int, unsigned int> >> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); >> - void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); >> + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms); >> SizeRange sizes(PixelFormat inputFormat, const Size &inputSize); >> const SharedFD &getStatsFD() { return stats_->getStatsFD(); } >> >> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp >> index 9693d7252..1ac162427 100644 >> --- a/src/libcamera/software_isp/debayer_egl.cpp >> +++ b/src/libcamera/software_isp/debayer_egl.cpp >> @@ -386,7 +386,7 @@ DebayerEGL::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size >> return std::make_tuple(stride, stride * size.height); >> } >> >> -void DebayerEGL::setShaderVariableValues(DebayerParams ¶ms) >> +void DebayerEGL::setShaderVariableValues(const DebayerParams ¶ms) >> { >> /* >> * Raw Bayer 8-bit, and packed raw Bayer 10-bit/12-bit formats >> @@ -509,7 +509,7 @@ void DebayerEGL::setShaderVariableValues(DebayerParams ¶ms) >> return; >> } >> >> -int DebayerEGL::debayerGPU(MappedFrameBuffer &in, int out_fd, DebayerParams ¶ms) >> +int DebayerEGL::debayerGPU(MappedFrameBuffer &in, int out_fd, const DebayerParams ¶ms) >> { >> /* eGL context switch */ >> egl_.makeCurrent(); >> @@ -536,7 +536,7 @@ int DebayerEGL::debayerGPU(MappedFrameBuffer &in, int out_fd, DebayerParams &par >> return 0; >> } >> >> -void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) >> +void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) >> { >> bench_.startFrame(); >> >> diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h >> index a5033bc63..790348249 100644 >> --- a/src/libcamera/software_isp/debayer_egl.h >> +++ b/src/libcamera/software_isp/debayer_egl.h >> @@ -50,7 +50,7 @@ public: >> std::vector<PixelFormat> formats(PixelFormat input); >> std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); >> >> - void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); >> + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms); >> int start(); >> void stop(); >> >> @@ -72,9 +72,9 @@ private: >> std::vector<std::string> shaderEnv); >> int linkShaderProgram(void); >> int getShaderVariableLocations(); >> - void setShaderVariableValues(DebayerParams ¶ms); >> + void setShaderVariableValues(const DebayerParams ¶ms); >> void configureTexture(GLuint &texture); >> - int debayerGPU(MappedFrameBuffer &in, int out_fd, DebayerParams ¶ms); >> + int debayerGPU(MappedFrameBuffer &in, int out_fd, const DebayerParams ¶ms); >> >> /* Shader program identifiers */ >> GLuint vertexShaderId_ = 0;
2026. 01. 22. 14:46 keltezéssel, Milan Zamazal írta: > Milan Zamazal <mzamazal@redhat.com> writes: > >> Hi Barnabás, >> >> thank you for the patch. >> >> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: >> >>> When calling `Debayer::process()` from `SoftwareIsp::process()`, the >>> `DebayerParams` object is copied multiple time: >>> >>> (1) call of `BoundMethodMember<...>::activate()` >>> inside `Object::invokeMethod()` >>> (2) constructor of `BoundMethodArgs<...>` >>> inside `BoundMethodMember<...>::activate()` >>> (3) call of `BoundMethodMember<...>::invoke()` >>> inside `BoundMethodArgs::invokePack()` >>> (4) call of the actual pointer to member function >>> inside `BoundMethodMember::invoke()` >>> >>> Given that the type has a size of 5696 bytes at the moment on x86-64, >>> while the size is expected to shrink considerably and compilers might >>> avoid one or two of the above copies, this is still not ideal. By making >>> `Debayer::process()` take the parameter object by const lvalue reference, >>> only the copy in the `BoundMethodArgs` constructor remains. So do that. >> >> Why RFC? Looks like harmless cleanup. >> >> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> >> >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>> --- >>> src/libcamera/software_isp/debayer.cpp | 2 +- >>> src/libcamera/software_isp/debayer.h | 4 ++-- >>> src/libcamera/software_isp/debayer_cpu.cpp | 2 +- >>> src/libcamera/software_isp/debayer_cpu.h | 2 +- >>> src/libcamera/software_isp/debayer_egl.cpp | 6 +++--- >>> src/libcamera/software_isp/debayer_egl.h | 6 +++--- >>> 6 files changed, 11 insertions(+), 11 deletions(-) >>> >>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp >>> index 65a1762dd..3e7702525 100644 >>> --- a/src/libcamera/software_isp/debayer.cpp >>> +++ b/src/libcamera/software_isp/debayer.cpp >>> @@ -401,7 +401,7 @@ Debayer::~Debayer() >>> * \brief Select the bayer params to use for the next frame debayer >>> * \param[in] params The parameters to be used in debayering >>> */ >>> -void Debayer::setParams(DebayerParams ¶ms) >>> +void Debayer::setParams(const DebayerParams ¶ms) >>> { >>> green_ = params.green; >>> greenCcm_ = params.greenCcm; >>> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h >>> index cd2db9930..99e22ed85 100644 >>> --- a/src/libcamera/software_isp/debayer.h >>> +++ b/src/libcamera/software_isp/debayer.h >>> @@ -47,7 +47,7 @@ public: >>> virtual std::tuple<unsigned int, unsigned int> >>> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; >>> >>> - virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; >>> + virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) = 0; > > Well, this looks risky and I think it's better to avoid. How so? > >>> virtual int start() { return 0; } >>> virtual void stop() {} >>> >>> @@ -92,7 +92,7 @@ private: >>> virtual Size patternSize(PixelFormat inputFormat) = 0; >>> >>> protected: >>> - void setParams(DebayerParams ¶ms); >>> + void setParams(const DebayerParams ¶ms); >>> void dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *input, FrameBuffer *output); >>> static bool isStandardBayerOrder(BayerFormat::Order order); > [...]
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 01. 22. 14:46 keltezéssel, Milan Zamazal írta: >> Milan Zamazal <mzamazal@redhat.com> writes: >> > >>> Hi Barnabás, >>> >>> thank you for the patch. >>> >>> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: >>> >>>> When calling `Debayer::process()` from `SoftwareIsp::process()`, the >>>> `DebayerParams` object is copied multiple time: >>>> >>>> (1) call of `BoundMethodMember<...>::activate()` >>>> inside `Object::invokeMethod()` >>>> (2) constructor of `BoundMethodArgs<...>` >>>> inside `BoundMethodMember<...>::activate()` >>>> (3) call of `BoundMethodMember<...>::invoke()` >>>> inside `BoundMethodArgs::invokePack()` >>>> (4) call of the actual pointer to member function >>>> inside `BoundMethodMember::invoke()` >>>> >>>> Given that the type has a size of 5696 bytes at the moment on x86-64, >>>> while the size is expected to shrink considerably and compilers might >>>> avoid one or two of the above copies, this is still not ideal. By making >>>> `Debayer::process()` take the parameter object by const lvalue reference, >>>> only the copy in the `BoundMethodArgs` constructor remains. So do that. >>> >>> Why RFC? Looks like harmless cleanup. >>> >>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> >>> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> src/libcamera/software_isp/debayer.cpp | 2 +- >>>> src/libcamera/software_isp/debayer.h | 4 ++-- >>>> src/libcamera/software_isp/debayer_cpu.cpp | 2 +- >>>> src/libcamera/software_isp/debayer_cpu.h | 2 +- >>>> src/libcamera/software_isp/debayer_egl.cpp | 6 +++--- >>>> src/libcamera/software_isp/debayer_egl.h | 6 +++--- >>>> 6 files changed, 11 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp >>>> index 65a1762dd..3e7702525 100644 >>>> --- a/src/libcamera/software_isp/debayer.cpp >>>> +++ b/src/libcamera/software_isp/debayer.cpp >>>> @@ -401,7 +401,7 @@ Debayer::~Debayer() >>>> * \brief Select the bayer params to use for the next frame debayer >>>> * \param[in] params The parameters to be used in debayering >>>> */ >>>> -void Debayer::setParams(DebayerParams ¶ms) >>>> +void Debayer::setParams(const DebayerParams ¶ms) >>>> { >>>> green_ = params.green; >>>> greenCcm_ = params.greenCcm; >>>> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h >>>> index cd2db9930..99e22ed85 100644 >>>> --- a/src/libcamera/software_isp/debayer.h >>>> +++ b/src/libcamera/software_isp/debayer.h >>>> @@ -47,7 +47,7 @@ public: >>>> virtual std::tuple<unsigned int, unsigned int> >>>> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; >>>> - virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams >>>> params) = 0; >>>> + virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) = 0; >> Well, this looks risky and I think it's better to avoid. > > How so? If debayering and algorithms run asynchronously and we don't have clear guarantees that params don't change while debayering can still consume them, we can get a subtle bug. Saving the copying wouldn't be worth the risk. >> >>>> virtual int start() { return 0; } >>>> virtual void stop() {} >>>> @@ -92,7 +92,7 @@ private: >>>> virtual Size patternSize(PixelFormat inputFormat) = 0; >>>> protected: >>>> - void setParams(DebayerParams ¶ms); >>>> + void setParams(const DebayerParams ¶ms); >>>> void dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *input, FrameBuffer *output); >>>> static bool isStandardBayerOrder(BayerFormat::Order order); >> [...]
2026. 01. 22. 16:09 keltezéssel, Milan Zamazal írta: > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> 2026. 01. 22. 14:46 keltezéssel, Milan Zamazal írta: >>> Milan Zamazal <mzamazal@redhat.com> writes: >>> >> >>>> Hi Barnabás, >>>> >>>> thank you for the patch. >>>> >>>> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: >>>> >>>>> When calling `Debayer::process()` from `SoftwareIsp::process()`, the >>>>> `DebayerParams` object is copied multiple time: >>>>> >>>>> (1) call of `BoundMethodMember<...>::activate()` >>>>> inside `Object::invokeMethod()` >>>>> (2) constructor of `BoundMethodArgs<...>` >>>>> inside `BoundMethodMember<...>::activate()` >>>>> (3) call of `BoundMethodMember<...>::invoke()` >>>>> inside `BoundMethodArgs::invokePack()` >>>>> (4) call of the actual pointer to member function >>>>> inside `BoundMethodMember::invoke()` >>>>> >>>>> Given that the type has a size of 5696 bytes at the moment on x86-64, >>>>> while the size is expected to shrink considerably and compilers might >>>>> avoid one or two of the above copies, this is still not ideal. By making >>>>> `Debayer::process()` take the parameter object by const lvalue reference, >>>>> only the copy in the `BoundMethodArgs` constructor remains. So do that. >>>> >>>> Why RFC? Looks like harmless cleanup. >>>> >>>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> >>>> >>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>> --- >>>>> src/libcamera/software_isp/debayer.cpp | 2 +- >>>>> src/libcamera/software_isp/debayer.h | 4 ++-- >>>>> src/libcamera/software_isp/debayer_cpu.cpp | 2 +- >>>>> src/libcamera/software_isp/debayer_cpu.h | 2 +- >>>>> src/libcamera/software_isp/debayer_egl.cpp | 6 +++--- >>>>> src/libcamera/software_isp/debayer_egl.h | 6 +++--- >>>>> 6 files changed, 11 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp >>>>> index 65a1762dd..3e7702525 100644 >>>>> --- a/src/libcamera/software_isp/debayer.cpp >>>>> +++ b/src/libcamera/software_isp/debayer.cpp >>>>> @@ -401,7 +401,7 @@ Debayer::~Debayer() >>>>> * \brief Select the bayer params to use for the next frame debayer >>>>> * \param[in] params The parameters to be used in debayering >>>>> */ >>>>> -void Debayer::setParams(DebayerParams ¶ms) >>>>> +void Debayer::setParams(const DebayerParams ¶ms) >>>>> { >>>>> green_ = params.green; >>>>> greenCcm_ = params.greenCcm; >>>>> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h >>>>> index cd2db9930..99e22ed85 100644 >>>>> --- a/src/libcamera/software_isp/debayer.h >>>>> +++ b/src/libcamera/software_isp/debayer.h >>>>> @@ -47,7 +47,7 @@ public: >>>>> virtual std::tuple<unsigned int, unsigned int> >>>>> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; >>>>> - virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams >>>>> params) = 0; >>>>> + virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) = 0; >>> Well, this looks risky and I think it's better to avoid. >> >> How so? > > If debayering and algorithms run asynchronously and we don't have clear > guarantees that params don't change while debayering can still consume > them, we can get a subtle bug. Saving the copying wouldn't be worth the > risk. One copy still remains. void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output) { ipa_->computeParams(frame); debayer_->invokeMethod(&Debayer::process, ConnectionTypeQueued, frame, input, output, debayerParams_); } here `Object::invokeMethod()` will make a copy of the `DebayerParams` object: * Arguments \a args passed by value or reference are copied, while pointers * are passed untouched. The caller shall ensure that any pointer argument * remains valid until the method is invoked. so I am fairly certain this should cause no issues. It mostly gets rid of the copies that happen inside the cross-thread calling mechanism. But there will still be one copy inside `invokeMethod()`. > >>> >>>>> virtual int start() { return 0; } >>>>> virtual void stop() {} >>>>> @@ -92,7 +92,7 @@ private: >>>>> virtual Size patternSize(PixelFormat inputFormat) = 0; >>>>> protected: >>>>> - void setParams(DebayerParams ¶ms); >>>>> + void setParams(const DebayerParams ¶ms); >>>>> void dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *input, FrameBuffer *output); >>>>> static bool isStandardBayerOrder(BayerFormat::Order order); >>> [...] >
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 01. 22. 16:09 keltezéssel, Milan Zamazal írta: >> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: >> > >>> 2026. 01. 22. 14:46 keltezéssel, Milan Zamazal írta: >>>> Milan Zamazal <mzamazal@redhat.com> writes: >>>> >>> >>>>> Hi Barnabás, >>>>> >>>>> thank you for the patch. >>>>> >>>>> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: >>>>> >>>>>> When calling `Debayer::process()` from `SoftwareIsp::process()`, the >>>>>> `DebayerParams` object is copied multiple time: >>>>>> >>>>>> (1) call of `BoundMethodMember<...>::activate()` >>>>>> inside `Object::invokeMethod()` >>>>>> (2) constructor of `BoundMethodArgs<...>` >>>>>> inside `BoundMethodMember<...>::activate()` >>>>>> (3) call of `BoundMethodMember<...>::invoke()` >>>>>> inside `BoundMethodArgs::invokePack()` >>>>>> (4) call of the actual pointer to member function >>>>>> inside `BoundMethodMember::invoke()` >>>>>> >>>>>> Given that the type has a size of 5696 bytes at the moment on x86-64, >>>>>> while the size is expected to shrink considerably and compilers might >>>>>> avoid one or two of the above copies, this is still not ideal. By making >>>>>> `Debayer::process()` take the parameter object by const lvalue reference, >>>>>> only the copy in the `BoundMethodArgs` constructor remains. So do that. >>>>> >>>>> Why RFC? Looks like harmless cleanup. >>>>> >>>>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> >>>>> >>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>>> --- >>>>>> src/libcamera/software_isp/debayer.cpp | 2 +- >>>>>> src/libcamera/software_isp/debayer.h | 4 ++-- >>>>>> src/libcamera/software_isp/debayer_cpu.cpp | 2 +- >>>>>> src/libcamera/software_isp/debayer_cpu.h | 2 +- >>>>>> src/libcamera/software_isp/debayer_egl.cpp | 6 +++--- >>>>>> src/libcamera/software_isp/debayer_egl.h | 6 +++--- >>>>>> 6 files changed, 11 insertions(+), 11 deletions(-) >>>>>> >>>>>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp >>>>>> index 65a1762dd..3e7702525 100644 >>>>>> --- a/src/libcamera/software_isp/debayer.cpp >>>>>> +++ b/src/libcamera/software_isp/debayer.cpp >>>>>> @@ -401,7 +401,7 @@ Debayer::~Debayer() >>>>>> * \brief Select the bayer params to use for the next frame debayer >>>>>> * \param[in] params The parameters to be used in debayering >>>>>> */ >>>>>> -void Debayer::setParams(DebayerParams ¶ms) >>>>>> +void Debayer::setParams(const DebayerParams ¶ms) >>>>>> { >>>>>> green_ = params.green; >>>>>> greenCcm_ = params.greenCcm; >>>>>> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h >>>>>> index cd2db9930..99e22ed85 100644 >>>>>> --- a/src/libcamera/software_isp/debayer.h >>>>>> +++ b/src/libcamera/software_isp/debayer.h >>>>>> @@ -47,7 +47,7 @@ public: >>>>>> virtual std::tuple<unsigned int, unsigned int> >>>>>> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; >>>>>> - virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams >>>>>> params) = 0; >>>>>> + virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) = 0; >>>> Well, this looks risky and I think it's better to avoid. >>> >>> How so? >> If debayering and algorithms run asynchronously and we don't have clear >> guarantees that params don't change while debayering can still consume >> them, we can get a subtle bug. Saving the copying wouldn't be worth the >> risk. > > One copy still remains. > > void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output) > { > ipa_->computeParams(frame); > debayer_->invokeMethod(&Debayer::process, > ConnectionTypeQueued, frame, input, output, debayerParams_); > } > > here `Object::invokeMethod()` will make a copy of the `DebayerParams` object: > > * Arguments \a args passed by value or reference are copied, while pointers > * are passed untouched. The caller shall ensure that any pointer argument > * remains valid until the method is invoked. > > so I am fairly certain this should cause no issues. It mostly gets > rid of the copies that happen inside the cross-thread calling mechanism. > But there will still be one copy inside `invokeMethod()`. Ah, right, thank you for clarification. Sorry for the noise and not reading the commit message carefully enough. >> >>>> >>>>>> virtual int start() { return 0; } >>>>>> virtual void stop() {} >>>>>> @@ -92,7 +92,7 @@ private: >>>>>> virtual Size patternSize(PixelFormat inputFormat) = 0; >>>>>> protected: >>>>>> - void setParams(DebayerParams ¶ms); >>>>>> + void setParams(const DebayerParams ¶ms); >>>>>> void dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *input, FrameBuffer *output); >>>>>> static bool isStandardBayerOrder(BayerFormat::Order order); >>>> [...] >>
On 22/01/2026 15:18, Barnabás Pőcze wrote: > One copy still remains. > > void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output) > { > ipa_->computeParams(frame); > debayer_->invokeMethod(&Debayer::process, > ConnectionTypeQueued, frame, input, output, debayerParams_); > } We should really have code we know to be thread-safe - or not - whatever the case may be. A copy is a low-intervention version of thread synchronisation though. Assuming you've tested this with gpuisp. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- bod
diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp index 65a1762dd..3e7702525 100644 --- a/src/libcamera/software_isp/debayer.cpp +++ b/src/libcamera/software_isp/debayer.cpp @@ -401,7 +401,7 @@ Debayer::~Debayer() * \brief Select the bayer params to use for the next frame debayer * \param[in] params The parameters to be used in debayering */ -void Debayer::setParams(DebayerParams ¶ms) +void Debayer::setParams(const DebayerParams ¶ms) { green_ = params.green; greenCcm_ = params.greenCcm; diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h index cd2db9930..99e22ed85 100644 --- a/src/libcamera/software_isp/debayer.h +++ b/src/libcamera/software_isp/debayer.h @@ -47,7 +47,7 @@ public: virtual std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; - virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; + virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) = 0; virtual int start() { return 0; } virtual void stop() {} @@ -92,7 +92,7 @@ private: virtual Size patternSize(PixelFormat inputFormat) = 0; protected: - void setParams(DebayerParams ¶ms); + void setParams(const DebayerParams ¶ms); void dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *input, FrameBuffer *output); static bool isStandardBayerOrder(BayerFormat::Order order); }; diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index 00738c56b..b52f4d5c7 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -740,7 +740,7 @@ void DebayerCpu::process4(uint32_t frame, const uint8_t *src, uint8_t *dst) } } -void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) +void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) { bench_.startFrame(); diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h index 67df2b93a..27eff021d 100644 --- a/src/libcamera/software_isp/debayer_cpu.h +++ b/src/libcamera/software_isp/debayer_cpu.h @@ -37,7 +37,7 @@ public: std::vector<PixelFormat> formats(PixelFormat input); std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); - void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms); SizeRange sizes(PixelFormat inputFormat, const Size &inputSize); const SharedFD &getStatsFD() { return stats_->getStatsFD(); } diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp index 9693d7252..1ac162427 100644 --- a/src/libcamera/software_isp/debayer_egl.cpp +++ b/src/libcamera/software_isp/debayer_egl.cpp @@ -386,7 +386,7 @@ DebayerEGL::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size return std::make_tuple(stride, stride * size.height); } -void DebayerEGL::setShaderVariableValues(DebayerParams ¶ms) +void DebayerEGL::setShaderVariableValues(const DebayerParams ¶ms) { /* * Raw Bayer 8-bit, and packed raw Bayer 10-bit/12-bit formats @@ -509,7 +509,7 @@ void DebayerEGL::setShaderVariableValues(DebayerParams ¶ms) return; } -int DebayerEGL::debayerGPU(MappedFrameBuffer &in, int out_fd, DebayerParams ¶ms) +int DebayerEGL::debayerGPU(MappedFrameBuffer &in, int out_fd, const DebayerParams ¶ms) { /* eGL context switch */ egl_.makeCurrent(); @@ -536,7 +536,7 @@ int DebayerEGL::debayerGPU(MappedFrameBuffer &in, int out_fd, DebayerParams &par return 0; } -void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) +void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) { bench_.startFrame(); diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h index a5033bc63..790348249 100644 --- a/src/libcamera/software_isp/debayer_egl.h +++ b/src/libcamera/software_isp/debayer_egl.h @@ -50,7 +50,7 @@ public: std::vector<PixelFormat> formats(PixelFormat input); std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); - void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms); int start(); void stop(); @@ -72,9 +72,9 @@ private: std::vector<std::string> shaderEnv); int linkShaderProgram(void); int getShaderVariableLocations(); - void setShaderVariableValues(DebayerParams ¶ms); + void setShaderVariableValues(const DebayerParams ¶ms); void configureTexture(GLuint &texture); - int debayerGPU(MappedFrameBuffer &in, int out_fd, DebayerParams ¶ms); + int debayerGPU(MappedFrameBuffer &in, int out_fd, const DebayerParams ¶ms); /* Shader program identifiers */ GLuint vertexShaderId_ = 0;
When calling `Debayer::process()` from `SoftwareIsp::process()`, the `DebayerParams` object is copied multiple time: (1) call of `BoundMethodMember<...>::activate()` inside `Object::invokeMethod()` (2) constructor of `BoundMethodArgs<...>` inside `BoundMethodMember<...>::activate()` (3) call of `BoundMethodMember<...>::invoke()` inside `BoundMethodArgs::invokePack()` (4) call of the actual pointer to member function inside `BoundMethodMember::invoke()` Given that the type has a size of 5696 bytes at the moment on x86-64, while the size is expected to shrink considerably and compilers might avoid one or two of the above copies, this is still not ideal. By making `Debayer::process()` take the parameter object by const lvalue reference, only the copy in the `BoundMethodArgs` constructor remains. So do that. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/libcamera/software_isp/debayer.cpp | 2 +- src/libcamera/software_isp/debayer.h | 4 ++-- src/libcamera/software_isp/debayer_cpu.cpp | 2 +- src/libcamera/software_isp/debayer_cpu.h | 2 +- src/libcamera/software_isp/debayer_egl.cpp | 6 +++--- src/libcamera/software_isp/debayer_egl.h | 6 +++--- 6 files changed, 11 insertions(+), 11 deletions(-)