[v3,5/8] libcamera: egl: Add updateTexture2D
diff mbox series

Message ID 20260626113325.3218045-6-bryan.odonoghue@linaro.org
State New
Headers show
Series
  • libcamera: software_isp: gpu: Add go faster stripes
Related show

Commit Message

Bryan O'Donoghue June 26, 2026, 11:33 a.m. UTC 
The internet box tells me that glTextSubImage2D lets us update a texture's
data only, instead of recreating the texture and uploading data.

This is a smallish optimisation but we are hunting for every possible cycle
and watt so add the routine as precursor to using it in-place of
createTexture2D on every upload cycle.

Reviewed-by: Robert Mader <robert.mader@collabora.com>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 include/libcamera/internal/egl.h |  1 +
 src/libcamera/egl.cpp            | 35 ++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

Comments

Robert Mader June 26, 2026, 12:09 p.m. UTC | #1 
On 26.06.26 13:33, Bryan O'Donoghue wrote:
> The internet box tells me that glTextSubImage2D lets us update a texture's
> data only, instead of recreating the texture and uploading data.
>
> This is a smallish optimisation but we are hunting for every possible cycle
> and watt so add the routine as precursor to using it in-place of
> createTexture2D on every upload cycle.
>
> Reviewed-by: Robert Mader <robert.mader@collabora.com>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   include/libcamera/internal/egl.h |  1 +
>   src/libcamera/egl.cpp            | 35 ++++++++++++++++++++++++++++++++
>   2 files changed, 36 insertions(+)
>
> diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h
> index e24a726dc..9b679332c 100644
> --- a/include/libcamera/internal/egl.h
> +++ b/include/libcamera/internal/egl.h
> @@ -108,6 +108,7 @@ public:
>   	int createInputDMABufTexture2D(eGLImage &eglImage, int fd);
>   	int createOutputDMABufTexture2D(eGLImage &eglImage, int fd);
>   	void createTexture2D(eGLImage &eglImage, void *data);
> +	void updateTexture2D(eGLImage &eglImage, void *data);
>   	void createOutputTexture2D(eGLImage &eglImage);
>   
>   	int attachTextureToFBO(eGLImage &eglImage);
> diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp
> index 8e8d61813..22f46e92a 100644
> --- a/src/libcamera/egl.cpp
> +++ b/src/libcamera/egl.cpp
> @@ -329,6 +329,41 @@ bool eGL::isAvailable()
>   	return true;
>   }
>   
> +/**
> + * \brief Update a 2D texture already created
> + * \param[in,out] eglImage EGL image to associate with the texture
> + * \param[data] Data to update the texture with
> + *
> + * Updates a 2D texture in VRAM.
> + */
> +void eGL::updateTexture2D(eGLImage &eglImage, void *data)
> +{
> +	GLenum format;
> +	GLenum type = GL_UNSIGNED_BYTE;
> +
> +	ASSERT(tid_ == Thread::currentId());
> +
> +	glActiveTexture(eglImage.texture_unit_);
> +	glBindTexture(GL_TEXTURE_2D, eglImage.texture_);
As mentioned before I think it would be better to move this commit after 
the next so you can use "activateBindTexture()" here.
> +
> +	switch (eglImage.format_) {
> +	case GL_R16F:
> +		format = GL_RED;
> +		type = GL_HALF_FLOAT;
> +		break;
> +	case GL_RG8:
> +		format = GL_RG;
> +		break;
> +	case GL_LUMINANCE:
> +	default:
> +		format = GL_LUMINANCE;
> +		break;
> +	}
This should be removed now as well.
> +
> +	// Update an already exsiting texture
> +	glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, eglImage.width_, eglImage.height_, format, type, data);
> +}
> +
>   /**
>    * \brief Create a 2D texture attached to an FBO for render-to-texture
>    * \param[in,out] eglImage EGL image to associate with the texture
Just some suggestions for cleanups - my R-B still holds.
Bryan O'Donoghue June 26, 2026, 12:13 p.m. UTC | #2 
On 26/06/2026 13:09, Robert Mader wrote:
> On 26.06.26 13:33, Bryan O'Donoghue wrote:
>> The internet box tells me that glTextSubImage2D lets us update a 
>> texture's
>> data only, instead of recreating the texture and uploading data.
>>
>> This is a smallish optimisation but we are hunting for every possible 
>> cycle
>> and watt so add the routine as precursor to using it in-place of
>> createTexture2D on every upload cycle.
>>
>> Reviewed-by: Robert Mader <robert.mader@collabora.com>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   include/libcamera/internal/egl.h |  1 +
>>   src/libcamera/egl.cpp            | 35 ++++++++++++++++++++++++++++++++
>>   2 files changed, 36 insertions(+)
>>
>> diff --git a/include/libcamera/internal/egl.h b/include/libcamera/ 
>> internal/egl.h
>> index e24a726dc..9b679332c 100644
>> --- a/include/libcamera/internal/egl.h
>> +++ b/include/libcamera/internal/egl.h
>> @@ -108,6 +108,7 @@ public:
>>       int createInputDMABufTexture2D(eGLImage &eglImage, int fd);
>>       int createOutputDMABufTexture2D(eGLImage &eglImage, int fd);
>>       void createTexture2D(eGLImage &eglImage, void *data);
>> +    void updateTexture2D(eGLImage &eglImage, void *data);
>>       void createOutputTexture2D(eGLImage &eglImage);
>>       int attachTextureToFBO(eGLImage &eglImage);
>> diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp
>> index 8e8d61813..22f46e92a 100644
>> --- a/src/libcamera/egl.cpp
>> +++ b/src/libcamera/egl.cpp
>> @@ -329,6 +329,41 @@ bool eGL::isAvailable()
>>       return true;
>>   }
>> +/**
>> + * \brief Update a 2D texture already created
>> + * \param[in,out] eglImage EGL image to associate with the texture
>> + * \param[data] Data to update the texture with
>> + *
>> + * Updates a 2D texture in VRAM.
>> + */
>> +void eGL::updateTexture2D(eGLImage &eglImage, void *data)
>> +{
>> +    GLenum format;
>> +    GLenum type = GL_UNSIGNED_BYTE;
>> +
>> +    ASSERT(tid_ == Thread::currentId());
>> +
>> +    glActiveTexture(eglImage.texture_unit_);
>> +    glBindTexture(GL_TEXTURE_2D, eglImage.texture_);
> As mentioned before I think it would be better to move this commit after 
> the next so you can use "activateBindTexture()" here.

Ah, now I get what you said.

My brain runs hiberno-english and said `no comprendo` at the last 
iteration..


>> +
>> +    switch (eglImage.format_) {
>> +    case GL_R16F:
>> +        format = GL_RED;
>> +        type = GL_HALF_FLOAT;
>> +        break;
>> +    case GL_RG8:
>> +        format = GL_RG;
>> +        break;
>> +    case GL_LUMINANCE:
>> +    default:
>> +        format = GL_LUMINANCE;
>> +        break;
>> +    }
> This should be removed now as well.

eh.  yes

>> +
>> +    // Update an already exsiting texture
>> +    glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, eglImage.width_, 
>> eglImage.height_, format, type, data);
>> +}
>> +
>>   /**
>>    * \brief Create a 2D texture attached to an FBO for render-to-texture
>>    * \param[in,out] eglImage EGL image to associate with the texture
> Just some suggestions for cleanups - my R-B still holds.
Milan Zamazal June 26, 2026, 12:26 p.m. UTC | #3 
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes:

> The internet box tells me that glTextSubImage2D lets us update a texture's
> data only, instead of recreating the texture and uploading data.
>
> This is a smallish optimisation but we are hunting for every possible cycle
> and watt so add the routine as precursor to using it in-place of
> createTexture2D on every upload cycle.
>
> Reviewed-by: Robert Mader <robert.mader@collabora.com>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  include/libcamera/internal/egl.h |  1 +
>  src/libcamera/egl.cpp            | 35 ++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h
> index e24a726dc..9b679332c 100644
> --- a/include/libcamera/internal/egl.h
> +++ b/include/libcamera/internal/egl.h
> @@ -108,6 +108,7 @@ public:
>  	int createInputDMABufTexture2D(eGLImage &eglImage, int fd);
>  	int createOutputDMABufTexture2D(eGLImage &eglImage, int fd);
>  	void createTexture2D(eGLImage &eglImage, void *data);
> +	void updateTexture2D(eGLImage &eglImage, void *data);
>  	void createOutputTexture2D(eGLImage &eglImage);
>  
>  	int attachTextureToFBO(eGLImage &eglImage);
> diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp
> index 8e8d61813..22f46e92a 100644
> --- a/src/libcamera/egl.cpp
> +++ b/src/libcamera/egl.cpp
> @@ -329,6 +329,41 @@ bool eGL::isAvailable()
>  	return true;
>  }
>  
> +/**
> + * \brief Update a 2D texture already created
> + * \param[in,out] eglImage EGL image to associate with the texture
> + * \param[data] Data to update the texture with
> + *
> + * Updates a 2D texture in VRAM.
> + */
> +void eGL::updateTexture2D(eGLImage &eglImage, void *data)
> +{
> +	GLenum format;

Uninitialised.

> +	GLenum type = GL_UNSIGNED_BYTE;
> +
> +	ASSERT(tid_ == Thread::currentId());
> +
> +	glActiveTexture(eglImage.texture_unit_);
> +	glBindTexture(GL_TEXTURE_2D, eglImage.texture_);
> +
> +	switch (eglImage.format_) {
> +	case GL_R16F:
> +		format = GL_RED;
> +		type = GL_HALF_FLOAT;
> +		break;
> +	case GL_RG8:
> +		format = GL_RG;
> +		break;
> +	case GL_LUMINANCE:
> +	default:
> +		format = GL_LUMINANCE;
> +		break;
> +	}
> +
> +	// Update an already exsiting texture

C-style

s/exsiting/existing/

> +	glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, eglImage.width_, eglImage.height_, format, type, data);
> +}
> +
>  /**
>   * \brief Create a 2D texture attached to an FBO for render-to-texture
>   * \param[in,out] eglImage EGL image to associate with the texture
Milan Zamazal June 26, 2026, 1:59 p.m. UTC | #4 
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes:

> The internet box tells me that glTextSubImage2D lets us update a texture's
> data only, instead of recreating the texture and uploading data.
>
> This is a smallish optimisation but we are hunting for every possible cycle
> and watt so add the routine as precursor to using it in-place of
> createTexture2D on every upload cycle.
>
> Reviewed-by: Robert Mader <robert.mader@collabora.com>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  include/libcamera/internal/egl.h |  1 +
>  src/libcamera/egl.cpp            | 35 ++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h
> index e24a726dc..9b679332c 100644
> --- a/include/libcamera/internal/egl.h
> +++ b/include/libcamera/internal/egl.h
> @@ -108,6 +108,7 @@ public:
>  	int createInputDMABufTexture2D(eGLImage &eglImage, int fd);
>  	int createOutputDMABufTexture2D(eGLImage &eglImage, int fd);
>  	void createTexture2D(eGLImage &eglImage, void *data);
> +	void updateTexture2D(eGLImage &eglImage, void *data);
>  	void createOutputTexture2D(eGLImage &eglImage);
>  
>  	int attachTextureToFBO(eGLImage &eglImage);
> diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp
> index 8e8d61813..22f46e92a 100644
> --- a/src/libcamera/egl.cpp
> +++ b/src/libcamera/egl.cpp
> @@ -329,6 +329,41 @@ bool eGL::isAvailable()
>  	return true;
>  }
>  
> +/**
> + * \brief Update a 2D texture already created
> + * \param[in,out] eglImage EGL image to associate with the texture
> + * \param[data] Data to update the texture with
> + *
> + * Updates a 2D texture in VRAM.
> + */
> +void eGL::updateTexture2D(eGLImage &eglImage, void *data)
> +{
> +	GLenum format;
> +	GLenum type = GL_UNSIGNED_BYTE;
> +
> +	ASSERT(tid_ == Thread::currentId());
> +
> +	glActiveTexture(eglImage.texture_unit_);
> +	glBindTexture(GL_TEXTURE_2D, eglImage.texture_);
> +
> +	switch (eglImage.format_) {
> +	case GL_R16F:
> +		format = GL_RED;
> +		type = GL_HALF_FLOAT;
> +		break;
> +	case GL_RG8:
> +		format = GL_RG;
> +		break;
> +	case GL_LUMINANCE:
> +	default:
> +		format = GL_LUMINANCE;
> +		break;
> +	}
> +
> +	// Update an already exsiting texture
> +	glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, eglImage.width_, eglImage.height_, format, type, data);

