| Message ID | 20260810105942.1098192-6-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > Add a function that undoes the effects of `initEGLContext()`. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > --- > include/libcamera/internal/egl.h | 1 + > src/libcamera/egl.cpp | 25 ++++++++++++++++++++----- > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h > index fb77225ac4..e18bb91f4d 100644 > --- a/include/libcamera/internal/egl.h > +++ b/include/libcamera/internal/egl.h > @@ -103,6 +103,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 3de5fab816..237791bca1 100644 > --- a/src/libcamera/egl.cpp > +++ b/src/libcamera/egl.cpp > @@ -71,14 +71,11 @@ eGL::eGL(EGLDisplay display) > } > > /** > - * \brief Destroy the EGL helper > - * > - * Destroys the EGL context and surface if they were successfully created. > + * \brief Release all EGL resources > */ > eGL::~eGL() > { > - if (context_ != EGL_NO_CONTEXT) > - eglDestroyContext(display_, context_); > + resetEGLContext(); > } > > /** > @@ -436,6 +433,24 @@ fail: > return -ENODEV; > } > > +/** > + * \brief Destroy the EGL context > + * > + * This function destroys the EGL context created by initEGLContext(). > + */ > +void eGL::resetEGLContext() > +{ > + if (context_ != EGL_NO_CONTEXT) { > + if (tid_ == Thread::currentId()) > + 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 > *
On Mon, Aug 10, 2026 at 12:59:39PM +0200, Barnabás Pőcze wrote: > Add a function that undoes the effects of `initEGLContext()`. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/internal/egl.h | 1 + > src/libcamera/egl.cpp | 25 ++++++++++++++++++++----- > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h > index fb77225ac4..e18bb91f4d 100644 > --- a/include/libcamera/internal/egl.h > +++ b/include/libcamera/internal/egl.h > @@ -103,6 +103,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 3de5fab816..237791bca1 100644 > --- a/src/libcamera/egl.cpp > +++ b/src/libcamera/egl.cpp > @@ -71,14 +71,11 @@ eGL::eGL(EGLDisplay display) > } > > /** > - * \brief Destroy the EGL helper > - * > - * Destroys the EGL context and surface if they were successfully created. > + * \brief Release all EGL resources > */ > eGL::~eGL() > { > - if (context_ != EGL_NO_CONTEXT) > - eglDestroyContext(display_, context_); > + resetEGLContext(); > } > > /** > @@ -436,6 +433,24 @@ fail: > return -ENODEV; > } > > +/** > + * \brief Destroy the EGL context > + * > + * This function destroys the EGL context created by initEGLContext(). Let's clearly say each initContext() invocation has to be balanced by a resetEGLContext() call Which makes me wonder, since the eGL class constructor doesn't call initEGLContext(), does the destructor need to call resetEGLContext() ? I think it doesn't hurt and was there already. > + */ > +void eGL::resetEGLContext() > +{ > + if (context_ != EGL_NO_CONTEXT) { > + if (tid_ == Thread::currentId()) All other usages of tid_ are assertions. Why is this different ? Do you expect other threads to call this function ? > + eglMakeCurrent(display_, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); > + > + eglDestroyContext(display_, std::exchange(context_, EGL_NO_CONTEXT)); > + } > + > + tid_ = -1; > + vtable_ = {}; I would have only changed the class destructor in this patch and introduce eGL::resetEGLContext() in the next patch where you modify the DebayerEGL class to call this function on stop(). > +} > + > /** > * \brief Make the EGL context current for the calling thread > * > -- > 2.55.0 >
2026. 08. 11. 11:30 keltezéssel, Jacopo Mondi írta: > On Mon, Aug 10, 2026 at 12:59:39PM +0200, Barnabás Pőcze wrote: >> Add a function that undoes the effects of `initEGLContext()`. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/internal/egl.h | 1 + >> src/libcamera/egl.cpp | 25 ++++++++++++++++++++----- >> 2 files changed, 21 insertions(+), 5 deletions(-) >> >> diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h >> index fb77225ac4..e18bb91f4d 100644 >> --- a/include/libcamera/internal/egl.h >> +++ b/include/libcamera/internal/egl.h >> @@ -103,6 +103,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 3de5fab816..237791bca1 100644 >> --- a/src/libcamera/egl.cpp >> +++ b/src/libcamera/egl.cpp >> @@ -71,14 +71,11 @@ eGL::eGL(EGLDisplay display) >> } >> >> /** >> - * \brief Destroy the EGL helper >> - * >> - * Destroys the EGL context and surface if they were successfully created. >> + * \brief Release all EGL resources >> */ >> eGL::~eGL() >> { >> - if (context_ != EGL_NO_CONTEXT) >> - eglDestroyContext(display_, context_); >> + resetEGLContext(); >> } >> >> /** >> @@ -436,6 +433,24 @@ fail: >> return -ENODEV; >> } >> >> +/** >> + * \brief Destroy the EGL context >> + * >> + * This function destroys the EGL context created by initEGLContext(). > > Let's clearly say each initContext() invocation has to be balanced by > a resetEGLContext() call Well, that depends on what the destructor does (see below). > > Which makes me wonder, since the eGL class constructor doesn't call > initEGLContext(), does the destructor need to call resetEGLContext() ? > > I think it doesn't hurt and was there already. > >> + */ >> +void eGL::resetEGLContext() >> +{ >> + if (context_ != EGL_NO_CONTEXT) { >> + if (tid_ == Thread::currentId()) > > All other usages of tid_ are assertions. Why is this different ? Do > you expect other threads to call this function ? This is somewhat related to the previous question. The reason is that it is called from the destructor. If we add `ASSERT(context_ == EGL_NO_CONTEXT)` to the destructor instead, then this function can also assert `tid_`. > >> + eglMakeCurrent(display_, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); >> + >> + eglDestroyContext(display_, std::exchange(context_, EGL_NO_CONTEXT)); >> + } >> + >> + tid_ = -1; >> + vtable_ = {}; > > I would have only changed the class destructor in this patch and > introduce eGL::resetEGLContext() in the next patch where you modify > the DebayerEGL class to call this function on stop(). > >> +} >> + >> /** >> * \brief Make the EGL context current for the calling thread >> * >> -- >> 2.55.0 >>
Hi Barnabás On Tue, Aug 11, 2026 at 11:41:06AM +0200, Barnabás Pőcze wrote: > 2026. 08. 11. 11:30 keltezéssel, Jacopo Mondi írta: > > On Mon, Aug 10, 2026 at 12:59:39PM +0200, Barnabás Pőcze wrote: > > > Add a function that undoes the effects of `initEGLContext()`. > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > include/libcamera/internal/egl.h | 1 + > > > src/libcamera/egl.cpp | 25 ++++++++++++++++++++----- > > > 2 files changed, 21 insertions(+), 5 deletions(-) > > > > > > diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h > > > index fb77225ac4..e18bb91f4d 100644 > > > --- a/include/libcamera/internal/egl.h > > > +++ b/include/libcamera/internal/egl.h > > > @@ -103,6 +103,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 3de5fab816..237791bca1 100644 > > > --- a/src/libcamera/egl.cpp > > > +++ b/src/libcamera/egl.cpp > > > @@ -71,14 +71,11 @@ eGL::eGL(EGLDisplay display) > > > } > > > > > > /** > > > - * \brief Destroy the EGL helper > > > - * > > > - * Destroys the EGL context and surface if they were successfully created. > > > + * \brief Release all EGL resources > > > */ > > > eGL::~eGL() > > > { > > > - if (context_ != EGL_NO_CONTEXT) > > > - eglDestroyContext(display_, context_); > > > + resetEGLContext(); > > > } > > > > > > /** > > > @@ -436,6 +433,24 @@ fail: > > > return -ENODEV; > > > } > > > > > > +/** > > > + * \brief Destroy the EGL context > > > + * > > > + * This function destroys the EGL context created by initEGLContext(). > > > > Let's clearly say each initContext() invocation has to be balanced by > > a resetEGLContext() call > > Well, that depends on what the destructor does (see below). > I might have missed why > > > > > Which makes me wonder, since the eGL class constructor doesn't call > > initEGLContext(), does the destructor need to call resetEGLContext() ? > > > > I think it doesn't hurt and was there already. > > > > > + */ > > > +void eGL::resetEGLContext() > > > +{ > > > + if (context_ != EGL_NO_CONTEXT) { > > > + if (tid_ == Thread::currentId()) > > > > All other usages of tid_ are assertions. Why is this different ? Do > > you expect other threads to call this function ? > > This is somewhat related to the previous question. The reason is that it is called > from the destructor. If we add `ASSERT(context_ == EGL_NO_CONTEXT)` to the destructor Would the assertion enforce that all calls to init are balanced ? > instead, then this function can also assert `tid_`. Oh I see. I would keep it the way it is then, but anyway require every init to be balanced by a reset. If that doesn't happen and at destruction time context_ != EGL_NO_CONTEXT we could Warn and call reset anyway to not leak resources ? > > > > > > > + eglMakeCurrent(display_, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); > > > + > > > + eglDestroyContext(display_, std::exchange(context_, EGL_NO_CONTEXT)); > > > + } > > > + > > > + tid_ = -1; > > > + vtable_ = {}; > > > > I would have only changed the class destructor in this patch and > > introduce eGL::resetEGLContext() in the next patch where you modify > > the DebayerEGL class to call this function on stop(). > > > > > +} > > > + > > > /** > > > * \brief Make the EGL context current for the calling thread > > > * > > > -- > > > 2.55.0 > > > >
2026. 08. 11. 12:01 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Tue, Aug 11, 2026 at 11:41:06AM +0200, Barnabás Pőcze wrote: >> 2026. 08. 11. 11:30 keltezéssel, Jacopo Mondi írta: >>> On Mon, Aug 10, 2026 at 12:59:39PM +0200, Barnabás Pőcze wrote: >>>> Add a function that undoes the effects of `initEGLContext()`. >>>> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> include/libcamera/internal/egl.h | 1 + >>>> src/libcamera/egl.cpp | 25 ++++++++++++++++++++----- >>>> 2 files changed, 21 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h >>>> index fb77225ac4..e18bb91f4d 100644 >>>> --- a/include/libcamera/internal/egl.h >>>> +++ b/include/libcamera/internal/egl.h >>>> @@ -103,6 +103,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 3de5fab816..237791bca1 100644 >>>> --- a/src/libcamera/egl.cpp >>>> +++ b/src/libcamera/egl.cpp >>>> @@ -71,14 +71,11 @@ eGL::eGL(EGLDisplay display) >>>> } >>>> >>>> /** >>>> - * \brief Destroy the EGL helper >>>> - * >>>> - * Destroys the EGL context and surface if they were successfully created. >>>> + * \brief Release all EGL resources >>>> */ >>>> eGL::~eGL() >>>> { >>>> - if (context_ != EGL_NO_CONTEXT) >>>> - eglDestroyContext(display_, context_); >>>> + resetEGLContext(); >>>> } >>>> >>>> /** >>>> @@ -436,6 +433,24 @@ fail: >>>> return -ENODEV; >>>> } >>>> >>>> +/** >>>> + * \brief Destroy the EGL context >>>> + * >>>> + * This function destroys the EGL context created by initEGLContext(). >>> >>> Let's clearly say each initContext() invocation has to be balanced by >>> a resetEGLContext() call >> >> Well, that depends on what the destructor does (see below). >> > > I might have missed why I meant that if the destructor calls `resetEGLContext()`, then it does not need to be fully balanced because the descriptor takes care of it. >> >>> >>> Which makes me wonder, since the eGL class constructor doesn't call >>> initEGLContext(), does the destructor need to call resetEGLContext() ? >>> >>> I think it doesn't hurt and was there already. >>> >>>> + */ >>>> +void eGL::resetEGLContext() >>>> +{ >>>> + if (context_ != EGL_NO_CONTEXT) { >>>> + if (tid_ == Thread::currentId()) >>> >>> All other usages of tid_ are assertions. Why is this different ? Do >>> you expect other threads to call this function ? >> >> This is somewhat related to the previous question. The reason is that it is called >> from the destructor. If we add `ASSERT(context_ == EGL_NO_CONTEXT)` to the destructor > > Would the assertion enforce that all calls to init are balanced ? Yes. > >> instead, then this function can also assert `tid_`. > > Oh I see. I would keep it the way it is then, but anyway require every > init to be balanced by a reset. If that doesn't happen and at > destruction time context_ != EGL_NO_CONTEXT we could Warn and call > reset anyway to not leak resources ? With the current use case, init/reset calls are balanced and so the assertion would be fine. And now that you mentioned it, I think I'd prefer that over the other approaches. > >> >> >>> >>>> + eglMakeCurrent(display_, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); >>>> + >>>> + eglDestroyContext(display_, std::exchange(context_, EGL_NO_CONTEXT)); >>>> + } >>>> + >>>> + tid_ = -1; >>>> + vtable_ = {}; >>> >>> I would have only changed the class destructor in this patch and >>> introduce eGL::resetEGLContext() in the next patch where you modify >>> the DebayerEGL class to call this function on stop(). >>> >>>> +} >>>> + >>>> /** >>>> * \brief Make the EGL context current for the calling thread >>>> * >>>> -- >>>> 2.55.0 >>>> >>
Hi Barnabás On Tue, Aug 11, 2026 at 12:22:01PM +0200, Barnabás Pőcze wrote: > 2026. 08. 11. 12:01 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Tue, Aug 11, 2026 at 11:41:06AM +0200, Barnabás Pőcze wrote: > > > 2026. 08. 11. 11:30 keltezéssel, Jacopo Mondi írta: > > > > On Mon, Aug 10, 2026 at 12:59:39PM +0200, Barnabás Pőcze wrote: > > > > > Add a function that undoes the effects of `initEGLContext()`. > > > > > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > --- > > > > > include/libcamera/internal/egl.h | 1 + > > > > > src/libcamera/egl.cpp | 25 ++++++++++++++++++++----- > > > > > 2 files changed, 21 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h > > > > > index fb77225ac4..e18bb91f4d 100644 > > > > > --- a/include/libcamera/internal/egl.h > > > > > +++ b/include/libcamera/internal/egl.h > > > > > @@ -103,6 +103,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 3de5fab816..237791bca1 100644 > > > > > --- a/src/libcamera/egl.cpp > > > > > +++ b/src/libcamera/egl.cpp > > > > > @@ -71,14 +71,11 @@ eGL::eGL(EGLDisplay display) > > > > > } > > > > > > > > > > /** > > > > > - * \brief Destroy the EGL helper > > > > > - * > > > > > - * Destroys the EGL context and surface if they were successfully created. > > > > > + * \brief Release all EGL resources > > > > > */ > > > > > eGL::~eGL() > > > > > { > > > > > - if (context_ != EGL_NO_CONTEXT) > > > > > - eglDestroyContext(display_, context_); > > > > > + resetEGLContext(); > > > > > } > > > > > > > > > > /** > > > > > @@ -436,6 +433,24 @@ fail: > > > > > return -ENODEV; > > > > > } > > > > > > > > > > +/** > > > > > + * \brief Destroy the EGL context > > > > > + * > > > > > + * This function destroys the EGL context created by initEGLContext(). > > > > > > > > Let's clearly say each initContext() invocation has to be balanced by > > > > a resetEGLContext() call > > > > > > Well, that depends on what the destructor does (see below). > > > > > > > I might have missed why > > I meant that if the destructor calls `resetEGLContext()`, then it > does not need to be fully balanced because the descriptor takes care of it. > > > > > > > > > > > > > Which makes me wonder, since the eGL class constructor doesn't call > > > > initEGLContext(), does the destructor need to call resetEGLContext() ? > > > > > > > > I think it doesn't hurt and was there already. > > > > > > > > > + */ > > > > > +void eGL::resetEGLContext() > > > > > +{ > > > > > + if (context_ != EGL_NO_CONTEXT) { > > > > > + if (tid_ == Thread::currentId()) > > > > > > > > All other usages of tid_ are assertions. Why is this different ? Do > > > > you expect other threads to call this function ? > > > > > > This is somewhat related to the previous question. The reason is that it is called > > > from the destructor. If we add `ASSERT(context_ == EGL_NO_CONTEXT)` to the destructor > > > > Would the assertion enforce that all calls to init are balanced ? > > Yes. > > > > > > > instead, then this function can also assert `tid_`. > > > > Oh I see. I would keep it the way it is then, but anyway require every > > init to be balanced by a reset. If that doesn't happen and at > > destruction time context_ != EGL_NO_CONTEXT we could Warn and call > > reset anyway to not leak resources ? > > With the current use case, init/reset calls are balanced and so the assertion would be fine. > And now that you mentioned it, I think I'd prefer that over the other approaches. As egl.h is an internal component, and its users are library components, I think it's fair to require them to properly use the API. I would be a bit hesitant in asserting if a user action could trigger that, but as far as I understand we're in control here, and if an assertion is hit, it should only happen during development ? > > > > > > > > > > > > > > > > > > > + eglMakeCurrent(display_, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); > > > > > + > > > > > + eglDestroyContext(display_, std::exchange(context_, EGL_NO_CONTEXT)); > > > > > + } > > > > > + > > > > > + tid_ = -1; > > > > > + vtable_ = {}; > > > > > > > > I would have only changed the class destructor in this patch and > > > > introduce eGL::resetEGLContext() in the next patch where you modify > > > > the DebayerEGL class to call this function on stop(). > > > > > > > > > +} > > > > > + > > > > > /** > > > > > * \brief Make the EGL context current for the calling thread > > > > > * > > > > > -- > > > > > 2.55.0 > > > > > > > > >
2026. 08. 11. 12:28 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Tue, Aug 11, 2026 at 12:22:01PM +0200, Barnabás Pőcze wrote: >> 2026. 08. 11. 12:01 keltezéssel, Jacopo Mondi írta: >>> Hi Barnabás >>> >>> On Tue, Aug 11, 2026 at 11:41:06AM +0200, Barnabás Pőcze wrote: >>>> 2026. 08. 11. 11:30 keltezéssel, Jacopo Mondi írta: >>>>> On Mon, Aug 10, 2026 at 12:59:39PM +0200, Barnabás Pőcze wrote: >>>>>> Add a function that undoes the effects of `initEGLContext()`. >>>>>> >>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>>> --- >>>>>> include/libcamera/internal/egl.h | 1 + >>>>>> src/libcamera/egl.cpp | 25 ++++++++++++++++++++----- >>>>>> 2 files changed, 21 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h >>>>>> index fb77225ac4..e18bb91f4d 100644 >>>>>> --- a/include/libcamera/internal/egl.h >>>>>> +++ b/include/libcamera/internal/egl.h >>>>>> @@ -103,6 +103,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 3de5fab816..237791bca1 100644 >>>>>> --- a/src/libcamera/egl.cpp >>>>>> +++ b/src/libcamera/egl.cpp >>>>>> @@ -71,14 +71,11 @@ eGL::eGL(EGLDisplay display) >>>>>> } >>>>>> >>>>>> /** >>>>>> - * \brief Destroy the EGL helper >>>>>> - * >>>>>> - * Destroys the EGL context and surface if they were successfully created. >>>>>> + * \brief Release all EGL resources >>>>>> */ >>>>>> eGL::~eGL() >>>>>> { >>>>>> - if (context_ != EGL_NO_CONTEXT) >>>>>> - eglDestroyContext(display_, context_); >>>>>> + resetEGLContext(); >>>>>> } >>>>>> >>>>>> /** >>>>>> @@ -436,6 +433,24 @@ fail: >>>>>> return -ENODEV; >>>>>> } >>>>>> >>>>>> +/** >>>>>> + * \brief Destroy the EGL context >>>>>> + * >>>>>> + * This function destroys the EGL context created by initEGLContext(). >>>>> >>>>> Let's clearly say each initContext() invocation has to be balanced by >>>>> a resetEGLContext() call >>>> >>>> Well, that depends on what the destructor does (see below). >>>> >>> >>> I might have missed why >> >> I meant that if the destructor calls `resetEGLContext()`, then it >> does not need to be fully balanced because the descriptor takes care of it. >> >> >>>> >>>>> >>>>> Which makes me wonder, since the eGL class constructor doesn't call >>>>> initEGLContext(), does the destructor need to call resetEGLContext() ? >>>>> >>>>> I think it doesn't hurt and was there already. >>>>> >>>>>> + */ >>>>>> +void eGL::resetEGLContext() >>>>>> +{ >>>>>> + if (context_ != EGL_NO_CONTEXT) { >>>>>> + if (tid_ == Thread::currentId()) >>>>> >>>>> All other usages of tid_ are assertions. Why is this different ? Do >>>>> you expect other threads to call this function ? >>>> >>>> This is somewhat related to the previous question. The reason is that it is called >>>> from the destructor. If we add `ASSERT(context_ == EGL_NO_CONTEXT)` to the destructor >>> >>> Would the assertion enforce that all calls to init are balanced ? >> >> Yes. >> >> >>> >>>> instead, then this function can also assert `tid_`. >>> >>> Oh I see. I would keep it the way it is then, but anyway require every >>> init to be balanced by a reset. If that doesn't happen and at >>> destruction time context_ != EGL_NO_CONTEXT we could Warn and call >>> reset anyway to not leak resources ? >> >> With the current use case, init/reset calls are balanced and so the assertion would be fine. >> And now that you mentioned it, I think I'd prefer that over the other approaches. > > As egl.h is an internal component, and its users are library > components, I think it's fair to require them to properly use the API. > > I would be a bit hesitant in asserting if a user action could trigger > that, but as far as I understand we're in control here, and if an > assertion is hit, it should only happen during development ? I think user action shouldn't be able to trigger it. Maybe in some edge case that's already running into undefined behaviour, like destroying the `CameraManager` while cameras are running or something like that, but that's already majorly problematic. > >> >> >>> >>>> >>>> >>>>> >>>>>> + eglMakeCurrent(display_, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); >>>>>> + >>>>>> + eglDestroyContext(display_, std::exchange(context_, EGL_NO_CONTEXT)); >>>>>> + } >>>>>> + >>>>>> + tid_ = -1; >>>>>> + vtable_ = {}; >>>>> >>>>> I would have only changed the class destructor in this patch and >>>>> introduce eGL::resetEGLContext() in the next patch where you modify >>>>> the DebayerEGL class to call this function on stop(). >>>>> >>>>>> +} >>>>>> + >>>>>> /** >>>>>> * \brief Make the EGL context current for the calling thread >>>>>> * >>>>>> -- >>>>>> 2.55.0 >>>>>> >>>> >>
diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h index fb77225ac4..e18bb91f4d 100644 --- a/include/libcamera/internal/egl.h +++ b/include/libcamera/internal/egl.h @@ -103,6 +103,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 3de5fab816..237791bca1 100644 --- a/src/libcamera/egl.cpp +++ b/src/libcamera/egl.cpp @@ -71,14 +71,11 @@ eGL::eGL(EGLDisplay display) } /** - * \brief Destroy the EGL helper - * - * Destroys the EGL context and surface if they were successfully created. + * \brief Release all EGL resources */ eGL::~eGL() { - if (context_ != EGL_NO_CONTEXT) - eglDestroyContext(display_, context_); + resetEGLContext(); } /** @@ -436,6 +433,24 @@ fail: return -ENODEV; } +/** + * \brief Destroy the EGL context + * + * This function destroys the EGL context created by initEGLContext(). + */ +void eGL::resetEGLContext() +{ + if (context_ != EGL_NO_CONTEXT) { + if (tid_ == Thread::currentId()) + 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 *
Add a function that undoes the effects of `initEGLContext()`. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/internal/egl.h | 1 + src/libcamera/egl.cpp | 25 ++++++++++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-)