Skip to content
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.

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

Fix crash when using stream callback in Python #714

Merged
merged 4 commits into from
Sep 20, 2022

Conversation

lostcontrol
Copy link
Contributor

This PR should fix the crash seen in #453. The crash was caused by ArvStreamCallback only living for the time of the call to arv_camera_create_stream because of (scope call). Using (scope notified) allows us to control the lifetime of the callback.

Also added a small Python sample showing how to use stream callback.

⚠️ GObject & GLib noob here, careful review required.

With (scope call), PyGObject assumes the callback will only be used for
the lifetime of the call to create_stream. However, Aravis keeps track of
the callback pointer and calls it in a separate thread, multiple times,
way after the invocation of create_stream is over. That pointer is then no
more valid, causing a crash.

With (scope notified), PyGObject waits until the GDestroyNotify argument
is called to destroy the callback data.

Fix AravisProject#453
@lostcontrol
Copy link
Contributor Author

Applying clang-format to the files results in many unrelated changes. It seems also like there is a mix of tabs and spaces already.

I didn't write any tests. Should I extend tests/fake.py to also use a callback?

@EmmanuelP
Copy link
Contributor

Applying clang-format to the files results in many unrelated changes. It seems also like there is a mix of tabs and spaces already.

Don't bother about clang-format. I should remove it from the repository.

I didn't write any tests. Should I extend tests/fake.py to also use a callback?

That would be nice, yes.

Copy link
Contributor

@EmmanuelP EmmanuelP left a comment

Choose a reason for hiding this comment

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

Looks fine.

The ArvDevice::create_stream signature change is an API break, but adding a new create_stream_full would still be an ABI break. ArvDevice virtual methods are not really meant to be public, so your change is fine.

src/arvcamera.c Outdated Show resolved Hide resolved
src/arvdevice.c Outdated Show resolved Hide resolved
The *_full methods have been introduced after 0.8.22.
@lostcontrol
Copy link
Contributor Author

I also quickly tested with a Lucid TRI064S-M. Seems to work fine.

@lostcontrol lostcontrol marked this pull request as ready for review September 12, 2022 12:07
@EmmanuelP
Copy link
Contributor

Thanks a lot Cyril. Everything looks fine now.

I'm still a bit worried about the ABI break. So before merging these changes, I have to check what was the ABI stability of Aravis during the 0.8 series, and see if this sort of breakage already happened. If not, I will probably switch to a new series. It may take a couple of weeks.

@EmmanuelP EmmanuelP merged commit c9965a4 into AravisProject:main Sep 20, 2022
@EmmanuelP
Copy link
Contributor

It already happened in the 0.8 series a class definition has changed, so let's commit these changes.

Subclassing Aravis classes is not really supported. It should be made more clear by either making most of the class definition private (which would still allow subclassing, but will not permit to overwrite the virtual functions), or by simply moving the class definition in the private headers. That will not happen in the 0.8 series though.

Thanks a lot for your work on this issue. I had a look several time at it without success.

Cheers.

@bpinsard
Copy link

Thanks a lot for the fix to both a you, just tested it and works perfectly. Will close the issue.

@lostcontrol lostcontrol deleted the python-callback branch September 23, 2022 12:30
@lostcontrol
Copy link
Contributor Author

Thank you for your feedback. By the way, Asyril SA, my employer, "sponsored" this fix by letting me work on it during my working time.

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.

3 participants