[libcamera-devel,v2,3/5] qcam: viewfinder_gl: RAW10P: handle the padding bytes properly
diff mbox series

Message ID 20210527105511.447089-4-andrey.konovalov@linaro.org
State Superseded
Headers show
Series
  • qcam: viewfinder_gl: add RAW8, RAW10P and RAW12P formats
Related show

Commit Message

Andrey Konovalov May 27, 2021, 10:55 a.m. UTC 
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@linaro.org>
---
 src/qcam/viewfinder_gl.cpp | 11 +++++++----
 src/qcam/viewfinder_gl.h   |  1 +
 2 files changed, 8 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart June 9, 2021, 2:17 p.m. UTC | #1 
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@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 */
Laurent Pinchart June 9, 2021, 10:31 p.m. UTC | #2 
Hi Andrey,

On Wed, Jun 09, 2021 at 05:17:38PM +0300, Laurent Pinchart wrote:
> 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@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.

Thinking a bit more about this, I think there's an issue. Let's imagine
a case where the padding at the end of the line would be very large,
make the stride twice as large as the actual contents. When rendering
the scene, the GPU will render the full width of the texture. The left
part of the output will then contain actual image data, and the right
part padding data.

To fix this, I think we need to adjust the texture coordinates in the
vertex buffer to go from 0 to width / stride instead of 0 to 1.

> >  		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 */
Andrey Konovalov June 11, 2021, 3:46 p.m. UTC | #3 
Hi Laurent,

Thank you for the review.

On 09.06.2021 17:17, Laurent Pinchart wrote:
> 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@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 ?

The above change is in the part of the code which will be used by the other
formats when they are added. No changes for the other formats are needed that is.

> In that case, I'd prefer moving this before 2/5, and
> using the stride directly in 2/5 instead of width * 5 / 4.

Right. It makes no sense to keep the development history in the patchset.
I'll squash 2/5 and 3/5 into single commit in v3.

>>   		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.

OK, will fix that in v3.

Thanks,
Andrey

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

Patch
diff mbox series 

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));
 		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_;
 	unsigned char *data_;
 
 	/* Shaders */