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

[Andrey Konovalov]

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

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 */
>