Fixed issue with image buffers not being displayed properly

[skibu]

Video is worth a thousand words so here is what it is supposed to look like: https://youtu.be/81JUmu6EaWg

And here is what it looks like using an image buffer. Quite disappointing. https://youtu.be/W6i9Ffs2YzA

Please provide feedback if you have additional suggestions.

I tried to implement the dynamic scrolling of some pretty pictures in my Norns script that is under development. In order to do some interesting things graphically I tried reading in a PNG file into an image buffer, and then just writing the image buffer to the screen using screen.display_image(). The images showed up, but maybe a quarter of the time I just got a blank screen or just the lower part of the image. It was quite disappointing and not very pretty at all.

I did lots of exploring and testing and found that there is a timing issue. Screen commands are not executed fully immediately. Instead, they are queued and then processed by Cairo. It means that after doing a screen.clear() and then immediately afterwards a screen.display_image(png_buffer, x, y) that the clearing of the screen hadn't yet been completed. This resulted sometimes in the image buffer being overwritten by the clearing of the screen that was still in process. Likely, this is also occurring for other graphics operations, but just not yet noticed by anyone.

The solution was to change weaver.c _screen_clear() so that it doesn't return until the clear has been processed. I found that putting in a delay after the screen_clear() fixed the problem, but a delay is of course not a desirable solution. Since don't have good access to the workings of the screen queue, nor to Cairo, I found that needed to use a screen command that waits for results. This of course makes sure that the previous clear event is done. I chose the simplest screen function that returns a value, which is _screen_current_point().

The difference is like night and day. With the simple change to weaver.c the image buffer is always drawn correctly.

I created two scripts to illustrate the problem and the solution. Scripts are at https://github.com/skibu/nornsFun and are scrollImageFile.lua and scrollImageBuffer.lua .

scrollImageFile.lua shows what it is supposed to look like and doesn't actually use an image buffer. But scrollImageBuffer.lua will show the problem. And you can try the simple change to weaver.c to see how scrollImageBuffer.lua works with the change.

And lastly, I made screen.c consistent and added surface_may_have_color = true; to screen_surface_display(). I think it was previously just an oversight from when other screen changes were made a few months ago.

[dndrks]

i'm not confident enough to speak on the screen redraw backend, but wanted to add that @skibu 's changes test well for me -- seems like a very sensible solution!

i was able to get scrollImageBuffer.lua drawing appropriately without those changes, through @skibu's suggestion of just calling screen.current_point() after the screen.clear() in the Lua layer -- @tlubke, do you remember if the necessity of the screen.current_point() call is by design? if not, then it seems like the proposed changes are a good fix!

[tlubke]

... -- @tlubke, do you remember if the necessity of the screen.current_point() call is by design? if not, then it seems like the proposed changes are a good fix!

I wouldn't say the necessity is by design, but rather a consequence of moving Cairo operations into its own thread and some of the functions were required to be blocking asynchronous. Some of the open GH issues about screen seem related.

I'll need to get back into the code a little bit before I can say whether or not it's a good idea to call _screen_current_point() inside _screen_clear(). I'm curious if just screen_results_wait() would be enough to sync, or if there has to be an asynchronous event like current_point in order for that to work. (this definitely deadlocks the on the results semaphore if there are no results to wait on.

There are definitely some subtle timing things to work out with the screen that are probably beyond the scope of this particular PR. I'll check in here again soon.

[tlubke]

The solution might be to give all screen functions in weaver an event so that screen operations happen sequentially. Specifically screen_load_png(), screen_display_image(), and screen_display_image_region() for this example. I'll try that out quick.

EDIT: Yeah, that seems to do the trick. screen_load_png() already has a screen event. Not sure why screen_display_image() and screen_display_image_region() didn't get one. Is there any reason why they should exist outside of the event loop?

[tlubke]

2024-08-08_norns_pr_1795_d.patch

Here's a patch that adds two more screen events, one for display_surface() ("display_image" in Lua) and the other for display_surface_region(). This seems to fix https://github.com/skibu/nornsFun/blob/main/scrollImageBuffer.lua for me.

[skibu]

I’ll try out @tlubke ’s patch in a bit. I’m pretty confident that adding the new screen events is the proper solution. I can see how making functions like screen_display_image() event driven because the event queue would sequentially process the events as desired. Thanks for coming up with a solid solution!

Btw, I had also tried just using screen_results_wait(). It deadlocking meant that I needed a command that retrieved data in order to sync the commands.

[skibu]

I tested out @tlubke 's change and took out my original change. It works great! Therefore I checked in @tlubke 's changes into the branch for everyone to easily see.

I think that this PR can now be re-reviewed by folks and accepted if others agree.

[catfact]

Thanks awesome

Yes, queing everything is better than blocking the caller (main thread)

[tlubke]

Probably a good time to cross off #1747 too. I got started on it and modified scrollImageBuffer.lua to use screen.draw_to() on png_buffer. I occasionally see a flicker on the scrolling image, so not sure if I implemented everything correctly, or if I went too far with how many functions I made into events.

I'll push a branch and see what people think. https://github.com/tlubke/norns/commit/a995249f0f5b0b620a6204f79855112123b9f3ee

EDIT: I removed screen.update() from the function inside screen.draw_to() and the flicker went away. Could be a hint to what the issue might be.