| Message ID | 20251122205507.37387-3-johannes.goede@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On 22/11/2025 20:55, Hans de Goede wrote: > When bayer_unpacked.vert is calculating the center and x/yCoord values > stride != width is taken into account for x/yCoord deltas since it is taken > into account by debayer_egl when setting the x part of tex_step uniform. > > But it is not taken into account for the center.x which is just directly > copied from textureIn, leading to the input width sampling covering > the entire input stride instead of just covering the input width. > > Use the existing and currently unused stride_factor uniform to pass > the width/stride ratio and correct center.x for this. This fixes > the misrendering seen on x86 laptops which is caused by the CSI2 receiver > there requiring a stride which is a multiple of 32 often leading to > stride != width. > > Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com> > --- > Besides using stride_factor to correct center.x this also requires > debayer_egl to actuall set stride_factor instead of hardcoding it to 1.0, > this last part should be squashed into "libcamera: software_isp: > debayer_egl: Add an eGL debayer class" > --- > include/libcamera/internal/shaders/bayer_unpacked.vert | 4 +++- > src/libcamera/software_isp/debayer_egl.cpp | 2 +- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/internal/shaders/bayer_unpacked.vert b/include/libcamera/internal/shaders/bayer_unpacked.vert > index fb5109ee..1425b449 100644 > --- a/include/libcamera/internal/shaders/bayer_unpacked.vert > +++ b/include/libcamera/internal/shaders/bayer_unpacked.vert > @@ -40,8 +40,10 @@ varying vec4 xCoord; > /** of the adjacent pixels.*/ > varying vec4 yCoord; > > +uniform float stride_factor; > + > void main(void) { > - center.xy = textureIn; > + center.xy = vec2(textureIn.x * stride_factor, textureIn.y); > center.zw = textureIn * tex_size + tex_bayer_first_red; > > xCoord = center.x + vec4(-2.0 * tex_step.x, > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > index cf40478f..29a8ae70 100644 > --- a/src/libcamera/software_isp/debayer_egl.cpp > +++ b/src/libcamera/software_isp/debayer_egl.cpp > @@ -455,7 +455,7 @@ void DebayerEGL::setShaderVariableValues(DebayerParams ¶ms) > (GLfloat)height_ }; > GLfloat Step[] = { static_cast<float>(bytesPerPixel_) / (inputConfig_.stride - 1), > 1.0f / (height_ - 1) }; > - GLfloat Stride = 1.0f; > + GLfloat Stride = (GLfloat)width_ / (inputConfig_.stride / bytesPerPixel_); > GLfloat scaleX = (GLfloat)window_.width / width_; > GLfloat scaleY = (GLfloat)window_.height / height_; > GLfloat transX = -(1.0f - scaleX); Nice. Is it OK to squash this down and do Co-developed-by/Signed-off-by ? --- bod
On 22/11/2025 20:55, Hans de Goede wrote: > When bayer_unpacked.vert is calculating the center and x/yCoord values > stride != width is taken into account for x/yCoord deltas since it is taken > into account by debayer_egl when setting the x part of tex_step uniform. > > But it is not taken into account for the center.x which is just directly > copied from textureIn, leading to the input width sampling covering > the entire input stride instead of just covering the input width. > > Use the existing and currently unused stride_factor uniform to pass > the width/stride ratio and correct center.x for this. This fixes > the misrendering seen on x86 laptops which is caused by the CSI2 receiver > there requiring a stride which is a multiple of 32 often leading to > stride != width. > > Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com> On second thought I think this should be cherry-picked as-is on top of Milan's changes. --- bod
Hi, On 23-Nov-25 11:27 AM, Bryan O'Donoghue wrote: > On 22/11/2025 20:55, Hans de Goede wrote: >> When bayer_unpacked.vert is calculating the center and x/yCoord values >> stride != width is taken into account for x/yCoord deltas since it is taken >> into account by debayer_egl when setting the x part of tex_step uniform. >> >> But it is not taken into account for the center.x which is just directly >> copied from textureIn, leading to the input width sampling covering >> the entire input stride instead of just covering the input width. >> >> Use the existing and currently unused stride_factor uniform to pass >> the width/stride ratio and correct center.x for this. This fixes >> the misrendering seen on x86 laptops which is caused by the CSI2 receiver >> there requiring a stride which is a multiple of 32 often leading to >> stride != width. >> >> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com> > On second thought I think this should be cherry-picked as-is on top of Milan's changes. As I said under the --- line I think the debayer_egl.cpp change can probably be squashed but for the shader changes cherry-pick probably makes sense. But whatever works for you also works for me :) Regards, Hans
Hi Hans, thank you for the patch. Hans de Goede <johannes.goede@oss.qualcomm.com> writes: > When bayer_unpacked.vert is calculating the center and x/yCoord values > stride != width is taken into account for x/yCoord deltas since it is taken > into account by debayer_egl when setting the x part of tex_step uniform. > > But it is not taken into account for the center.x which is just directly > copied from textureIn, leading to the input width sampling covering > the entire input stride instead of just covering the input width. > > Use the existing and currently unused stride_factor uniform to pass Unused in bayer_unpacked.vert; already used in identity.vert (but yes, not set to the proper value). > the width/stride ratio and correct center.x for this. This fixes > the misrendering seen on x86 laptops which is caused by the CSI2 receiver > there requiring a stride which is a multiple of 32 often leading to > stride != width. > > Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com> Your patches seem to work fine in my environment. Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > --- > Besides using stride_factor to correct center.x this also requires > debayer_egl to actuall set stride_factor instead of hardcoding it to 1.0, > this last part should be squashed into "libcamera: software_isp: > debayer_egl: Add an eGL debayer class" > --- > include/libcamera/internal/shaders/bayer_unpacked.vert | 4 +++- > src/libcamera/software_isp/debayer_egl.cpp | 2 +- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/internal/shaders/bayer_unpacked.vert b/include/libcamera/internal/shaders/bayer_unpacked.vert > index fb5109ee..1425b449 100644 > --- a/include/libcamera/internal/shaders/bayer_unpacked.vert > +++ b/include/libcamera/internal/shaders/bayer_unpacked.vert > @@ -40,8 +40,10 @@ varying vec4 xCoord; > /** of the adjacent pixels.*/ > varying vec4 yCoord; > > +uniform float stride_factor; > + > void main(void) { > - center.xy = textureIn; > + center.xy = vec2(textureIn.x * stride_factor, textureIn.y); > center.zw = textureIn * tex_size + tex_bayer_first_red; > > xCoord = center.x + vec4(-2.0 * tex_step.x, > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > index cf40478f..29a8ae70 100644 > --- a/src/libcamera/software_isp/debayer_egl.cpp > +++ b/src/libcamera/software_isp/debayer_egl.cpp > @@ -455,7 +455,7 @@ void DebayerEGL::setShaderVariableValues(DebayerParams ¶ms) > (GLfloat)height_ }; > GLfloat Step[] = { static_cast<float>(bytesPerPixel_) / (inputConfig_.stride - 1), > 1.0f / (height_ - 1) }; > - GLfloat Stride = 1.0f; > + GLfloat Stride = (GLfloat)width_ / (inputConfig_.stride / bytesPerPixel_); > GLfloat scaleX = (GLfloat)window_.width / width_; > GLfloat scaleY = (GLfloat)window_.height / height_; > GLfloat transX = -(1.0f - scaleX);
On 22/11/2025 20:55, Hans de Goede wrote: > - GLfloat Stride = 1.0f; > + GLfloat Stride = (GLfloat)width_ / (inputConfig_.stride / bytesPerPixel_); This ends up cropping the image on the 1x shader for me since we use identity.vert not bayer_unpacked.vert so, I'll differentiate this logic between packed and unpacked in v5. --- bod
Hi, On 26-Nov-25 6:49 PM, Bryan O'Donoghue wrote: > On 22/11/2025 20:55, Hans de Goede wrote: >> - GLfloat Stride = 1.0f; >> + GLfloat Stride = (GLfloat)width_ / (inputConfig_.stride / bytesPerPixel_); > > This ends up cropping the image on the 1x shader for me since we use identity.vert not bayer_unpacked.vert so, I'll differentiate this logic between packed and unpacked in v5. Hmm, I see. Then maybe copy identity.vert which is shared with qcam shaders for YUV, RGB, etc. to bayer_1x_packed.frag and add: diff --git a/include/libcamera/internal/shaders/identity.vert b/include/libcamera/internal/sha index 907e8741..649f1a6c 100644 --- a/include/libcamera/internal/shaders/identity.vert +++ b/include/libcamera/internal/shaders/identity.vert @@ -10,10 +10,9 @@ attribute vec2 textureIn; varying vec2 textureOut; uniform mat4 proj_matrix; -uniform float stride_factor; void main(void) { gl_Position = proj_matrix * vertexIn; - textureOut = vec2(textureIn.x * stride_factor, textureIn.y); + textureOut = textureIn; } ? There really is no need to waste GPU cycles / energy on multiplying with 1.0 . Regards, Hans
diff --git a/include/libcamera/internal/shaders/bayer_unpacked.vert b/include/libcamera/internal/shaders/bayer_unpacked.vert index fb5109ee..1425b449 100644 --- a/include/libcamera/internal/shaders/bayer_unpacked.vert +++ b/include/libcamera/internal/shaders/bayer_unpacked.vert @@ -40,8 +40,10 @@ varying vec4 xCoord; /** of the adjacent pixels.*/ varying vec4 yCoord; +uniform float stride_factor; + void main(void) { - center.xy = textureIn; + center.xy = vec2(textureIn.x * stride_factor, textureIn.y); center.zw = textureIn * tex_size + tex_bayer_first_red; xCoord = center.x + vec4(-2.0 * tex_step.x, diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp index cf40478f..29a8ae70 100644 --- a/src/libcamera/software_isp/debayer_egl.cpp +++ b/src/libcamera/software_isp/debayer_egl.cpp @@ -455,7 +455,7 @@ void DebayerEGL::setShaderVariableValues(DebayerParams ¶ms) (GLfloat)height_ }; GLfloat Step[] = { static_cast<float>(bytesPerPixel_) / (inputConfig_.stride - 1), 1.0f / (height_ - 1) }; - GLfloat Stride = 1.0f; + GLfloat Stride = (GLfloat)width_ / (inputConfig_.stride / bytesPerPixel_); GLfloat scaleX = (GLfloat)window_.width / width_; GLfloat scaleY = (GLfloat)window_.height / height_; GLfloat transX = -(1.0f - scaleX);
When bayer_unpacked.vert is calculating the center and x/yCoord values stride != width is taken into account for x/yCoord deltas since it is taken into account by debayer_egl when setting the x part of tex_step uniform. But it is not taken into account for the center.x which is just directly copied from textureIn, leading to the input width sampling covering the entire input stride instead of just covering the input width. Use the existing and currently unused stride_factor uniform to pass the width/stride ratio and correct center.x for this. This fixes the misrendering seen on x86 laptops which is caused by the CSI2 receiver there requiring a stride which is a multiple of 32 often leading to stride != width. Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com> --- Besides using stride_factor to correct center.x this also requires debayer_egl to actuall set stride_factor instead of hardcoding it to 1.0, this last part should be squashed into "libcamera: software_isp: debayer_egl: Add an eGL debayer class" --- include/libcamera/internal/shaders/bayer_unpacked.vert | 4 +++- src/libcamera/software_isp/debayer_egl.cpp | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-)