Osc shader by avilleret · Pull Request #223 · raspberrypi/userland

avilleret · 2015-02-23T18:32:36Z

this patch add a new OpenGL scene to Raspistill application.
It's called shader and let the user load vertex and fragment shader via command line.
Also, if compiled with liblo, it automatically exposed each uniform via OSC.
Thus any shader parameter can be controlled by another OSC capable application locally on the Pi or remotely.

…l it work only with uniform for now TODO : support varying

this doesn't seem to work for cross-compiling yet

don't fully understand all the consequences of this...

don't know if it is a good idea since the image is black with this setting

Ruffio · 2016-11-18T12:35:31Z

@6by9 does this PR have any chance to be merged (look aside from the conflicts)? If not, it should be closed

6by9

Already done. Needs to be dropped. It's not relevant to the PR either.

6by9 · 2016-11-18T13:48:25Z

I know next to nothing about OpenGL so can't comment on that side. I don't know if @timgover has a few minutes and would be prepared to review the GL side of this.

There are several other bits mixed in to the PR as well (eg liblo, and extra exposure mode) - if this is for a new OpenGL scene then it should ideally be just that bit. Raise separate PRs for the other things.
There's also no need for the development history that lead to the PR - changes should really be squashed into one commit.

I will find 10 mins to go through and review this, but comments like "don't fully understand all the consequences of this.." instill a deep sense of foreboding.

# Conflicts: # host_applications/linux/apps/raspicam/CMakeLists.txt # host_applications/linux/apps/raspicam/RaspiStill.c

avilleret · 2016-11-18T20:50:44Z

Hi,

I just merge master into osc-shader branch to fix merging conflict.
Merging should be safe now.
Liblo is optionnal, if you don't have it, it will not be enabled and build fine.
You will still be able to load vertex and fragment shader, but you can't control it with OSC.

Fell free to ask me some modification before merging.

timg236

I've only scanned the GL code, it looks ok, a few minor comments. It would be useful to have something in a comment that explains how people can use this.

It would be better if the GL scenes were plugins that were dynamically loaded with a slightly better defined GL interface. However, that's my fault for not designing it that way in the first place!

timg236 · 2016-11-21T17:03:40Z

host_applications/linux/apps/raspicam/RaspiTexUtil.c

+      array[i].size   = 0;
+      array[i].type   = 0;
+      array[i].loc    = 0;
+      // uniform_array[i].param  = (float*) malloc(sizeof(float)*16);


remove commented out code

timg236 · 2016-11-21T17:03:52Z

host_applications/linux/apps/raspicam/RaspiTexUtil.c

+      array[i].size   = 0;
+      array[i].type   = 0;
+      array[i].loc    = 0;
+      // attribute_array[i].param  = (float*) malloc(sizeof(float)*16);


remove commented out code

timg236 · 2016-11-21T17:07:28Z

host_applications/linux/apps/raspicam/gl_scenes/shader.c

