Fixed issue with image buffers not being displayed properly #1795
Conversation
Since screen_event_clear() only puts item a request into a queue it turns out that the clearing of the screen can actually not be finished until after the next screen command has started. This is especially apparent if the next screen command is screen.display_image() for displaying a buffer. Frequently the end result is that the image buffer is not displayed at all or only part of it becomes visible. Therefore need to execute a command that gets a return value to make sure that the screen has finished being cleared before this function returns. Using _screen_current_point() here completely fixes the problem. Wanted to just look at the screen queue to see if there was a way to wait until the queue was cleared but there did not appear to be such a solution.
It appears that during previous round of screen changes that screen_surface_display() was modified to include surface_may_have_color=true. But screen_surface_display() does the exact same thing, and therefore should be consistent. Therefore made the code consistent in order to avoid problems from cropping up.
|
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 |
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 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. |
|
The solution might be to give all screen functions in weaver an event so that screen operations happen sequentially. Specifically EDIT: Yeah, that seems to do the trick. |
|
2024-08-08_norns_pr_1795_d.patch Here's a patch that adds two more screen events, one for |
|
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 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. |
This code change was purely done by @tlubke. It addresses the issue of making sure a display image buffer command doesn't happen until the screen clear command has been executed. This was done by making the display image buffer commands events, which means they are processed by the event queue sequentially. Tested this change for PR monome#1795 and it works great. Reverted my original proposed change in weaver.c.
|
Thanks awesome Yes, queing everything is better than blocking the caller (main thread) |
|
Probably a good time to cross off #1747 too. I got started on it and modified scrollImageBuffer.lua to use I'll push a branch and see what people think. tlubke@a995249 EDIT: I removed |
|
so, i really haven't been able to get dug in to norns code for a while, i think you are the expert on the screen modules at this point. you now have write access; feel free to approve and merge if you see fit. |
|
Sounds good, I'll go over these changes again soon and see if I can remember where my train of thought was on my last edit. If there is an upcoming norns version release, it probably makes sense to get this fix merged in there. |
There was a problem hiding this comment.
I spent a little while tracing back through this code and some of that paths it effects. I think this is good to merge on a functional basis.
I realize these are mostly my changes and I had forgotten about the coding style in the wiki here. Looking at some more recent commits in matron though, they don't seem to bet strictly adhered to.
@skibu I'll leave it up to you if you want to adjust the formatting to match the wiki.
I've approved the PR, and will merge it in a week or two if there is no opposition from other maintainers on formatting.
| ev->buf = NULL; // Didn't make a copy of the pointer, nullify to avoid double free. | ||
| break; | ||
| case SCREEN_EVENT_DISPLAY_SURFACE_REGION: | ||
| screen_surface_display_region(ev->buf, ev->payload.d.d1, ev->payload.d.d2, ev->payload.d.d3, ev->payload.d.d4, ev->payload.d.d5, ev->payload.d.d6); |
There was a problem hiding this comment.
minor: coding style (c), column length.
| screen_event_data_push(&ev); | ||
| } | ||
|
|
||
| void screen_event_display_surface_region(void* surface, double left, double top, double width, double height, double x, double y) { |
There was a problem hiding this comment.
minor: coding style (c), column length + '*' alignment.
| @@ -33,6 +33,8 @@ extern void screen_event_close_path(void); | |||
| extern void screen_event_text_extents(const char *s); | |||
| extern void screen_event_export_png(const char *s); | |||
| extern void screen_event_display_png(const char *filename, double x, double y); | |||
| extern void screen_event_display_surface(void* surface, double x, double y); | |||
| extern void screen_event_display_surface_region(void* surface, double left, double top, double width, double height, double x, double y); | |||
There was a problem hiding this comment.
minor: coding style (c), column length + '*' alignment.
|
for what it's worth, the entire repo might benefit from having a clang-format pass (there is a rule, i think it should match the code style) |
|
Since I was asked, I don't see formatting issues as critical since they are currently only loosely adhered to. If folks want formatting to be consistent I think it is far better to enforce it automatically every time a file is checked into github. Therefore I agree that would be good to go ahead and merge. I don't think an official release should be done at this time though. After this PR I found another screen function that needs to be a screen event in order to prevent problems. Plus I found some other problematic bugs as well. I'll try to create issues for these soon so that they an be dealt with. |
|
I finally was able to create a separate issue for two other functions that also need to be queue functions. The issue is #1808 . This PR should be merged though instead of waiting for the new issue to also be resolved. |
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.