Conversation
…g and cursor management
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…s for GUI rendering
There was a problem hiding this comment.
Pull Request Overview
This PR implements double buffering support for the framebuffer system to eliminate visual artifacts during rendering. The implementation adds buffer management infrastructure, updates drawing APIs to support targeting specific buffers, and integrates present() calls throughout the GUI rendering pipeline to synchronize buffer swaps.
Key changes:
- Added
BufferTargetenum and double buffering functions (present(),double_buffering_enabled()) to the framebuffer API - Updated all drawing functions to accept
BufferTargetparameters with default values favoring the draw buffer - Enhanced window system to support resizable windows with proper dimension tracking and validation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| include/kernel/framebuffer.h | Introduced BufferTarget enum and updated function signatures for buffer-aware drawing |
| src/kernel/framebuffer.cpp | Core double buffering implementation with buffer management, swap logic, and optimized drawing routines |
| src/kernel/vga.cpp | Integrated present() calls in terminal rendering and updated cursor drawing to use Display buffer |
| src/kernel/gui.cpp | Updated mouse cursor and boot screen rendering to call present() after drawing operations |
| src/kernel/kernel.cpp | Added debug logging for framebuffer initialization status and double buffering availability |
| src/kernel/terminal_windows.cpp | Implemented window resizing functionality with per-window dimension tracking and proper frame clamping |
| src/kernel/debug.cpp | Added present() call after panic screen rendering |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| void ensure_geometry(Terminal &terminal) | ||
| { | ||
| (void)terminal; |
There was a problem hiding this comment.
The terminal parameter is explicitly marked as unused but this function was previously using it. Since geometry calculations for windows no longer depend on a specific terminal instance, consider removing the parameter entirely from ensure_geometry() and updating all call sites to improve API clarity.
| void ensure_geometry(Terminal &terminal) | |
| { | |
| (void)terminal; | |
| void ensure_geometry() | |
| { |
| if (g_bytes_per_pixel == 4) | ||
| { | ||
| uint8_t *row_ptr = base + (y + row) * row_stride + x * g_bytes_per_pixel; | ||
| for (uint32_t col = 0; col < width; ++col) | ||
| for (uint32_t row = 0; row < height; ++row) | ||
| { | ||
| uint8_t *pixel = row_ptr + col * g_bytes_per_pixel; | ||
| switch (g_bytes_per_pixel) | ||
| uint8_t *row_base = base + (y + row) * row_stride + x * pixel_stride; | ||
| uint32_t *dst = reinterpret_cast<uint32_t *>(row_base); | ||
| for (uint32_t col = 0; col < width; ++col) | ||
| { | ||
| case 4: | ||
| { | ||
| uint32_t value = color; | ||
| memcpy(pixel, &value, sizeof(uint32_t)); | ||
| break; | ||
| } | ||
| case 3: | ||
| { | ||
| pixel[0] = static_cast<uint8_t>(color & 0xFF); | ||
| pixel[1] = static_cast<uint8_t>((color >> 8) & 0xFF); | ||
| pixel[2] = static_cast<uint8_t>((color >> 16) & 0xFF); | ||
| break; | ||
| } | ||
| case 2: | ||
| { | ||
| uint16_t value = static_cast<uint16_t>(color & 0xFFFF); | ||
| memcpy(pixel, &value, sizeof(uint16_t)); | ||
| break; | ||
| } | ||
| default: | ||
| break; | ||
| dst[col] = color; | ||
| } | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
The fast path for 4-byte pixels uses pointer arithmetic that could cause unaligned memory access if row_base is not 4-byte aligned. Consider adding an alignment check or using memset for single-color fills: for (uint32_t row = 0; row < height; ++row) { uint32_t *dst = reinterpret_cast<uint32_t *>(base + (y + row) * row_stride + x * pixel_stride); if (reinterpret_cast<uintptr_t>(dst) % 4 == 0) { /* fast path */ } else { /* fallback */ } }
Framebuffer Double Buffering Core Implementation
BufferTargetenum to distinguish between draw and display buffers, and added functions for double buffering management such aspresent(),double_buffering_enabled(), and internal buffer handling logic inframebuffer.cppandframebuffer.h.Framebuffer Drawing APIs Update
fill_rect,draw_mono_bitmap, andpeek_pixelto accept aBufferTargetparameter, enabling drawing to either the draw or display buffer. Internal drawing logic was refactored for efficiency and correctness with double buffering.Integration with GUI and Terminal Components
Modified GUI and terminal rendering functions to use the new double buffering APIs and call
present()appropriately after drawing operations, ensuring rendered output is displayed only after all drawing is complete.Initialization and Debug Logging
Enhanced initialization routines to attempt enabling double buffering if supported, and added debug logging to report framebuffer status and double buffering availability at boot.
Internal Refactoring and Robustness
Refactored internal framebuffer code for buffer management, memory access, and color storage, improving robustness and maintainability. Added safety checks for buffer boundaries and drawing operations.