[v1,1/3] libcamera: egl: Remove `eGLImage::image_`

26516 diff mbox series

From: Gianfranco Mariotti <gianfranco.mariotti94@gmail.com>

This member stores an egl image handle, but currently there is no need for it
since the image handle is only really used in `createDMABufTexture2D()`.
(`destroyDMABufTexture()` is an unused function.)

So remove the member (and the unused function), and instead destroy the image
immediately after calling `glEGLImageTargetTexture2DOES()`. The texture will
keep a reference to the image, so this is safe to do. In fact, this solves
an issue, specifically, the egl images were never destroyed, and continuously
leaked during streaming.

Fixes: f520b29fe9e6 ("libcamera: software_isp: debayer_egl: Add an eGL Debayer class")
Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/322
Signed-off-by: Gianfranco Mariotti <gianfranco.mariotti94@gmail.com>
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---

Supersedes https://patchwork.libcamera.org/patch/26319/

---
 include/libcamera/internal/egl.h |  2 --
 src/libcamera/egl.cpp            | 37 +++++++++-----------------------
 2 files changed, 10 insertions(+), 29 deletions(-)

--
2.53.0

Quoting Barnabás Pőcze (2026-04-13 12:47:11)
> From: Gianfranco Mariotti <gianfranco.mariotti94@gmail.com>
> 
> This member stores an egl image handle, but currently there is no need for it
> since the image handle is only really used in `createDMABufTexture2D()`.
> (`destroyDMABufTexture()` is an unused function.)
> 
> So remove the member (and the unused function), and instead destroy the image
> immediately after calling `glEGLImageTargetTexture2DOES()`. The texture will
> keep a reference to the image, so this is safe to do. In fact, this solves
> an issue, specifically, the egl images were never destroyed, and continuously
> leaked during streaming.
> 
> Fixes: f520b29fe9e6 ("libcamera: software_isp: debayer_egl: Add an eGL Debayer class")
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/322
> Signed-off-by: Gianfranco Mariotti <gianfranco.mariotti94@gmail.com>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


I think I saw someone tested this, or was that Gianfranco ?

Anyway, if it's fixing things then great!

Nothing I object to so:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
> 
> Supersedes https://patchwork.libcamera.org/patch/26319/
> 
> ---
>  include/libcamera/internal/egl.h |  2 --
>  src/libcamera/egl.cpp            | 37 +++++++++-----------------------
>  2 files changed, 10 insertions(+), 29 deletions(-)
> 
> diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h
> index 8a2d96d7a..0ad2320b1 100644
> --- a/include/libcamera/internal/egl.h
> +++ b/include/libcamera/internal/egl.h
> @@ -88,7 +88,6 @@ public:
>         GLenum texture_unit_; /**< Texture unit associated with this image eg (GL_TEXTURE0) */
>         GLuint texture_; /**< OpenGL texture object ID */
>         GLuint fbo_; /**< OpenGL frame buffer object ID */
> -       EGLImageKHR image_; /**< EGL Image handle */
> 
>  private:
>         LIBCAMERA_DISABLE_COPY_AND_MOVE(eGLImage)
> @@ -104,7 +103,6 @@ public:
> 
>         int createInputDMABufTexture2D(eGLImage &eglImage, int fd);
>         int createOutputDMABufTexture2D(eGLImage &eglImage, int fd);
> -       void destroyDMABufTexture(eGLImage &eglImage);
>         void createTexture2D(eGLImage &eglImage, GLint format, uint32_t width, uint32_t height, void *data);
> 
>         void pushEnv(std::vector<std::string> &shaderEnv, const char *str);
> diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp
> index 5b9bbf410..f65929470 100644
> --- a/src/libcamera/egl.cpp
> +++ b/src/libcamera/egl.cpp
> @@ -112,8 +112,6 @@ void eGL::syncOutput()
>   */
>  int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, bool output)
>  {
> -       int ret = 0;
> -
>         ASSERT(tid_ == Thread::currentId());
> 
>         // clang-format off
> @@ -130,14 +128,13 @@ int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, bool output)
>         };
>         // clang-format on
> 
> -       eglImage.image_ = eglCreateImageKHR(display_, EGL_NO_CONTEXT,
> -                                           EGL_LINUX_DMA_BUF_EXT,
> -                                           NULL, image_attrs);
> +       EGLImageKHR image = eglCreateImageKHR(display_, EGL_NO_CONTEXT,
> +                                             EGL_LINUX_DMA_BUF_EXT,
> +                                             NULL, image_attrs);
> 
> -       if (eglImage.image_ == EGL_NO_IMAGE_KHR) {
> +       if (image == EGL_NO_IMAGE_KHR) {
>                 LOG(eGL, Error) << "eglCreateImageKHR fail";
> -               ret = -ENODEV;
> -               goto done;
> +               return -ENODEV;
>         }
> 
>         // Bind texture unit and texture
> @@ -145,7 +142,8 @@ int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, bool output)
>         glBindTexture(GL_TEXTURE_2D, eglImage.texture_);
> 
>         // Generate texture with filter semantics
> -       glEGLImageTargetTexture2DOES(GL_TEXTURE_2D, eglImage.image_);
> +       glEGLImageTargetTexture2DOES(GL_TEXTURE_2D, image);
> +       eglDestroyImageKHR(display_, image);
> 
>         // Nearest filtering
>         glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
> @@ -163,13 +161,11 @@ int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, bool output)
>                 GLenum err = glCheckFramebufferStatus(GL_FRAMEBUFFER);
>                 if (err != GL_FRAMEBUFFER_COMPLETE) {
>                         LOG(eGL, Error) << "glFrameBufferTexture2D error " << err;
> -                       eglDestroyImageKHR(display_, eglImage.image_);
> -                       ret = -ENODEV;
> -                       goto done;
> +                       return -ENODEV;
>                 }
>         }
> -done:
> -       return ret;
> +
> +       return 0;
>  }
> 
>  /**
> @@ -209,19 +205,6 @@ int eGL::createOutputDMABufTexture2D(eGLImage &eglImage, int fd)
>         return createDMABufTexture2D(eglImage, fd, true);
>  }
> 
> -/**
> - * \brief Destroy a DMA-BUF texture's EGL image
> - * \param[in,out] eglImage EGL image to destroy
> - *
> - * Destroys the EGL image associated with a DMA-BUF texture. The OpenGL
> - * texture and framebuffer objects are destroyed separately in the
> - * eGLImage destructor.
> - */
> -void eGL::destroyDMABufTexture(eGLImage &eglImage)
> -{
> -       eglDestroyImage(display_, std::exchange(eglImage.image_, EGL_NO_IMAGE_KHR));
> -}
> -
>  /**
>   * \brief Create a 2D texture from a memory buffer
>   * \param[in,out] eglImage EGL image to associate with the texture
> --
> 2.53.0