search

Dewb / ndi-mod

norns system mod to share screen via the NDI streaming video standard
MIT License
11 stars 0 forks source link

Cairo context switching #4

Closed ngwese closed 2 years ago

ngwese commented 2 years ago

I noticed the comment on this line: https://github.com/Dewb/ndi-mod/blob/72edc9d7dec8788a29fca1385bfa381a736518bc/src/ndi_mod.cpp#L118

The screen_context_get_current() function was added recently as part of this change https://github.com/monome/norns/pull/1522. The motivation was to provide a means for the lua screen interface to change which underlying surface screen.update() ultimately targets, thus allowing offscreen drawing to an image to be added. There are few if any scripts which use the functionality as of now but that could change over time. Since the offscreen surfaces can have arbitrary dimensions which could lead to problems/SEGVs with the call to memcpy() to grab the data.

A potentially safer option would be to call screen_context_get_current() when the mod is being initialized and store the value since that will be guaranteed (more or less) to be the screen. In the ndi_mod_send_frame() function you could check the current context and if it doesn't match the one you grabbed at initialization time then exit early since the drawing is targeting something which isn't the screen.

Dewb commented 2 years ago

Thanks! yeah, I already have some improvements that eliminate the memcpy and get the frame size and stride directly from the surface, so if the context changes, it shouldn't SEGV. The offscreen surfaces are also ARGB, right? (NDI allows you to change the frame dimensions and other properties every frame.)

I do eventually want to allow for the possibility that a script that was aware of this mod could decide to write one thing to the screen and send something else out over NDI. But yeah for now storing the current context at startup and checking for a match sounds like a good idea.

ngwese commented 2 years ago

I think a SEGV is possible if the active screen context was one of the images targeted for offscreen drawing because it could easily be smaller than the screen. Depending on where that buffer was in the process address space the memcpy could attempt to read addresses which fall outside of the process address space.

Dewb commented 2 years ago

Oh yes, agreed, what I meant was I’ve already fixed that locally, apologies for not being clearer.

Dewb commented 2 years ago

It ended up being not that much trouble to let scripts send offscreen surfaces as additional NDI sources! https://github.com/Dewb/ndi-mod/blob/main/mod/examples/send_image.lua

image

Dewb commented 2 years ago

This should be fixed by d51f17e and 1b75a22e, thanks for the tips. Additional feedback absolutely welcome!