Rotate output image + show markers for min an max

[poettig]

This implements support for rotating the output image by 180° when a camera is plugged in upside down (possible with USB-C) using a hotkey (U). It also implements a hotkey to show a marker in the output image for the minimum and maximum temperature (M).

I can also create a separate pull request for only the rotation part, but the current implementation of the min/max markers depends on the rotation changes (because of refactoring).

[jcalvinowens]

Great work, thank you! One really minor thing, otherwise this looks good.

[jcalvinowens]

Could you make center_point and color pointers instead of pass-by-value? They end up having to be pushed-and-popped from the stack when paint_colored_marker() is called.

On my Pi Zero W, I see a ~10% increase in CPU enabling the markers (48% -> 52%). If I change this to pass center_point and color as pointers instead of by value, I get half that back (48% -> 50%).

diff --git a/sdl.c b/sdl.c
index 64c1e47..d4ebeb3 100644
--- a/sdl.c
+++ b/sdl.c
@@ -547,16 +547,16 @@ static SDL_Point calc_point_from_buf_offset(const struct sdl_ctx *c, const int o
  * @param size The size (radius) of the marker.
  * @param color The color to use for the marker.
  */
-static void paint_colored_marker(const struct sdl_ctx *c, const SDL_Point center_point, const int size, const SDL_Color color)
+static void paint_colored_marker(const struct sdl_ctx *c, const SDL_Point *center_point, const int size, const SDL_Color *color)
 {
 	Uint8 original_red_color;
 	Uint8 original_green_color;
 	Uint8 original_blue_color;
 	Uint8 original_alpha;
 	SDL_GetRenderDrawColor(c->r, &original_red_color, &original_green_color, &original_blue_color, &original_alpha);
-	SDL_SetRenderDrawColor(c->r, color.r, color.g, color.b, color.a);
-	SDL_RenderDrawLine(c->r, center_point.x, center_point.y - size, center_point.x, center_point.y + size);
-	SDL_RenderDrawLine(c->r, center_point.x - size, center_point.y, center_point.x + size, center_point.y);
+	SDL_SetRenderDrawColor(c->r, color->r, color->g, color->b, color->a);
+	SDL_RenderDrawLine(c->r, center_point->x, center_point->y - size, center_point->x, center_point->y + size);
+	SDL_RenderDrawLine(c->r, center_point->x - size, center_point->y, center_point->x + size, center_point->y);
 	SDL_SetRenderDrawColor(c->r, original_red_color, original_green_color, original_blue_color, original_alpha);
 }
 
@@ -679,14 +679,16 @@ skippaint:
 		c->frame_paint_seq++;
 
 	if (c->showtext) {
+		SDL_Color c_color = {c->textval, c->textval, c->textval, 0xFF};
+
 		showtexts(c, raw_to_celsius(orig_max), raw_to_celsius(ptemp),
 			  raw_to_celsius(orig_min), seq);
 
-		paint_colored_marker(c, c->crosshair, 2, (SDL_Color){c->textval, c->textval, c->textval, 0xFF});
+		paint_colored_marker(c, &c->crosshair, 2, &c_color);
 
 		if (c->show_min_max_marker) {
-			paint_colored_marker(c, min_point, 1, SDL_COLOR_BLUE);
-			paint_colored_marker(c, max_point, 1, SDL_COLOR_RED);
+			paint_colored_marker(c, &min_point, 1, &SDL_COLOR_BLUE);
+			paint_colored_marker(c, &max_point, 1, &SDL_COLOR_RED);
 		}
 	}

[jcalvinowens]

This is the actual difference:

--- before.s    2024-12-29 22:32:29.584853013 -0800
+++ after.s     2024-12-29 22:32:28.982872909 -0800
@@ -86,58 +86,62 @@
        .arm
        .type   paint_colored_marker, %function
 paint_colored_marker:
-       @ args = 4, pretend = 0, frame = 16
+       @ args = 0, pretend = 0, frame = 8
        @ frame_needed = 0, uses_anonymous_args = 0
        push    {r4, r5, r6, r7, lr}    @
-       sub     sp, sp, #28     @,,
+       sub     sp, sp, #20     @,,
+@ sdl.c:551: {
+       mov     r7, r3  @ color, tmp162
+@ sdl.c:556:   SDL_GetRenderDrawColor(c->r, &original_red_color, &original_green_color, &original_blue_color, &original_alpha);
+       add     r3, sp, #15     @ tmp142,,
 @ sdl.c:551: {
-       mov     r4, r0  @ c, tmp156
-       add     r0, sp, #8      @ tmp133,,
-       mov     r5, r3  @ tmp157, size
-       stm     r0, {r1, r2}    @ tmp133,,
+       mov     r4, r0  @ c, tmp159
+       mov     r5, r2  @ size, tmp161
+       mov     r6, r1  @ center_point, tmp160
 @ sdl.c:556:   SDL_GetRenderDrawColor(c->r, &original_red_color, &original_green_color, &original_blue_color, &original_alpha);
-       add     r1, sp, #23     @ tmp139,,
-       str     r1, [sp]        @ tmp139,
-       add     r3, sp, #22     @,,
-       add     r2, sp, #21     @,,
-       add     r1, sp, #20     @,,
-       ldr     r0, [r4]        @, c_19(D)->r
-       ldr     r7, [sp, #8]    @ center_point$x, center_point.x
-       ldr     r6, [sp, #12]   @ center_point$y, center_point.y
+       str     r3, [sp]        @ tmp142,
+       add     r2, sp, #13     @,,
+       add     r3, sp, #14     @,,
+       add     r1, sp, #12     @,,
+       ldr     r0, [r0]        @, c_23(D)->r
        bl      SDL_GetRenderDrawColor          @

[poettig]

Oh damn, thats significant. I mostly write high-level code so I didn't think about that. I changed it to use pointers and malloc, unfortunately i'm not great at reading assembly. Can you check again if it worked out? I couldn't notice a difference in CPU usage on my laptop, but thats pretty overpowered for the use case compared to a PI Zero W.

[jcalvinowens]

This return-by-value is fine, SDL_Point is two ints so it can be returned in a pair of registers, and it gets inlined anyway.

[jcalvinowens]

Please run the make format I just added too, while you're at it.

[poettig]

Please run the make format I just added too, while you're at it.

Thanks for adding that, ran the format!

[jcalvinowens]

Dynamic allocation isn't strictly necessary: could you make it a field in the sdl_ctx struct, and pass the address of that field to paint_colored_marker() instead? The color structure could be declared on the stack like in the diff here: #5 (comment)

Again, this is a really minor point, I'd just like to avoid the malloc indirection where it isn't required.