Skip to content

Issue 51 - release GIL while calling ndiCommand - #52

Open
MattClarkson wants to merge 2 commits into
PlusToolkit:masterfrom
UCL:51-release-GIL
Open

Issue 51 - release GIL while calling ndiCommand#52
MattClarkson wants to merge 2 commits into
PlusToolkit:masterfrom
UCL:51-release-GIL

Conversation

@MattClarkson

Copy link
Copy Markdown

Hi @thompson318, @Sunderlandkyl, @adamrankin.

Who's best placed to look at this?

In terms of testing - i just ran it on an NDI Vega. It seemed to still track, but I did not have full access to an application to test it on. I'll try to get access to a Spectra shortly.

Kind regards
Matt

@MattClarkson

Copy link
Copy Markdown
Author

I just ran this on our SmartLiver clinical system. Without this fix, when I set the tracking thread to 60fps, the UI thread grinds down to about 4 fps. When I have this fix, the UI thread is stable at about 16-17 fps (on a Qt timer set to 20 fps, so - about what I would expect).

So, while I didnt check that tracking transforms had changed or not, it did improve concurrency.

@thompson318, @Sunderlandkyl, @adamrankin.

@adamrankin

Copy link
Copy Markdown
Member

Looks straightforward, but I'm not familiar with threading in python.

Should this be an #ifdef compile option in case it breaks things for other users?

If you're able to test/confirm that tracking is unaffected, that would be ideal.

Comment thread ndicapimodule.cxx
}
else
{
Py_DECREF(initial);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Again, not my expertise, but why are these two reference decreases repeated? Is this a separate bug being fixed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have to be honest and say it was suggested by AI as a separate bug, so I left it in there. I don't really know how to test it in a reasonable time frame? Is there a simple test harness, where we could output the ref count while tracking?

@MattClarkson

Copy link
Copy Markdown
Author

Looks straightforward, but I'm not familiar with threading in python.

Should this be an #ifdef compile option in case it breaks things for other users?

If you're able to test/confirm that tracking is unaffected, that would be ideal.

OK. My next opportunity is Thursday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants