| Message ID | 20260811130042.213139-8-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > Currently each `start()` invocation causes a new EGL context to > be acquired, and only the last allocation will be properly freed > in the `eGL` destructor. > > To avoid that, add and use `eGL::resetEGLContext()` in `stop()`; > and also make `eGL` destructor stricter by requring no active > EGL context. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/internal/egl.h | 1 + > src/libcamera/egl.cpp | 27 +++++++++++++++++++--- > src/libcamera/software_isp/debayer_egl.cpp | 2 ++ > 3 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h > index 608d53151c..8f2ef296c3 100644 > --- a/include/libcamera/internal/egl.h > +++ b/include/libcamera/internal/egl.h > @@ -104,6 +104,7 @@ public: > ~eGL(); > > int initEGLContext(); > + void resetEGLContext(); > static EGLDisplay probeDisplay(); > > int createInputDMABufTexture2D(eGLImage &eglImage, int fd); > diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp > index 2a1f577845..1f9e743c2f 100644 > --- a/src/libcamera/egl.cpp > +++ b/src/libcamera/egl.cpp > @@ -73,12 +73,13 @@ eGL::eGL(EGLDisplay display) > /** > * \brief Destroy the EGL helper > * > - * Destroys the EGL context and surface if they were successfully created. > + * An instance must only be destroyed if there is no active EGL context. > + * > + * \sa resetEGLContext() > */ > eGL::~eGL() > { > - if (context_ != EGL_NO_CONTEXT) > - eglDestroyContext(display_, context_); > + ASSERT(context_ == EGL_NO_CONTEXT); > } > > /** > @@ -368,6 +369,8 @@ void eGL::createOutputTexture2D(eGLImage &eglImage) > * - eglCreateImageKHR / eglDestroyImageKHR > * - glEGLImageTargetTexture2DOES > * > + * Each successful invocation must be followed by a call to resetEGLContext(). > + * > * \return 0 on success, or -ENODEV on failure > */ > int eGL::initEGLContext() > @@ -444,6 +447,24 @@ fail: > return -ENODEV; > } > > +/** > + * \brief Destroy the EGL context > + * > + * This function destroys the EGL context created by initEGLContext(). > + * If there is no valid EGL context, this function has no effect. Not exactly, it still resets the member variables. > + */ > +void eGL::resetEGLContext() > +{ > + if (context_ != EGL_NO_CONTEXT) { Is `context_ == EGL_NO_CONTEXT' a regular situation or does it deserve logging a warning here? Anyway, with whatever clarification: Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > + assertThread(), > + eglMakeCurrent(display_, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); > + eglDestroyContext(display_, std::exchange(context_, EGL_NO_CONTEXT)); > + } > + > + tid_ = -1; > + vtable_ = {}; > +} > + > /** > * \brief Make the EGL context current for the calling thread > * > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > index 7b2b367305..97aa037935 100644 > --- a/src/libcamera/software_isp/debayer_egl.cpp > +++ b/src/libcamera/software_isp/debayer_egl.cpp > @@ -680,6 +680,8 @@ void DebayerEGL::stop() > > if (programId_) > glDeleteProgram(programId_); > + > + egl_.resetEGLContext(); > } > > SizeRange DebayerEGL::sizes(PixelFormat inputFormat, const Size &inputSize)
2026. 08. 11. 16:52 keltezéssel, Milan Zamazal írta: > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> Currently each `start()` invocation causes a new EGL context to >> be acquired, and only the last allocation will be properly freed >> in the `eGL` destructor. >> >> To avoid that, add and use `eGL::resetEGLContext()` in `stop()`; >> and also make `eGL` destructor stricter by requring no active >> EGL context. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/internal/egl.h | 1 + >> src/libcamera/egl.cpp | 27 +++++++++++++++++++--- >> src/libcamera/software_isp/debayer_egl.cpp | 2 ++ >> 3 files changed, 27 insertions(+), 3 deletions(-) >> >> diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h >> index 608d53151c..8f2ef296c3 100644 >> --- a/include/libcamera/internal/egl.h >> +++ b/include/libcamera/internal/egl.h >> @@ -104,6 +104,7 @@ public: >> ~eGL(); >> >> int initEGLContext(); >> + void resetEGLContext(); >> static EGLDisplay probeDisplay(); >> >> int createInputDMABufTexture2D(eGLImage &eglImage, int fd); >> diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp >> index 2a1f577845..1f9e743c2f 100644 >> --- a/src/libcamera/egl.cpp >> +++ b/src/libcamera/egl.cpp >> @@ -73,12 +73,13 @@ eGL::eGL(EGLDisplay display) >> /** >> * \brief Destroy the EGL helper >> * >> - * Destroys the EGL context and surface if they were successfully created. >> + * An instance must only be destroyed if there is no active EGL context. >> + * >> + * \sa resetEGLContext() >> */ >> eGL::~eGL() >> { >> - if (context_ != EGL_NO_CONTEXT) >> - eglDestroyContext(display_, context_); >> + ASSERT(context_ == EGL_NO_CONTEXT); >> } >> >> /** >> @@ -368,6 +369,8 @@ void eGL::createOutputTexture2D(eGLImage &eglImage) >> * - eglCreateImageKHR / eglDestroyImageKHR >> * - glEGLImageTargetTexture2DOES >> * >> + * Each successful invocation must be followed by a call to resetEGLContext(). >> + * >> * \return 0 on success, or -ENODEV on failure >> */ >> int eGL::initEGLContext() >> @@ -444,6 +447,24 @@ fail: >> return -ENODEV; >> } >> >> +/** >> + * \brief Destroy the EGL context >> + * >> + * This function destroys the EGL context created by initEGLContext(). >> + * If there is no valid EGL context, this function has no effect. > > Not exactly, it still resets the member variables. True, my thinking was that in that case member variables should already be "reset", so no real "observable" effect. > >> + */ >> +void eGL::resetEGLContext() >> +{ >> + if (context_ != EGL_NO_CONTEXT) { > > Is `context_ == EGL_NO_CONTEXT' a regular situation or does it deserve > logging a warning here? At the moment? No, it shouldn't happen. The motivation here was something like `std::{unique,shared}_ptr::reset()`, which are idempotent; and I believe one cannot usually go wrong by making these "reset"-like functions idempotent. > > Anyway, with whatever clarification: > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > >> + assertThread(), >> + eglMakeCurrent(display_, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); >> + eglDestroyContext(display_, std::exchange(context_, EGL_NO_CONTEXT)); >> + } >> + >> + tid_ = -1; >> + vtable_ = {}; >> +} >> + >> /** >> * \brief Make the EGL context current for the calling thread >> * >> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp >> index 7b2b367305..97aa037935 100644 >> --- a/src/libcamera/software_isp/debayer_egl.cpp >> +++ b/src/libcamera/software_isp/debayer_egl.cpp >> @@ -680,6 +680,8 @@ void DebayerEGL::stop() >> >> if (programId_) >> glDeleteProgram(programId_); >> + >> + egl_.resetEGLContext(); >> } >> >> SizeRange DebayerEGL::sizes(PixelFormat inputFormat, const Size &inputSize) >
Hi Barnabás On Tue, Aug 11, 2026 at 03:00:41PM +0200, Barnabás Pőcze wrote: > Currently each `start()` invocation causes a new EGL context to > be acquired, and only the last allocation will be properly freed > in the `eGL` destructor. > > To avoid that, add and use `eGL::resetEGLContext()` in `stop()`; > and also make `eGL` destructor stricter by requring no active > EGL context. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > include/libcamera/internal/egl.h | 1 + > src/libcamera/egl.cpp | 27 +++++++++++++++++++--- > src/libcamera/software_isp/debayer_egl.cpp | 2 ++ > 3 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h > index 608d53151c..8f2ef296c3 100644 > --- a/include/libcamera/internal/egl.h > +++ b/include/libcamera/internal/egl.h > @@ -104,6 +104,7 @@ public: > ~eGL(); > > int initEGLContext(); > + void resetEGLContext(); > static EGLDisplay probeDisplay(); > > int createInputDMABufTexture2D(eGLImage &eglImage, int fd); > diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp > index 2a1f577845..1f9e743c2f 100644 > --- a/src/libcamera/egl.cpp > +++ b/src/libcamera/egl.cpp > @@ -73,12 +73,13 @@ eGL::eGL(EGLDisplay display) > /** > * \brief Destroy the EGL helper > * > - * Destroys the EGL context and surface if they were successfully created. > + * An instance must only be destroyed if there is no active EGL context. > + * > + * \sa resetEGLContext() > */ > eGL::~eGL() > { > - if (context_ != EGL_NO_CONTEXT) > - eglDestroyContext(display_, context_); > + ASSERT(context_ == EGL_NO_CONTEXT); > } > > /** > @@ -368,6 +369,8 @@ void eGL::createOutputTexture2D(eGLImage &eglImage) > * - eglCreateImageKHR / eglDestroyImageKHR > * - glEGLImageTargetTexture2DOES > * > + * Each successful invocation must be followed by a call to resetEGLContext(). > + * > * \return 0 on success, or -ENODEV on failure > */ > int eGL::initEGLContext() > @@ -444,6 +447,24 @@ fail: > return -ENODEV; > } > > +/** > + * \brief Destroy the EGL context > + * > + * This function destroys the EGL context created by initEGLContext(). > + * If there is no valid EGL context, this function has no effect. > + */ > +void eGL::resetEGLContext() > +{ > + if (context_ != EGL_NO_CONTEXT) { > + assertThread(), > + eglMakeCurrent(display_, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); > + eglDestroyContext(display_, std::exchange(context_, EGL_NO_CONTEXT)); > + } > + > + tid_ = -1; > + vtable_ = {}; > +} > + > /** > * \brief Make the EGL context current for the calling thread > * > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > index 7b2b367305..97aa037935 100644 > --- a/src/libcamera/software_isp/debayer_egl.cpp > +++ b/src/libcamera/software_isp/debayer_egl.cpp > @@ -680,6 +680,8 @@ void DebayerEGL::stop() > > if (programId_) > glDeleteProgram(programId_); > + > + egl_.resetEGLContext(); > } > > SizeRange DebayerEGL::sizes(PixelFormat inputFormat, const Size &inputSize) > -- > 2.55.0 >
diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h index 608d53151c..8f2ef296c3 100644 --- a/include/libcamera/internal/egl.h +++ b/include/libcamera/internal/egl.h @@ -104,6 +104,7 @@ public: ~eGL(); int initEGLContext(); + void resetEGLContext(); static EGLDisplay probeDisplay(); int createInputDMABufTexture2D(eGLImage &eglImage, int fd); diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp index 2a1f577845..1f9e743c2f 100644 --- a/src/libcamera/egl.cpp +++ b/src/libcamera/egl.cpp @@ -73,12 +73,13 @@ eGL::eGL(EGLDisplay display) /** * \brief Destroy the EGL helper * - * Destroys the EGL context and surface if they were successfully created. + * An instance must only be destroyed if there is no active EGL context. + * + * \sa resetEGLContext() */ eGL::~eGL() { - if (context_ != EGL_NO_CONTEXT) - eglDestroyContext(display_, context_); + ASSERT(context_ == EGL_NO_CONTEXT); } /** @@ -368,6 +369,8 @@ void eGL::createOutputTexture2D(eGLImage &eglImage) * - eglCreateImageKHR / eglDestroyImageKHR * - glEGLImageTargetTexture2DOES * + * Each successful invocation must be followed by a call to resetEGLContext(). + * * \return 0 on success, or -ENODEV on failure */ int eGL::initEGLContext() @@ -444,6 +447,24 @@ fail: return -ENODEV; } +/** + * \brief Destroy the EGL context + * + * This function destroys the EGL context created by initEGLContext(). + * If there is no valid EGL context, this function has no effect. + */ +void eGL::resetEGLContext() +{ + if (context_ != EGL_NO_CONTEXT) { + assertThread(), + eglMakeCurrent(display_, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); + eglDestroyContext(display_, std::exchange(context_, EGL_NO_CONTEXT)); + } + + tid_ = -1; + vtable_ = {}; +} + /** * \brief Make the EGL context current for the calling thread * diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp index 7b2b367305..97aa037935 100644 --- a/src/libcamera/software_isp/debayer_egl.cpp +++ b/src/libcamera/software_isp/debayer_egl.cpp @@ -680,6 +680,8 @@ void DebayerEGL::stop() if (programId_) glDeleteProgram(programId_); + + egl_.resetEGLContext(); } SizeRange DebayerEGL::sizes(PixelFormat inputFormat, const Size &inputSize)
Currently each `start()` invocation causes a new EGL context to be acquired, and only the last allocation will be properly freed in the `eGL` destructor. To avoid that, add and use `eGL::resetEGLContext()` in `stop()`; and also make `eGL` destructor stricter by requring no active EGL context. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/internal/egl.h | 1 + src/libcamera/egl.cpp | 27 +++++++++++++++++++--- src/libcamera/software_isp/debayer_egl.cpp | 2 ++ 3 files changed, 27 insertions(+), 3 deletions(-)