darksylinc / betsy

Betsy GPU compressor
Other
322 stars 18 forks source link

p_numRefinements uniform in bc1 encoder is incorrectly set #9

Closed nanley closed 2 years ago

nanley commented 2 years ago

I believe the uniform is being used with its default value of zero instead of the intended value of two.

The mesa driver on my system seems to suggest that the provided value is being ignored. When I use the bc1 codec with mesa debug output enabled (MESA_DEBUG=true), I get this error:

Mesa: User error: GL_INVALID_OPERATION in glUniform1("constarray_1_5"@0 is float, not uint)

Either of these changes is enough to fix it:

index 4cdccd7..d79e7b8 100644
--- a/src/betsy/EncoderBC1.cpp
+++ b/src/betsy/EncoderBC1.cpp
@@ -119,7 +119,7 @@ namespace betsy
                bindUav( 0u, m_bc1TargetRes, PFG_RG32_UINT, ResourceAccess::Write );
                bindUavBuffer( 1u, m_bc1TablesSsbo, 0u, sizeof( Bc1Tables ) );

-               glUniform1ui( 0, 2u );
+               glUniform1ui( glGetUniformLocation( m_bc1Pso.computePso, "p_numRefinements" ), 2u );

                glDispatchCompute( alignToNextMultiple( m_width, ( 8u * 4u ) ) / ( 8u * 4u ),
                                                   alignToNextMultiple( m_height, ( 8u * 4u ) ) / ( 8u * 4u ), 1u );
index deb17e7..8568800 100644
--- a/bin/Data/bc1.glsl
+++ b/bin/Data/bc1.glsl
@@ -7,7 +7,7 @@

 #define FLT_MAX 340282346638528859811704183484516925440.0f

-uniform uint p_numRefinements;
+layout( location = 0 ) uniform uint p_numRefinements;

 uniform sampler2D srcTex;
nanley commented 2 years ago

With one of these fixes applied, the time to encode BC1 increases, as expected.

nanley commented 2 years ago

@darksylinc Mind if I send out a PR for this?

darksylinc commented 2 years ago

I'm not sure why it happens (explicit layout( location = 0 ) means glGetUniformLocation( m_bc1Pso.computePso, "p_numRefinements" ) should not be necessary) but I guess sure, submit a PR.

Did you try only layout( location = 0 ) ?

nanley commented 2 years ago

Sorry for the confusion, only one of these changes is needed to fix the issue (i.e., doing only layout( location = 0 ) will fix this).