[libcamera-devel] [PATCH v2 3/5] qcam: viewfinder_gl: RAW10P: handle the padding bytes properly

[Laurent Pinchart]

Hi Andrey,

Thank you for the patch.

On Thu, May 27, 2021 at 01:55:09PM +0300, Andrey Konovalov wrote:
> The texture size must account for the padding bytes which may be present
> at the end of the lines in the frame.
> 
> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> ---
>  src/qcam/viewfinder_gl.cpp | 11 +++++++----
>  src/qcam/viewfinder_gl.h   |  1 +
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp
> index b324d77f..9db536a6 100644
> --- a/src/qcam/viewfinder_gl.cpp
> +++ b/src/qcam/viewfinder_gl.cpp
> @@ -111,6 +111,10 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
>  		renderComplete(buffer_);
>  
>  	data_ = static_cast<unsigned char *>(map->memory);
> +	/*
> +	 * \todo Get the stride from the buffer instead of computing it naively
> +	 */
> +	stride_ = buffer->metadata().planes[0].bytesused / size_.height();
>  	update();
>  	buffer_ = buffer;
>  }
> @@ -594,14 +598,13 @@ void ViewFinderGL::doRender()
>  		/*
>  		 * Packed raw Bayer 10-bit formats are stored in GL_RED texture.
>  		 * The texture width is 10/8 of the image width.
> -		 * TODO: account for padding bytes at the end of the line.
>  		 */
>  		glActiveTexture(GL_TEXTURE0);
>  		configureTexture(*textures_[0]);
>  		glTexImage2D(GL_TEXTURE_2D,
>  			     0,
>  			     GL_RED,
> -			     size_.width() * 5 / 4,
> +			     stride_,
>  			     size_.height(),
>  			     0,
>  			     GL_RED,
> @@ -611,12 +614,12 @@ void ViewFinderGL::doRender()
>  		shaderProgram_.setUniformValue(textureUniformBayerFirstRed_,
>  					       firstRed_);
>  		shaderProgram_.setUniformValue(textureUniformSize_,
> -					       size_.width() * 5 / 4,
> +					       stride_,
>  					       size_.height(),
>  					       size_.width(),
>  					       size_.height());
>  		shaderProgram_.setUniformValue(textureUniformStep_,
> -					       1.0f / (size_.width() * 5 / 4 - 1),
> +					       1.0f / (stride_ - 1),
>  					       1.0f / (size_.height() - 1));

This seems like a good change, but shouldn't we also do the same for the
other formats ? In that case, I'd prefer moving this before 2/5, and
using the stride directly in 2/5 instead of width * 5 / 4.

>  		break;
>  
> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h
> index 337718e3..49f8364a 100644
> --- a/src/qcam/viewfinder_gl.h
> +++ b/src/qcam/viewfinder_gl.h
> @@ -66,6 +66,7 @@ private:
>  	libcamera::FrameBuffer *buffer_;
>  	libcamera::PixelFormat format_;
>  	QSize size_;
> +	int stride_;

As the stride can't be negative, I'd make it an unsigned int.

>  	unsigned char *data_;
>  
>  	/* Shaders */

-- 
Regards,

Laurent Pinchart