Skip to content

shaders: report line numbers error accurately #297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

shaders: report line numbers error accurately #297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ Versioning](https://semver.org/spec/v2.0.0.html) for `libnopegl`.
- `GraphicConfig.depth_write_mask` is renamed to `GraphicConfig.depth_write`
- `Circle.count`, `Buffer*.count`, `StreamedBuffer*.count` are now unsigned
- `Program.nb_frag_output` is now unsigned
- Line numbers in shader compilation errors are now reported accurately
according to the user input, so shaders are not printed on stderr anymore

### Removed
- `Text.aspect_ratio`, it now matches the viewport aspect ratio
Expand Down
26 changes: 2 additions & 24 deletions libnopegl/src/ngpu/opengl/program_gl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@
#include "log.h"
#include "ngpu/type.h"
#include "program_gl.h"
#include "utils/bstr.h"
#include "utils/memory.h"
#include "utils/string.h"

static int program_check_status(const struct glcontext *gl, GLuint id, GLenum status)
{
Expand Down Expand Up @@ -120,12 +118,7 @@ int ngpu_program_gl_init(struct ngpu_program *s, const struct ngpu_program_param
gl->funcs.CompileShader(shader);
ret = program_check_status(gl, shader, GL_COMPILE_STATUS);
if (ret < 0) {
char *s_with_numbers = ngli_numbered_lines(shaders[i].src);
if (s_with_numbers) {
LOG(ERROR, "failed to compile shader \"%s\":\n%s",
params->label ? params->label : "", s_with_numbers);
ngli_free(s_with_numbers);
}
LOG(ERROR, "failed to compile shader \"%s\"", params->label ? params->label : "");
Copy link
Contributor

@mbouron mbouron Jun 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested a bit the PR, and for now, I think it makes debugging harder. While it works for simple cases, it does not scale well, especially since we are injecting the ressources / inputs / outputs / helpers, ie: there's no other way to visualize the whole assembled shader, which is of great help for debugging. This is even more true for builtin shaders assembled from different glsl helpers and/or when nopegl is distributed across multiple devices with different kind of gpus (for example, one shader can fail on a specific hw / driver combinaison and works nicely on the other devices). Just having the error reported without the full context would make debugging a nightmare.

Copy link
Contributor

@mbouron mbouron Jun 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another issue I see, is that it would not work with builtins such as ngl_texvideo() is they use more than one line. This is currently the case so not an issue. But I had the idea at some point to use multiple line to make the final shader more readable.

Copy link
Contributor Author

@ubitux ubitux Jun 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a debug option we could add a flag to keep dumping the whole thing for the project developers.

For users though the current experience as-is is much better experience. They don't have access to includes so and the line reset appears right before their input, which means the lines are aligned with their input, always (unless they have their own include system, in which case they can also use line directives in their own preprocessor).

Copy link
Contributor Author

@ubitux ubitux Jun 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another issue I see, is that it would not work with builtins such as ngl_texvideo() is they use more than one line. This is currently the case so not an issue. But I had the idea at some point to use multiple line to make the final shader more readable.

We could still trick it with something like that:

#line 34
tex = texture(
   foo,
   bar
);
#line 35

And if we use a function declared before the user input it's a non-issue.

goto fail;
}
gl->funcs.AttachShader(s_priv->id, shader);
Expand All @@ -134,22 +127,7 @@ int ngpu_program_gl_init(struct ngpu_program *s, const struct ngpu_program_param
gl->funcs.LinkProgram(s_priv->id);
ret = program_check_status(gl, s_priv->id, GL_LINK_STATUS);
if (ret < 0) {
struct bstr *bstr = ngli_bstr_create();
if (bstr) {
ngli_bstr_printf(bstr, "failed to link shaders \"%s\":",
params->label ? params->label : "");
for (size_t i = 0; i < NGLI_ARRAY_NB(shaders); i++) {
if (!shaders[i].src)
continue;
char *s_with_numbers = ngli_numbered_lines(shaders[i].src);
if (s_with_numbers) {
ngli_bstr_printf(bstr, "\n\n%s shader:\n%s", shaders[i].name, s_with_numbers);
ngli_free(s_with_numbers);
}
}
LOG(ERROR, "%s", ngli_bstr_strptr(bstr));
ngli_bstr_freep(&bstr);
}
LOG(ERROR, "failed to link shaders \"%s\"", params->label ? params->label : "");
goto fail;
}

Expand Down
3 changes: 3 additions & 0 deletions libnopegl/src/ngpu/pgcraft.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1076,6 +1076,7 @@ static int craft_vert(struct ngpu_pgcraft *s, const struct ngpu_pgcraft_params *
(ret = inject_ublock(s, b, NGPU_PROGRAM_STAGE_VERT)) < 0)
return ret;

ngli_bstr_print(b, "#line 1\n");
ngli_bstr_print(b, params->vert_base);
return samplers_preproc(s, params, b);
}
Expand Down Expand Up @@ -1128,6 +1129,7 @@ static int craft_frag(struct ngpu_pgcraft *s, const struct ngpu_pgcraft_params *

ngli_bstr_print(b, "\n");

ngli_bstr_print(b, "#line 1\n");
ngli_bstr_print(b, params->frag_base);
return samplers_preproc(s, params, b);
}
Expand All @@ -1148,6 +1150,7 @@ static int craft_comp(struct ngpu_pgcraft *s, const struct ngpu_pgcraft_params *
(ret = inject_ublock(s, b, NGPU_PROGRAM_STAGE_COMP)) < 0)
return ret;

ngli_bstr_print(b, "#line 1\n");
ngli_bstr_print(b, params->comp_base);
return samplers_preproc(s, params, b);
}
Expand Down
15 changes: 2 additions & 13 deletions libnopegl/src/ngpu/vulkan/program_vk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
#include "log.h"
#include "program_vk.h"
#include "utils/memory.h"
#include "utils/string.h"
#include "utils/utils.h"
#include "vkutils.h"

Expand Down Expand Up @@ -65,12 +64,7 @@ int ngpu_program_vk_init(struct ngpu_program *s, const struct ngpu_program_param
size_t size = 0;
int ret = ngli_glslang_compile(shaders[i].stage, shaders[i].src, s->gpu_ctx->config.debug, &data, &size);
if (ret < 0) {
char *s_with_numbers = ngli_numbered_lines(shaders[i].src);
if (s_with_numbers) {
LOG(ERROR, "failed to compile shader \"%s\":\n%s",
params->label ? params->label : "", s_with_numbers);
ngli_free(s_with_numbers);
}
LOG(ERROR, "failed to compile shader \"%s\"", params->label ? params->label : "");
return ret;
}

Expand All @@ -82,12 +76,7 @@ int ngpu_program_vk_init(struct ngpu_program *s, const struct ngpu_program_param
VkResult res = vkCreateShaderModule(vk->device, &shader_module_create_info, NULL, &s_priv->shaders[i]);
ngli_freep(&data);
if (res != VK_SUCCESS) {
char *s_with_numbers = ngli_numbered_lines(shaders[i].src);
if (s_with_numbers) {
LOG(ERROR, "failed to compile shader \"%s\":\n%s",
params->label ? params->label : "", s_with_numbers);
ngli_free(s_with_numbers);
}
LOG(ERROR, "failed to compile shader \"%s\"", params->label ? params->label : "");
return ngli_vk_res2ret(res);
}
}
Expand Down
18 changes: 0 additions & 18 deletions libnopegl/src/test_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,8 @@
*/

#include "utils/crc32.h"
#include "utils/memory.h"
#include "utils/string.h"
#include "utils/utils.h"

static void test_numbered_line(uint32_t crc, const char *s)
{
char *p = ngli_numbered_lines(s);
ngli_assert(p);
ngli_assert(ngli_crc32(p) == crc);
ngli_freep(&p);
}

int main(void)
{
ngli_assert(ngli_crc32("") == 0);
Expand All @@ -42,13 +32,5 @@ int main(void)
buf[i] = (char)(0xff - i);
ngli_assert(ngli_crc32(buf) == 0x5473AA4D);

#define X "x\n"
#define S "foo\nbar\nhello\nworld\nbla\nxxx\nyyy\n"
test_numbered_line(0x2d7f40af, S S S S S S S S);
test_numbered_line(0xbcea3585, "\n");
test_numbered_line(0xc58462d3, "foo\nbar");
test_numbered_line(0x00000000, "");
test_numbered_line(0x25b15360, X X X X X X X X X);
test_numbered_line(0x759455a5, X X X X X X X X X X);
return 0;
}
50 changes: 0 additions & 50 deletions libnopegl/src/utils/string.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
#include <stdio.h>
#include <string.h>

#include "bstr.h"
#include "memory.h"
#include "string.h"

Expand Down Expand Up @@ -64,52 +63,3 @@ char *ngli_asprintf(const char *fmt, ...)

return p;
}
static int count_lines(const char *s)
{
int count = 0;
while (*s) {
const size_t len = strcspn(s, "\n");
count++;
if (!s[len])
break;
s += len + 1;
}
return count;
}

static int count_digits(int x)
{
int n = 1;
while (x /= 10)
n++;
return n;
}

char *ngli_numbered_lines(const char *s)
{
struct bstr *b = ngli_bstr_create();
if (!b)
return NULL;

const int nb_lines = count_lines(s);
const int nb_digits = count_digits(nb_lines);
int line = 1;
while (*s) {
const size_t len = strcspn(s, "\n");
ngli_bstr_printf(b, "%*d %.*s\n", nb_digits, line++, (int)len, s);
if (!s[len])
break;
s += len + 1;
}

char *ret = ngli_bstr_strdup(b);
ngli_bstr_freep(&b);
if (!ret)
return NULL;

size_t len = strlen(ret);
while (len && ret[len - 1] == '\n')
ret[--len] = 0;

return ret;
}
1 change: 0 additions & 1 deletion libnopegl/src/utils/string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,5 @@

char *ngli_strdup(const char *s);
char *ngli_asprintf(const char *fmt, ...) ngli_printf_format(1, 2);
char *ngli_numbered_lines(const char *s);

#endif /* STRING_H */
Loading