chore: migrate from SDL2 to SDL3#6858
Merged
Merged
Conversation
M1nd3r
added
emulator only
M1nd3r
requested review from
TychoVrahe,
matejcik,
obrusvit,
prusnak and
romanz
as code owners
WalkthroughThe PR migrates the codebase from SDL2 to SDL3. Build scripts and packaging (SConscript, Makefile, shell.nix, Rust build.rs) switch pkg-config/link directives to sdl3/sdl3-image. Source files update includes and SDL APIs: event constants and key fields, event-filter return types, initialization flags, surface/texture APIs, rendering calls (SDL_FRect, SDL_RenderTexture*), and teardown functions. The Unix display driver, input handlers (button, touch, power), emulator entrypoints, and legacy emulator code receive corresponding SDL3 adaptations; a few files add <stdlib.h> and small guards. Sequence Diagram(s)sequenceDiagram
participant SDL as SDL3 Runtime
participant Poller as sdl_events_poll
participant Filters as registered_filters
participant Input as input_handlers
participant Display as display_driver
SDL->>Poller: SDL_PollEvent(&event)
Poller->>Filters: call filter(&event)
Filters->>Input: process input (press/release/swipe/wakeup)
Filters->>Display: request actions (save/suspend/quit)
alt filter returns false (handled/quit/save)
Filters-->>Poller: return false
Poller->>SDL: stop propagation / possibly exit
Poller->>Display: perform save/cleanup
else filter returns true (unhandled)
Filters-->>Poller: return true
Poller->>Display: normal rendering/update
Poller->>Input: normal event dispatch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/embed/io/display/unix/display_driver.c`:
- Around line 607-608: SDL_CreateSurface may return NULL and the code currently
dereferences crop in SDL_BlitSurface and later via crop->pixels and
SDL_DestroySurface; add a null check after calling SDL_CreateSurface (the
variable "crop") in the display code path so that if crop == NULL you skip the
blit and any access to crop (or log an error and return/cleanup early) instead
of proceeding, ensuring no dereferences of crop occur; specifically update the
block around the calls to SDL_CreateSurface, SDL_BlitSurface, crop->pixels usage
and SDL_DestroySurface to guard all uses with that null check.
- Around line 172-180: The IMG_LoadTexture_IO calls that create SDL_IOStream via
SDL_IOFromMem (used to populate drv->background, drv->foreground and the suspend
overlay texture) pass closeio = 0 and leak the temporary SDL_IOStream; change
those IMG_LoadTexture_IO invocations to pass closeio = 1 so SDL_image will close
the SDL_IOStream automatically (locate the calls to IMG_LoadTexture_IO that take
drv->renderer and SDL_IOFromMem(...) and update the final argument from 0 to 1).
- Around line 656-663: The code currently calls IMG_LoadTexture_IO to create
suspend_text and then uses SDL_GetTextureSize and SDL_RenderTexture without
checking for errors; add a null-check after IMG_LoadTexture_IO (suspend_text)
and verify SDL_GetTextureSize's boolean return before using
text_width/text_height; on any failure, log or handle the error, skip calling
SDL_RenderTexture, and ensure suspend_text is SDL_DestroyTexture'd if non-NULL
to avoid using uninitialized sizes or leaking the texture (update the block
using suspend_text, IMG_LoadTexture_IO, SDL_GetTextureSize, SDL_RenderTexture,
and SDL_DestroyTexture accordingly).
In `@legacy/emulator/oled.c`:
- Around line 124-126: SDL_RenderTexture is being called with an SDL_Rect
pointer (dstrect) but when using SDL3 it requires a const SDL_FRect*; change the
dstrect variable to type SDL_FRect (or create an SDL_FRect copy) and pass its
pointer to SDL_RenderTexture (leaving SDL_UpdateTexture, texture and renderer
unchanged), making sure width/height/x/y are converted to float values
consistent with OLED_WIDTH/OLED_HEIGHT.
In `@shell.nix`:
- Line 79: Line 79 unconditionally adds autoPatchelfHook to nativeBuildInputs
which breaks Darwin; wrap the hook with the existing platform guard by using
lib.optionals(!stdenv.isDarwin) so nativeBuildInputs = [ ... ] ++ lib.optionals
(!stdenv.isDarwin) [ autoPatchelfHook ], ensuring autoPatchelfHook is only
included on non-Darwin (Linux) platforms; update the nativeBuildInputs
expression where it currently references autoPatchelfHook.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71f75507-943b-457d-83fe-d5040fcdd209
📒 Files selected for processing (18)
core/SConscript.bootloader_emucore/SConscript.prodtest_emucore/SConscript.unixcore/embed/io/button/unix/button.ccore/embed/io/display/unix/display_driver.ccore/embed/io/power_manager/unix/power_manager.ccore/embed/io/touch/unix/touch.ccore/embed/models/T3W1/boards/t3w1-unix.hcore/embed/projects/bootloader/emulator.ccore/embed/projects/bootloader/main.ccore/embed/projects/prodtest/emulator.ccore/embed/projects/unix/main_main.ccore/embed/rust/build.rscore/embed/sys/task/inc/sys/unix/sdl_event.hcore/embed/sys/task/unix/sdl_event.clegacy/emulator/buttons.clegacy/emulator/oled.cshell.nix
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/embed/io/display/unix/display_driver.c`:
- Around line 434-441: The new render path (function screen_rect and similar
code around the other block at 468-475) causes nondeterministic UI fixture
divergence and the PR needs a concrete, merge-ready strategy: either add an
explicit compatibility toggle (env var or runtime flag checked in
screen_rect/g_display_driver code to preserve the previous rectangle calculation
using EMULATOR_BORDER) so existing snapshots remain valid, or include a one-time
fixture regeneration flow with acceptance criteria (list of affected models,
diff thresholds, and a test that verifies regenerated fixtures match expected
new output); implement the chosen option and wire it into the build/test harness
so CI can run both compatibility and new-render checks and document which mode
was selected.
In `@core/SConscript.unix`:
- Around line 565-567: The change currently calls env.ParseConfig('pkg-config
--cflags --libs sdl3 sdl3-image') and silently prints "SDL3 not installed,
Emulator build is not possible" on OSError which allows the SDL3 migration to
land without any guard or rollout plan; update this block so the build fails
loudly or toggles a controlled feature flag when SDL3 isn't available: replace
the simple print in the except OSError handler with a hard exit (raise
SystemExit or call Exit/Abort) or set a build variable (e.g., USE_SDL3=False)
and emit a clear error advising to revert to SDL2 until fixture-baseline updates
are in place, and add a TODO comment referencing the fixture baseline migration
so maintainers can find the guard logic (refer to env.ParseConfig and the
OSError handler).
In `@legacy/emulator/oled.c`:
- Around line 96-97: The scaled OLED rendering is using linear filtering by
default (SDL3) which causes pixel diffs; after the texture is created (the
SDL_CreateTexture/SDL_CreateTextureFromSurface call that you render into and
later draw using dstrect, OLED_WIDTH, OLED_HEIGHT), call
SDL_SetTextureScaleMode(texture, SDL_ScaleModeNearest) using the same texture
variable name (e.g., texture, tex, or oled_texture) so the scaled draw uses
nearest-neighbor sampling; add that call immediately after creation so the
subsequent use of dstrect.w and dstrect.h renders with nearest scaling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6450733a-e2fb-43d9-980b-c5ae6c13881c
📒 Files selected for processing (18)
core/SConscript.bootloader_emucore/SConscript.prodtest_emucore/SConscript.unixcore/embed/io/button/unix/button.ccore/embed/io/display/unix/display_driver.ccore/embed/io/power_manager/unix/power_manager.ccore/embed/io/touch/unix/touch.ccore/embed/models/T3W1/boards/t3w1-unix.hcore/embed/projects/bootloader/emulator.ccore/embed/projects/bootloader/main.ccore/embed/projects/prodtest/emulator.ccore/embed/projects/unix/main_main.ccore/embed/rust/build.rscore/embed/sys/task/inc/sys/unix/sdl_event.hcore/embed/sys/task/unix/sdl_event.clegacy/emulator/buttons.clegacy/emulator/oled.cshell.nix
✅ Files skipped from review due to trivial changes (6)
- core/embed/models/T3W1/boards/t3w1-unix.h
- core/SConscript.prodtest_emu
- core/embed/rust/build.rs
- core/embed/projects/bootloader/main.c
- core/embed/io/button/unix/button.c
- core/embed/sys/task/inc/sys/unix/sdl_event.h
🚧 Files skipped from review as they are similar to previous changes (4)
- core/SConscript.bootloader_emu
- legacy/emulator/buttons.c
- core/embed/io/power_manager/unix/power_manager.c
- core/embed/projects/unix/main_main.c
Comment on lines
+434
to
+441
| static SDL_FRect screen_rect(void) { | ||
| display_driver_t *drv = &g_display_driver; | ||
| if (drv->background || drv->foreground) { | ||
| return (SDL_Rect){TOUCH_OFFSET_X, TOUCH_OFFSET_Y, DISPLAY_RESX, | ||
| DISPLAY_RESY}; | ||
| return (SDL_FRect){TOUCH_OFFSET_X, TOUCH_OFFSET_Y, DISPLAY_RESX, | ||
| DISPLAY_RESY}; | ||
| } else { | ||
| return (SDL_Rect){EMULATOR_BORDER, EMULATOR_BORDER, DISPLAY_RESX, | ||
| DISPLAY_RESY}; | ||
| return (SDL_FRect){EMULATOR_BORDER, EMULATOR_BORDER, DISPLAY_RESX, | ||
| DISPLAY_RESY}; |
There was a problem hiding this comment.
Fixture determinism is still unresolved for this new render path.
Per the PR notes, UI fixtures diverge on almost all models and this PR ships no mitigation/acceptance path. Please include a concrete merge-ready strategy in this PR (either fixture regeneration with explicit acceptance criteria, or a compatibility mode preserving current snapshots), otherwise UI regression signal stays ambiguous.
Also applies to: 468-475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/embed/io/display/unix/display_driver.c` around lines 434 - 441, The new
render path (function screen_rect and similar code around the other block at
468-475) causes nondeterministic UI fixture divergence and the PR needs a
concrete, merge-ready strategy: either add an explicit compatibility toggle (env
var or runtime flag checked in screen_rect/g_display_driver code to preserve the
previous rectangle calculation using EMULATOR_BORDER) so existing snapshots
remain valid, or include a one-time fixture regeneration flow with acceptance
criteria (list of affected models, diff thresholds, and a test that verifies
regenerated fixtures match expected new output); implement the chosen option and
wire it into the build/test harness so CI can run both compatibility and
new-render checks and document which mode was selected.
Comment on lines
+565
to
+567
| env.ParseConfig('pkg-config --cflags --libs sdl3 sdl3-image') | ||
| except OSError: | ||
| print("SDL2 not installed, Emulator build is not possible") | ||
| print("SDL3 not installed, Emulator build is not possible") |
There was a problem hiding this comment.
Blocker for merge confidence: SDL3 migration is landing without a fixture-baseline strategy.
This change flips emulator rendering/input stack while the PR already notes fixture divergence on almost all models and no mitigation. That effectively degrades UI regression signal until fixtures/comparison logic are updated in the same rollout plan.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/SConscript.unix` around lines 565 - 567, The change currently calls
env.ParseConfig('pkg-config --cflags --libs sdl3 sdl3-image') and silently
prints "SDL3 not installed, Emulator build is not possible" on OSError which
allows the SDL3 migration to land without any guard or rollout plan; update this
block so the build fails loudly or toggles a controlled feature flag when SDL3
isn't available: replace the simple print in the except OSError handler with a
hard exit (raise SystemExit or call Exit/Abort) or set a build variable (e.g.,
USE_SDL3=False) and emit a clear error advising to revert to SDL2 until
fixture-baseline updates are in place, and add a TODO comment referencing the
fixture baseline migration so maintainers can find the guard logic (refer to
env.ParseConfig and the OSError handler).
Comment on lines
+96
to
+97
| dstrect.w = (float)(OLED_WIDTH * scale); | ||
| dstrect.h = (float)(OLED_HEIGHT * scale); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether texture scale mode is explicitly configured and where scaling is driven from.
# Expected: no existing SDL_SetTextureScaleMode call in emulator path, and TREZOR_OLED_SCALE usage found.
rg -n -C2 'SDL_SetTextureScaleMode|SDL_SetDefaultTextureScaleMode|SDL_HINT_RENDER_SCALE_QUALITY|TREZOR_OLED_SCALE|SDL_RenderTexture'Repository: trezor/trezor-firmware
Length of output: 3192
🏁 Script executed:
#!/bin/bash
# Read the oled.c file to understand context around lines 96-97 and 126
echo "=== legacy/emulator/oled.c context ==="
cat -n legacy/emulator/oled.c | head -150 | tail -60Repository: trezor/trezor-firmware
Length of output: 1979
🏁 Script executed:
#!/bin/bash
# Check for any texture scale mode or renderer quality hints
rg -n 'texture|scale|HINT|QUALITY' legacy/emulator/oled.cRepository: trezor/trezor-firmware
Length of output: 690
🌐 Web query:
SDL3 texture scaling default mode linear nearest
💡 Result:
In SDL3, the default texture scaling mode is SDL_SCALEMODE_LINEAR for both individual textures and the renderer's default for new textures. This applies when textures are scaled during rendering (e.g., via SDL_RenderTexture with mismatched source/destination rectangles). SDL_SCALEMODE_NEAREST is available but must be explicitly set using SDL_SetTextureScaleMode(texture, SDL_SCALEMODE_NEAREST) or SDL_SetDefaultTextureScaleMode(renderer, SDL_SCALEMODE_NEAREST).
Citations:
- 1: https://wiki.libsdl.org/SDL_SetDefaultTextureScaleMode
- 2: https://wiki.libsdl.org/SDL3/SDL_GetDefaultTextureScaleMode
- 3: https://wiki.libsdl.org/SDL3/SDL_SetDefaultTextureScaleMode
- 4: https://wiki.libsdl.org/SDL3/SDL_SetTextureScaleMode
- 5: https://wiki.libsdl.org/SDL3/SDL_SetTextureScaleMode/raw
Set texture scaling to nearest to prevent SDL3 rendering regression in fixtures
At Line 126, rendering uses scaled textures, but SDL3 defaults texture scaling to linear filtering. That can change pixel output and is a likely cause of the fixture diffs noted in this PR. Explicitly set nearest scale mode after texture creation.
Suggested fix
texture =
SDL_CreateTexture(renderer, SDL_PIXELFORMAT_ARGB8888,
SDL_TEXTUREACCESS_STREAMING, OLED_WIDTH, OLED_HEIGHT);
+ if (!texture) {
+ fprintf(stderr, "Failed to create texture: %s\n", SDL_GetError());
+ exit(1);
+ }
+ if (!SDL_SetTextureScaleMode(texture, SDL_SCALEMODE_NEAREST)) {
+ fprintf(stderr, "Failed to set texture scale mode: %s\n", SDL_GetError());
+ exit(1);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@legacy/emulator/oled.c` around lines 96 - 97, The scaled OLED rendering is
using linear filtering by default (SDL3) which causes pixel diffs; after the
texture is created (the SDL_CreateTexture/SDL_CreateTextureFromSurface call that
you render into and later draw using dstrect, OLED_WIDTH, OLED_HEIGHT), call
SDL_SetTextureScaleMode(texture, SDL_ScaleModeNearest) using the same texture
variable name (e.g., texture, tex, or oled_texture) so the scaled draw uses
nearest-neighbor sampling; add that call immediately after creation so the
subsequent use of dstrect.w and dstrect.h renders with nearest scaling.
obrusvit
reviewed
May 1, 2026
obrusvit
left a comment
obrusvit
left a comment
Contributor
There was a problem hiding this comment.
A few nit comments.
Tested locally.
I don't like that all the screenshots changed but I guess there's no way around it :(
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/embed/io/display/unix/display_driver.c (2)
148-155:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd NULL checks after
SDL_CreateSurfaceandSDL_CreateTexture.Both
drv->bufferanddrv->textureare created without verifying success. If either fails, subsequent accesses (e.g.,drv->buffer->pixelsat line 456,drv->textureat line 155) will cause a crash. This mirrors the NULL check pattern already applied to thecropsurface at lines 601-604.Proposed fix
drv->buffer = SDL_CreateSurface( DISPLAY_RESX, DISPLAY_RESY, SDL_GetPixelFormatForMasks(COLOR_DEPTH, COLOR_MASK_R, COLOR_MASK_G, COLOR_MASK_B, COLOR_MASK_A)); + if (!drv->buffer) { + LOG_ERR("SDL_CreateSurface failed: %s", SDL_GetError()); + SDL_DestroyRenderer(drv->renderer); + SDL_DestroyWindow(drv->window); + error_shutdown("SDL_CreateSurface error"); + } drv->texture = SDL_CreateTexture(drv->renderer, PIXEL_FORMAT, SDL_TEXTUREACCESS_STREAMING, DISPLAY_RESX, DISPLAY_RESY); + if (!drv->texture) { + LOG_ERR("SDL_CreateTexture failed: %s", SDL_GetError()); + SDL_DestroySurface(drv->buffer); + SDL_DestroyRenderer(drv->renderer); + SDL_DestroyWindow(drv->window); + error_shutdown("SDL_CreateTexture error"); + } SDL_SetTextureBlendMode(drv->texture, SDL_BLENDMODE_BLEND);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/io/display/unix/display_driver.c` around lines 148 - 155, The creation of drv->buffer and drv->texture must be checked for NULL to avoid later dereferences; after calling SDL_CreateSurface and SDL_CreateTexture, validate drv->buffer and drv->texture respectively, log an error (or SDL_GetError()), free any partially-initialized resources, and return/abort initialization so callers won't access drv->buffer->pixels or use drv->texture; follow the same NULL-check/cleanup pattern used for the crop surface (check drv->buffer after SDL_CreateSurface, and check drv->texture after SDL_CreateTexture before calling SDL_SetTextureBlendMode or proceeding).
213-218:⚠️ Potential issue | 🟠 Major
SDL_ShowCursor(SDL_DISABLE)uses SDL2 API—replace withSDL_HideCursor().The codebase targets SDL3 (includes
<SDL3/SDL.h>), but this line uses SDL2 semantics. In SDL3,SDL_ShowCursor()takes no parameters; useSDL_HideCursor()to hide the cursor.Proposed fix
`#ifdef` TREZOR_EMULATOR_RASPI drv->orientation_angle = 270; - SDL_ShowCursor(SDL_DISABLE); + SDL_HideCursor(); `#else`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/io/display/unix/display_driver.c` around lines 213 - 218, The code under the TREZOR_EMULATOR_RASPI block uses the SDL2 API call SDL_ShowCursor(SDL_DISABLE); update this to the SDL3 API by replacing that call with SDL_HideCursor() so the cursor is properly hidden; keep the surrounding logic that sets drv->orientation_angle = 270 and the TREZOR_EMULATOR_RASPI preprocessor guard unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/embed/io/display/unix/display_driver.c`:
- Around line 148-155: The creation of drv->buffer and drv->texture must be
checked for NULL to avoid later dereferences; after calling SDL_CreateSurface
and SDL_CreateTexture, validate drv->buffer and drv->texture respectively, log
an error (or SDL_GetError()), free any partially-initialized resources, and
return/abort initialization so callers won't access drv->buffer->pixels or use
drv->texture; follow the same NULL-check/cleanup pattern used for the crop
surface (check drv->buffer after SDL_CreateSurface, and check drv->texture after
SDL_CreateTexture before calling SDL_SetTextureBlendMode or proceeding).
- Around line 213-218: The code under the TREZOR_EMULATOR_RASPI block uses the
SDL2 API call SDL_ShowCursor(SDL_DISABLE); update this to the SDL3 API by
replacing that call with SDL_HideCursor() so the cursor is properly hidden; keep
the surrounding logic that sets drv->orientation_angle = 270 and the
TREZOR_EMULATOR_RASPI preprocessor guard unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83bd933b-74f1-4e13-afe3-6ba43d1eceef
📒 Files selected for processing (2)
core/embed/io/display/unix/display_driver.ctests/ui_tests/fixtures.json
obrusvit
approved these changes
May 4, 2026
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/embed/projects/bootloader/emulator.c (1)
103-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the image read before erasing flash.
fread()is not checked for I/O failure or oversize input, so this path can erase the existing firmware and then write a partial/truncated image. Gate the erase behind read validation first.Proposed fix
bool preload_firmware_image(const char *filename) { static uint8_t fw_buffer[FIRMWARE_MAXSIZE]; FILE *file = fopen(filename, "rb"); if (!file) { printf("Failed to open file '%s'\n", filename); return false; } size_t read = fread(fw_buffer, 1, sizeof(fw_buffer), file); + bool read_error = ferror(file); + bool oversized = (read == sizeof(fw_buffer)) && !feof(file); fclose(file); + if (read_error || oversized || read == 0) { + printf("File '%s' does not contain a valid firmware image.\n", filename); + return false; + } + ensure(flash_area_erase(&FIRMWARE_AREA, NULL), NULL); return sectrue == flash_area_write_data_padded(&FIRMWARE_AREA, 0, fw_buffer, read, 0x0, FIRMWARE_MAXSIZE); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/projects/bootloader/emulator.c` around lines 103 - 114, The code currently calls flash_area_erase() before verifying the image read from fopen/fread, risking erasing firmware on partial/failed reads; change the flow in the block that opens filename/fread to: check fopen success, read into fw_buffer and validate the read result (use the returned size 'read', ensure it's >0, <= FIRMWARE_MAXSIZE, and check ferror(file) to detect I/O errors), close the file, and only if validation succeeds call flash_area_erase(&FIRMWARE_AREA, NULL) and then flash_area_write_data_padded(&FIRMWARE_AREA, 0, fw_buffer, read, 0x0, FIRMWARE_MAXSIZE) (and return appropriate boolean comparing to sectrue); do not erase the flash before these validations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@core/embed/projects/bootloader/emulator.c`:
- Around line 103-114: The code currently calls flash_area_erase() before
verifying the image read from fopen/fread, risking erasing firmware on
partial/failed reads; change the flow in the block that opens filename/fread to:
check fopen success, read into fw_buffer and validate the read result (use the
returned size 'read', ensure it's >0, <= FIRMWARE_MAXSIZE, and check
ferror(file) to detect I/O errors), close the file, and only if validation
succeeds call flash_area_erase(&FIRMWARE_AREA, NULL) and then
flash_area_write_data_padded(&FIRMWARE_AREA, 0, fw_buffer, read, 0x0,
FIRMWARE_MAXSIZE) (and return appropriate boolean comparing to sectrue); do not
erase the flash before these validations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25688d6a-2fa9-4420-b48a-5c2b6147a72b
📒 Files selected for processing (19)
core/SConscript.bootloader_emucore/SConscript.prodtest_emucore/SConscript.unixcore/embed/io/button/unix/button.ccore/embed/io/display/unix/display_driver.ccore/embed/io/power_manager/unix/power_manager.ccore/embed/io/touch/unix/touch.ccore/embed/models/T3W1/boards/t3w1-unix.hcore/embed/projects/bootloader/emulator.ccore/embed/projects/bootloader/main.ccore/embed/projects/prodtest/emulator.ccore/embed/projects/unix/main_main.ccore/embed/rust/build.rscore/embed/sys/task/inc/sys/unix/sdl_event.hcore/embed/sys/task/unix/sdl_event.clegacy/Makefile.includelegacy/emulator/buttons.clegacy/emulator/oled.cshell.nix
✅ Files skipped from review due to trivial changes (8)
- core/embed/models/T3W1/boards/t3w1-unix.h
- core/embed/sys/task/inc/sys/unix/sdl_event.h
- legacy/emulator/buttons.c
- core/embed/projects/bootloader/main.c
- legacy/Makefile.include
- core/SConscript.prodtest_emu
- core/SConscript.bootloader_emu
- shell.nix
🚧 Files skipped from review as they are similar to previous changes (8)
- core/embed/io/power_manager/unix/power_manager.c
- core/embed/io/button/unix/button.c
- core/embed/io/touch/unix/touch.c
- core/embed/projects/unix/main_main.c
- core/embed/rust/build.rs
- core/SConscript.unix
- legacy/emulator/oled.c
- core/embed/io/display/unix/display_driver.c
M1nd3r
force-pushed
the
m1nd3r/sdl3
branch
2 times, most recently
from
3bb8178 to
3f85f8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/embed/io/display/unix/display_driver.c (2)
148-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd null checks for framebuffer allocation.
Lines 148 and 152 can both return
NULLon failure, but the code immediately uses these pointers at line 155 and in subsequent operations (lines 456, 462) without validation. SDL3 documents both constructors as nullable on failure. This is inconsistent with the established pattern in this file (see lines 136–137 forSDL_CreateWindowand lines 141–143 forSDL_CreateRenderer).Suggested fix
drv->buffer = SDL_CreateSurface( DISPLAY_RESX, DISPLAY_RESY, SDL_GetPixelFormatForMasks(COLOR_DEPTH, COLOR_MASK_R, COLOR_MASK_G, COLOR_MASK_B, COLOR_MASK_A)); + if (drv->buffer == NULL) { + LOG_ERR("SDL_CreateSurface failed: %s", SDL_GetError()); + SDL_DestroyRenderer(drv->renderer); + SDL_DestroyWindow(drv->window); + error_shutdown("SDL_CreateSurface error"); + } drv->texture = SDL_CreateTexture(drv->renderer, PIXEL_FORMAT, SDL_TEXTUREACCESS_STREAMING, DISPLAY_RESX, DISPLAY_RESY); + if (drv->texture == NULL) { + LOG_ERR("SDL_CreateTexture failed: %s", SDL_GetError()); + SDL_DestroySurface(drv->buffer); + SDL_DestroyRenderer(drv->renderer); + SDL_DestroyWindow(drv->window); + error_shutdown("SDL_CreateTexture error"); + } SDL_SetTextureBlendMode(drv->texture, SDL_BLENDMODE_BLEND);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/io/display/unix/display_driver.c` around lines 148 - 155, The code must check for NULL returns from SDL_CreateSurface and SDL_CreateTexture: after calling SDL_CreateSurface (drv->buffer) and SDL_CreateTexture (drv->texture) validate each pointer, and on failure log an error via the existing logger (matching the pattern used for SDL_CreateWindow/SDL_CreateRenderer), free any already-created resources (e.g., if drv->buffer was created but drv->texture failed, free drv->buffer), and return/cleanup appropriately instead of proceeding to SDL_SetTextureBlendMode or later usage; reference the symbols SDL_CreateSurface, drv->buffer, SDL_CreateTexture, drv->texture, SDL_SetTextureBlendMode when making the checks and cleanup.
640-646:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChange texture access mode to SDL_TEXTUREACCESS_TARGET for render target use.
The suspend overlay texture is passed to
SDL_SetRenderTargeton line 646, which requires textures created withSDL_TEXTUREACCESS_TARGETin SDL3. UsingSDL_TEXTUREACCESS_STATICwill cause the operation to fail at runtime. Additionally, the current code lacks error handling for bothSDL_CreateTextureandSDL_SetRenderTargetcalls.Suggested fix
SDL_Texture *overlay = SDL_CreateTexture(drv->renderer, SDL_PIXELFORMAT_RGBA8888, - SDL_TEXTUREACCESS_STATIC, screen.w, screen.h); - SDL_SetTextureBlendMode(overlay, SDL_BLENDMODE_BLEND); - SDL_SetRenderTarget(drv->renderer, overlay); + SDL_TEXTUREACCESS_TARGET, screen.w, screen.h); + if (overlay == NULL) { + LOG_ERR("Failed to create suspend overlay texture: %s", SDL_GetError()); + return; + } + SDL_SetTextureBlendMode(overlay, SDL_BLENDMODE_BLEND); + if (!SDL_SetRenderTarget(drv->renderer, overlay)) { + LOG_ERR("SDL_SetRenderTarget failed: %s", SDL_GetError()); + SDL_DestroyTexture(overlay); + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/io/display/unix/display_driver.c` around lines 640 - 646, The suspend overlay texture is created with SDL_TEXTUREACCESS_STATIC but then used as a render target; change the SDL_CreateTexture call to use SDL_TEXTUREACCESS_TARGET, check for NULL return from SDL_CreateTexture and log/handle the error (e.g., free resources and abort creation) and then check the return value of SDL_SetRenderTarget for failure, logging the SDL error and cleaning up the overlay on failure; update the block around SDL_CreateTexture/SDL_SetTextureBlendMode/SDL_SetRenderTarget (using symbols overlay, drv->renderer, SDL_CreateTexture, SDL_SetTextureBlendMode, SDL_SetRenderTarget) to perform these checks and proper cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/embed/io/display/unix/display_driver.c`:
- Around line 238-239: The deinit path frees drv->prev_saved (and similarly at
the other cleanup site) but leaves the stale pointer in g_display_driver,
causing display_save() to treat it as valid; after calling SDL_DestroySurface on
drv->prev_saved (and on drv->buffer if freed there) set drv->prev_saved = NULL
(and drv->buffer = NULL where appropriate) in the display driver deinit/cleanup
function(s) so subsequent init→display_save() cycles won't memcmp freed memory;
reference g_display_driver, drv->prev_saved, drv->buffer and display_save() when
making the change.
---
Outside diff comments:
In `@core/embed/io/display/unix/display_driver.c`:
- Around line 148-155: The code must check for NULL returns from
SDL_CreateSurface and SDL_CreateTexture: after calling SDL_CreateSurface
(drv->buffer) and SDL_CreateTexture (drv->texture) validate each pointer, and on
failure log an error via the existing logger (matching the pattern used for
SDL_CreateWindow/SDL_CreateRenderer), free any already-created resources (e.g.,
if drv->buffer was created but drv->texture failed, free drv->buffer), and
return/cleanup appropriately instead of proceeding to SDL_SetTextureBlendMode or
later usage; reference the symbols SDL_CreateSurface, drv->buffer,
SDL_CreateTexture, drv->texture, SDL_SetTextureBlendMode when making the checks
and cleanup.
- Around line 640-646: The suspend overlay texture is created with
SDL_TEXTUREACCESS_STATIC but then used as a render target; change the
SDL_CreateTexture call to use SDL_TEXTUREACCESS_TARGET, check for NULL return
from SDL_CreateTexture and log/handle the error (e.g., free resources and abort
creation) and then check the return value of SDL_SetRenderTarget for failure,
logging the SDL error and cleaning up the overlay on failure; update the block
around SDL_CreateTexture/SDL_SetTextureBlendMode/SDL_SetRenderTarget (using
symbols overlay, drv->renderer, SDL_CreateTexture, SDL_SetTextureBlendMode,
SDL_SetRenderTarget) to perform these checks and proper cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a15b20e-4737-4f62-a2b8-e6c405fcb3fb
📒 Files selected for processing (20)
core/SConscript.bootloader_emucore/SConscript.prodtest_emucore/SConscript.unixcore/embed/io/button/unix/button.ccore/embed/io/display/unix/display_driver.ccore/embed/io/power_manager/unix/power_manager.ccore/embed/io/touch/unix/touch.ccore/embed/models/T3W1/boards/t3w1-unix.hcore/embed/projects/bootloader/emulator.ccore/embed/projects/bootloader/main.ccore/embed/projects/prodtest/emulator.ccore/embed/projects/unix/main_main.ccore/embed/rust/build.rscore/embed/sys/task/inc/sys/unix/sdl_event.hcore/embed/sys/task/unix/sdl_event.clegacy/Makefile.includelegacy/emulator/buttons.clegacy/emulator/oled.cshell.nixtests/ui_tests/fixtures.json
✅ Files skipped from review due to trivial changes (9)
- legacy/emulator/buttons.c
- core/embed/projects/bootloader/main.c
- core/embed/sys/task/inc/sys/unix/sdl_event.h
- core/embed/models/T3W1/boards/t3w1-unix.h
- core/embed/sys/task/unix/sdl_event.c
- core/embed/io/touch/unix/touch.c
- legacy/Makefile.include
- core/SConscript.bootloader_emu
- legacy/emulator/oled.c
🚧 Files skipped from review as they are similar to previous changes (5)
- core/embed/rust/build.rs
- core/SConscript.unix
- core/SConscript.prodtest_emu
- core/embed/projects/unix/main_main.c
- core/embed/projects/prodtest/emulator.c
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/embed/io/display/unix/display_driver.c (1)
152-173:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSDL3 texture scale mode changed to linear; likely root cause of fixture divergence — add explicit nearest-neighbor setting.
The PR notes that fixtures differ across nearly all models and SDL2 output cannot be reproduced under SDL3. The official SDL3 migration guide explicitly confirms the root cause: SDL3 changed the default texture scale mode from nearest-neighbor to linear filtering. Unlike SDL2 (which defaulted to
SDL_HINT_RENDER_SCALE_QUALITY=0), SDL3 textures now useSDL_SCALEMODE_LINEARby default, which alters virtually every output pixel—especially with rotation (SDL_RenderTextureRotatedat line 463) or non-integer scaling.To restore byte-for-byte parity with SDL2 fixtures, explicitly call
SDL_SetTextureScaleMode(texture, SDL_SCALEMODE_NEAREST)after creating each of the three textures (drv->texture,drv->background, anddrv->foreground). This should eliminate or dramatically reduce the fixture diff, avoiding the need to regenerate all model snapshots.🔧 Suggested changes
After line 156 (following
SDL_SetTextureBlendMode(drv->texture, ...)):SDL_SetTextureBlendMode(drv->texture, SDL_BLENDMODE_BLEND); + SDL_SetTextureScaleMode(drv->texture, SDL_SCALEMODE_NEAREST);Inside the existing
if (drv->foreground)block (lines 174–189):SDL_SetTextureBlendMode(drv->foreground, SDL_BLENDMODE_BLEND); + SDL_SetTextureScaleMode(drv->foreground, SDL_SCALEMODE_NEAREST);Inside the existing
if (drv->background)block (lines 190–193):SDL_SetTextureBlendMode(drv->background, SDL_BLENDMODE_NONE); + SDL_SetTextureScaleMode(drv->background, SDL_SCALEMODE_NEAREST);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/io/display/unix/display_driver.c` around lines 152 - 173, SDL3 defaults textures to linear filtering causing fixture diffs; after creating drv->texture, drv->background and drv->foreground call SDL_SetTextureScaleMode(...., SDL_SCALEMODE_NEAREST) to force nearest-neighbor filtering. Specifically, add SDL_SetTextureScaleMode(drv->texture, SDL_SCALEMODE_NEAREST) immediately after SDL_SetTextureBlendMode for drv->texture, and if drv->background/ drv->foreground are non-NULL call SDL_SetTextureScaleMode(drv->background, SDL_SCALEMODE_NEAREST) and SDL_SetTextureScaleMode(drv->foreground, SDL_SCALEMODE_NEAREST) respectively where those textures are assigned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@core/embed/io/display/unix/display_driver.c`:
- Around line 152-173: SDL3 defaults textures to linear filtering causing
fixture diffs; after creating drv->texture, drv->background and drv->foreground
call SDL_SetTextureScaleMode(...., SDL_SCALEMODE_NEAREST) to force
nearest-neighbor filtering. Specifically, add
SDL_SetTextureScaleMode(drv->texture, SDL_SCALEMODE_NEAREST) immediately after
SDL_SetTextureBlendMode for drv->texture, and if drv->background/
drv->foreground are non-NULL call SDL_SetTextureScaleMode(drv->background,
SDL_SCALEMODE_NEAREST) and SDL_SetTextureScaleMode(drv->foreground,
SDL_SCALEMODE_NEAREST) respectively where those textures are assigned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48d37b2f-6086-401a-9dbc-abd4393575f2
📒 Files selected for processing (20)
core/SConscript.bootloader_emucore/SConscript.prodtest_emucore/SConscript.unixcore/embed/io/button/unix/button.ccore/embed/io/display/unix/display_driver.ccore/embed/io/power_manager/unix/power_manager.ccore/embed/io/touch/unix/touch.ccore/embed/models/T3W1/boards/t3w1-unix.hcore/embed/projects/bootloader/emulator.ccore/embed/projects/bootloader/main.ccore/embed/projects/prodtest/emulator.ccore/embed/projects/unix/main_main.ccore/embed/rust/build.rscore/embed/sys/task/inc/sys/unix/sdl_event.hcore/embed/sys/task/unix/sdl_event.clegacy/Makefile.includelegacy/emulator/buttons.clegacy/emulator/oled.cshell.nixtests/ui_tests/fixtures.json
✅ Files skipped from review due to trivial changes (10)
- core/embed/sys/task/inc/sys/unix/sdl_event.h
- core/embed/models/T3W1/boards/t3w1-unix.h
- core/embed/projects/bootloader/main.c
- core/SConscript.prodtest_emu
- legacy/emulator/buttons.c
- core/SConscript.bootloader_emu
- core/embed/projects/bootloader/emulator.c
- core/embed/io/button/unix/button.c
- core/embed/projects/unix/main_main.c
- core/SConscript.unix
🚧 Files skipped from review as they are similar to previous changes (6)
- core/embed/projects/prodtest/emulator.c
- legacy/emulator/oled.c
- legacy/Makefile.include
- core/embed/io/power_manager/unix/power_manager.c
- core/embed/sys/task/unix/sdl_event.c
- core/embed/io/touch/unix/touch.c
cepetr
reviewed
May 7, 2026
cepetr
left a comment
cepetr
left a comment
Contributor
There was a problem hiding this comment.
I've successfully tested this PR on macOS. Everything seems to work fine.
mmilata
reviewed
May 7, 2026
Comment on lines
83
to
86
| # Current nixpkgs aliases SDL2 to sdl2-compat which on Ubuntu 25.04 makes the emulator | ||
| # crash with SDL_CreateRenderer error. | ||
| oldNixpkgs.SDL2 | ||
| oldNixpkgs.SDL2_image |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrating from SDL2 to SDL3 will allow us to drop oldNixpkgs from shell.nix
Note: Issue with UI tests - all fixtures on all models except T3W1 are changed. So far, I was not able to create fixtures with SDL3 that would be the same as fixtures made by SDL2.
Migration documentation: https://wiki.libsdl.org/SDL3/README-migration
Notes for QA