This fails in my environment with GL_INVALID_OPERATION.  Which means:

  GL_INVALID_OPERATION is generated by glTextureSubImage2D if texture is
  not the name of an existing texture object.

Which is misleading because the real cause seems to be that
eglImage.format_ is GL_RG, which is not represented in the switch.  When
I use it instead GL_RG8, it works.

> +}
> +
>  /**
>   * \brief Create a 2D texture attached to an FBO for render-to-texture
>   * \param[in,out] eglImage EGL image to associate with the texture

Patch
diff mbox series 

diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h
index e24a726dc..9b679332c 100644
--- a/include/libcamera/internal/egl.h
+++ b/include/libcamera/internal/egl.h
@@ -108,6 +108,7 @@  public:
 	int createInputDMABufTexture2D(eGLImage &eglImage, int fd);
 	int createOutputDMABufTexture2D(eGLImage &eglImage, int fd);
 	void createTexture2D(eGLImage &eglImage, void *data);
+	void updateTexture2D(eGLImage &eglImage, void *data);
 	void createOutputTexture2D(eGLImage &eglImage);
 
 	int attachTextureToFBO(eGLImage &eglImage);
diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp
index 8e8d61813..22f46e92a 100644
--- a/src/libcamera/egl.cpp
+++ b/src/libcamera/egl.cpp
@@ -329,6 +329,41 @@  bool eGL::isAvailable()
 	return true;
 }
 
+/**
+ * \brief Update a 2D texture already created
+ * \param[in,out] eglImage EGL image to associate with the texture
+ * \param[data] Data to update the texture with
+ *
+ * Updates a 2D texture in VRAM.
+ */
+void eGL::updateTexture2D(eGLImage &eglImage, void *data)
+{
+	GLenum format;
+	GLenum type = GL_UNSIGNED_BYTE;
+
+	ASSERT(tid_ == Thread::currentId());
+
+	glActiveTexture(eglImage.texture_unit_);
+	glBindTexture(GL_TEXTURE_2D, eglImage.texture_);
+
+	switch (eglImage.format_) {
+	case GL_R16F:
+		format = GL_RED;
+		type = GL_HALF_FLOAT;
+		break;
+	case GL_RG8:
+		format = GL_RG;
+		break;
+	case GL_LUMINANCE:
+	default:
+		format = GL_LUMINANCE;
+		break;
+	}
+
+	// Update an already exsiting texture
+	glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, eglImage.width_, eglImage.height_, format, type, data);
+}
+
 /**
  * \brief Create a 2D texture attached to an FBO for render-to-texture
  * \param[in,out] eglImage EGL image to associate with the texture