Syphon / Processing

Syphon Implementation for Processing
Other
137 stars 33 forks source link

Added explicit start method to syphon server #35

Closed cansik closed 5 years ago

cansik commented 6 years ago

Adds an explicit start method to the syphon sever as discussed in #34 .

Texture tex = pg.getTexture(source);
if (tex != null) {
  if (server == null) {
    start();
  }

The check if the server is null in the sendImage method is intended, because we can prevent a method invocation just for the same check in start and save a bit of performance.

vade commented 6 years ago

So its been a while since i touched the Processing / Java code, but when we first wrote this library GL context wasn't set up prior to draw in the OpenGL path iirc, and we had to rely on CGLGetCurrentContext to reliably know which OpenGL context to use, so we def had some limitations.

In terms of specific calls, I think if the JOGL resources / context always available at any point prior to setup being called, then awesome.

Is there a way to confirm that GL context doesnt change out from underneath us in Processing land?

Asking, because I honestly dont know!

vade commented 6 years ago

Also, thanks for this and the prior PR, we very much appreciate community contributions!

vade commented 6 years ago

Can you confirm @cansik that calling start very very early consistently works (either in setup, or even earlier if possible?) ? I dont have the latest processing setup, so im ill equipped to debug this. My only concern with the manual start is, to my knowledge, is if there is some sort of API context to gurantee when the underlying native OpenGL context is actually created for Processing. If we know when it is, and know we cant initialize against the wrong context, then I dont see any obvious issues!

Thank you again, and hope im not coming off nit-picky!

bangnoise commented 6 years ago

Thanks for looking into this and identifying the need for hasClients() to have a functioning server so it doesn't always return false if used to decide if a frame is published.

I'm not a huge fan of adding a start method because it puts the requirement to do GL work in the correct place on the user. I think we could handle this better internally without exposing it to the user. I would suggest the existing lazy init but consider a call to hasClients() as also being a reason to init the server.

There is no great harm in initting the underlying SyphonServer fairly early on - it won't create the IOSurface until a frame is published.

Note the Processing documentation describes when a library may do drawing work - which for our purposes includes initting a Syphon server.

https://github.com/processing/processing/wiki/Library-Basics

cansik commented 6 years ago

@bangnoise I think adding the lazy initialisation to the hasClients() method is good idea, but it makes the API even more counterintuitive. Because then sendImage() and hasClients() are doing more then they should, which is not really understandable by the user of the API.

I totally understand that changing this behaviour would break the API with older versions, so I left the lazy initialisation in the sendImage(), but gave me as developer the possibility to have to control over the initialisation process. This control is maybe not needed in a simple Processing sketch, but I use Processing as underlaying framework for larger projects, and so it would be great to have the control there.

I believe that with my PR both sides are happy, because you have the control if you need it, or you just let the library do the initialisation process.

Btw. if you add the lazy initialisation to the hasClients() method, it would make sense to have a start() method (or initialise() or whatever), which is called by sendImage() and hasClients(). Making this one public would fulfil the request of this PR.

cansik commented 6 years ago

I renamed the start() method to init() to have more intuitive API and added the texture check to the init() method. Is this really necessary for the initialisation, or just for the publishFrameTexture() method?

In addition I have added the lazy initialisation to the hasClients() method and removed the default return false.

bangnoise commented 6 years ago

I don't think the texture check is necessary, it looks like it just happened in that sequence in publishFrameTexture(). You can confirm it works as expected without it.

This looks great - still no need for init() to be public, as it is now correctly called by the other functions every time it is needed.

cansik commented 6 years ago

I removed the texture check and made the init() method private. I also added the reset of the server variable in the the stop() method.