+
+static const GLfloat vertices[] =
+{
+#define V0  -0.8,  0.8,  0.8,


Any reason for the choice of 0.8 ?

can't remember, maybe just aesthetic

timg236 · 2016-11-21T17:10:17Z

host_applications/linux/apps/raspicam/gl_scenes/shader.c

+
+   // allocate memory to contain the whole file:
+   shader.fragment_source = (GLchar*) malloc (sizeof(GLchar)*lSize);
+   if (shader.fragment_source == NULL) {fputs ("Memory error",stderr); exit (2);}


If these files are huge then it might be better to use mmap instead of malloc. If they are small then it's simpler to assume that malloc will succeed in userspace linux applications

then I'll assume that malloc succeed

timg236 · 2016-11-21T17:11:25Z

host_applications/linux/apps/raspicam/gl_scenes/shader.c

+   if (result != lSize) {fputs ("Reading error",stderr); exit (3);}
+
+   printf("vertex shader file : %s\n", state->vertex_shader_filename);
+


It would be cleaner to return an erorr from this function instead of just exiting. RaspiStill might want to do explicit cleanup

then shader_open() will return -1 on fread error, 0 on success and nothing if malloc fails

timg236 · 2016-11-21T17:12:11Z

host_applications/linux/apps/raspicam/gl_scenes/shader.c

+
+   pfFile = fopen (state->fragment_shader_filename,"rb");
+   if (pfFile==NULL) {
+      printf("No or invalid fragment file provided, used the default one\n");


It might be nicer to use stderr

avilleret · 2017-01-13T13:27:50Z

Do I need to make more changes to see this PR merged ?

JamesH65 · 2018-01-18T11:38:26Z

@avilleret @6by9 Another one that has slipped through the cracks - what should we do here?

JamesH65 · 2018-07-02T15:19:21Z

@avilleret Sorry about the delay, would it be possible to rebase against the latest tree then we can try and get this merged.

JamesH65 · 2018-12-13T10:27:40Z

@avilleret ping

avilleret · 2018-12-13T10:34:23Z

I'll try to rebase this against master in the next few weeks and let you know

JamesH65 · 2018-12-13T13:27:24Z

Thanks. Quite a few changes in the camera code, so good luck!

# Conflicts: # host_applications/linux/apps/raspicam/CMakeLists.txt # host_applications/linux/apps/raspicam/RaspiTex.c # host_applications/linux/apps/raspicam/RaspiTex.h

avilleret · 2018-12-25T19:14:31Z

@JamesH65 could you give it a try ? I don't have a camera to check until January 6th

Ruffio · 2020-05-03T13:14:22Z

@avilleret what do you say?

avilleret · 2020-05-03T14:08:49Z

@Ruffio, sorry, what is the question ?

Ruffio · 2020-05-04T07:27:17Z

@avilleret did you check with camera?
@JamesH65 could you give it a try ?

JamesH65 · 2020-05-04T08:10:33Z

Sorry, this has just completely slipped past. I have no idea whether this would work on a Pi4, I suspect not. @timg236

Unfortunately needs to be rebased again.

timg236 · 2020-05-04T08:16:02Z

Sorry, this has just completely slipped past. I have no idea whether this would work on a Pi4, I suspect not. @timg236

Unfortunately needs to be rebased again.

The Broadcom specific extensions are never going to work on Pi4, although if someone was sufficiently enthusiastic they could implement equivalent functionality using GLES 3.0 / MESA on Pi4.

Ruffio · 2020-05-12T12:44:27Z

If this specific PR is never going to work, and therefor never goint to be accepted, then it should be rejected IMHO.

timg236 · 2020-05-12T13:05:01Z

It will work on Pi3 or older so long as the legacy GL driver is selected which I believe is the default

avilleret added 11 commits February 18, 2015 12:13

add a structure to hold shader parameters state

ac0be97

add method to create array of shader parameters and automatically fil…

04225cf

…l it work only with uniform for now TODO : support varying

add an OpenGL scene

eed9739

include the new scene in build system

a859fd3

new scene files

b2331ed

add OSC support

529f4cf

try to detect liblo automatically

8e4a0fd

this doesn't seem to work for cross-compiling yet

do not redefined VCOS_LOG_CATEGORY

8809f9f

don't fully understand all the consequences of this...

fix typo

0f78260

add some tips to cross-compile with liblo

635d169

add a OFF parameter for exposure setting

e52509a

don't know if it is a good idea since the image is black with this setting

6by9 suggested changes Nov 18, 2016

View reviewed changes

Merge branch 'master' into osc-shader

8cf597e

# Conflicts: # host_applications/linux/apps/raspicam/CMakeLists.txt # host_applications/linux/apps/raspicam/RaspiStill.c

timg236 reviewed Nov 21, 2016

View reviewed changes

avilleret and others added 2 commits November 29, 2016 20:23

some changes according to @timgover review

fe627e0

Merge branch 'master' into osc-shader

150cb61

JamesH65 added the Waiting for input from originator label Jul 2, 2018

avilleret and others added 3 commits December 25, 2018 19:24

fix build

ddd6a66

Merge branch 'master' into osc-shader

1a7c19b

Merge remote-tracking branch 'avilleret/osc-shader' into osc-shader

3bbf273

# Conflicts: # host_applications/linux/apps/raspicam/CMakeLists.txt # host_applications/linux/apps/raspicam/RaspiTex.c # host_applications/linux/apps/raspicam/RaspiTex.h

		if (result != lSize) {fputs ("Reading error",stderr); exit (3);}

		printf("vertex shader file : %s\n", state->vertex_shader_filename);

Conversation

avilleret commented Feb 23, 2015

Uh oh!

Ruffio commented Nov 18, 2016

Uh oh!

6by9 left a comment

Choose a reason for hiding this comment

Uh oh!

6by9 commented Nov 18, 2016

Uh oh!

avilleret commented Nov 18, 2016

Uh oh!

timg236 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avilleret commented Jan 13, 2017

Uh oh!

JamesH65 commented Jan 18, 2018

Uh oh!

JamesH65 commented Jul 2, 2018

Uh oh!

JamesH65 commented Dec 13, 2018

Uh oh!

avilleret commented Dec 13, 2018

Uh oh!

JamesH65 commented Dec 13, 2018

Uh oh!

avilleret commented Dec 25, 2018

Uh oh!

Ruffio commented May 3, 2020

Uh oh!

avilleret commented May 3, 2020

Uh oh!

Ruffio commented May 4, 2020

Uh oh!

JamesH65 commented May 4, 2020

Uh oh!

timg236 commented May 4, 2020

Uh oh!

Ruffio commented May 12, 2020

Uh oh!

timg236 commented May 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants