Three MainWindow methods exist solely to map an enum value to a
fixed string literal: getThemeString, getCurrentViewTypeString,
and getCurrentThumbnailTypeString. They returned QString built
from a string literal at every call. Returning the literal
directly as const char* is one less heap allocation per call and
the caller (QSettings::setValue, which takes QVariant) accepts
it via the implicit QString conversion either way.
* getThemeString: 4 lines -> 1 line per branch.
* getCurrentViewTypeString: switch over a 2-value enum collapses
to an if.
* getCurrentThumbnailTypeString: dropped a dead trailing
'return QString("list")' that the compiler couldn't reach -
the THUMBNAIL_TYPE_BOXART branch already returned via the
default label.
No behaviour change. All four object files build with zero
warnings.
Four small dead-code removals turned up in another sweep over
the audit areas. No behaviour change.
* TreeView::columnCountChanged: pure passthrough to
QTreeView::columnCountChanged. Without this override, Qt's
default does exactly the same thing. Drop the slot decl + impl.
* ListWidget::isEditorOpen: declared and defined, never called.
TableView has the same method and one caller; ListWidget had
none.
* AppHandler::onLastWindowClosed + the QApplication::lastWindowClosed
-> onLastWindowClosed connect: the slot body was empty and the
connection was therefore a no-op. Drop the slot, the connect()
call, and the now-empty 'private slots:' section in the header.
* ThumbnailLabel::resizeEvent: pure passthrough to
QWidget::resizeEvent, same situation as columnCountChanged.
All four object files (ui_qt, ui_qt_widgets, moc_ui_qt,
moc_ui_qt_widgets) build with zero warnings.
The Qt UI's playlist rows used to round-trip through
QHash<QString,QString> with magic string keys ('path', 'label',
'core_path', etc.). This commit replaces that with a typed
PlaylistEntry struct. The keys actually used at the call sites
collapsed cleanly into 9 fields plus a row index. Other
QHash<QString,QString> uses in the file (core-info display rows,
core-selection-dialog rows) follow a different schema and are
intentionally left alone.
* ui_qt_widgets.h: define PlaylistEntry. The struct lives in this
header rather than ui_qt.h because PlaylistEntryDialog has it
in a default argument and ui_qt_widgets.h is included by
ui_qt.h, not the other way round. Q_DECLARE_METATYPE and
qRegisterMetaType<PlaylistEntry>() enable QVariant transport.
* PlaylistModel: m_contents is now QVector<PlaylistEntry>.
getThumbnailPath(), data() (HASH role), and setData() all
consume the struct directly. The custom user role enum is
renamed HASH -> ENTRY to match.
* MainWindow:
- getCurrentContentHash() -> getCurrentContentEntry()
- getFileContentHash() -> getFileContentEntry()
- loadContent(QHash) -> loadContent(PlaylistEntry)
- onCurrentItemChanged(QHash) -> onCurrentItemChanged(PlaylistEntry)
- updateCurrentPlaylistEntry(QHash) -> (PlaylistEntry)
All call sites (setCoreActions, getSelectedCorePath, onRunClicked,
onCurrentTableItemDataChanged, onCurrentFileChanged,
onFileDoubleClicked, changeThumbnail, deleteCurrentPlaylistItem,
the playlist-context-menu handler, and the addFilesToPlaylist
flow) updated to read entry.field instead of hash["field"].
* PlaylistEntryDialog::showDialog and setEntryValues now take
const PlaylistEntry &. The 'multiple entries' detection that
used QHash::isEmpty() now uses entry.path.isEmpty(); previously
the addFilesToPlaylist() flow had to write two empty-string keys
just to make the QHash non-empty as a signal, which is gone.
* Drop the dead 'index' field round-trip.
updateCurrentPlaylistEntry() and deleteCurrentPlaylistItem() used
to read 'index' from the hash via QString::toUInt, which meant
PlaylistModel::addPlaylistItems() had to format every row's index
as a decimal string into the hash. PlaylistEntry::index is just
an unsigned now.
* Drop dead m_currentGridHash member from MainWindow. It was only
ever cleared, never written or read, so it always held an empty
QHash. Removed along with the related conversions.
No behaviour change. All four object files (ui_qt, ui_qt_widgets,
moc_ui_qt, moc_ui_qt_widgets) build with zero warnings.
Pure dead-code removal in three files. No behaviour change.
* ui_qt.h: drop 8 redundant forward declarations.
GridView, ShaderParamsDialog, CoreOptionsDialog, CoreInfoDialog,
PlaylistEntryDialog and ViewOptionsDialog all have full
definitions in ui_qt_widgets.h, which ui_qt.h includes at line
46 - the forward decls below the include were noise.
MainWindow and ThumbnailWidget were forward-declared at the top
of ui_qt.h but never referenced before their own definitions
later in the same file. Only the ThumbnailLabel forward decl is
load-bearing - ThumbnailWidget references ThumbnailLabel* as a
member before ThumbnailLabel is defined.
* MainWindow: drop 5 dead signals and 4 dead connect() calls.
- gotReloadCoreOptions, gotThumbnailDownload, updateThumbnails
and extractArchiveDeferred each had a connect() in the
constructor but were never emitted anywhere - the signal
connections were inert. The slots they pointed to
(onDownloadThumbnail, onGotReloadCoreOptions, onExtractArchive,
updateVisibleItems) all stay - they have other callers, either
via direct calls or via QMetaObject::invokeMethod.
- gridItemChanged was declared but neither emitted nor connected.
* MainWindow: drop 3 dead public getters.
contentTableView(), contentGridView() and searchWidget() were
declared and defined but had zero callers across ui/, libretro-
common/, frontend/, menu/, and retroarch.c.
* MainWindow::MainWindow: drop dead 'QDir playlistDir' and the
path_dir_playlist local that only existed to construct it.
Both unused since the constructor split.
-Wunused-variable did not flag the QDir because of its
non-trivial constructor.
* MainWindow::changeThumbnail: replace QDir.exists() / QDir.mkpath
pair with path_is_directory() / path_mkdir() from libretro-
common, dropping one more QDir local. mkpath(".") and
path_mkdir() both create parent directories recursively.
* MainWindow::setCoreActions: drop dead 'QFileInfo info' block
('info' was assigned via setFile() but never read) and a dead
'coreName = "<n/a>"' assignment in a then-branch that did
nothing else with coreName.
* qt_companion_select_initial_playlist: drop redundant
'listWidget->count() &&' guard from two for-loop conditions
('i < count' is already false when count is 0).
* ui_qt_widgets.cpp: drop pre-existing dead 'uint32_t flags =
runloop_st->flags' local in CoreOptionsDialog (-Wunused-variable
was suppressed in the build and so this slipped in years ago).
Reduces Qt-specific surface and silences three pre-existing
-Wdeprecated-declarations warnings. All changes preserve behaviour
on Qt 5.2 and up (RetroArch's minimum, per qb/config.libs.sh).
* MainWindow::MainWindow:
Drop dead 'QDir playlistDir(path_dir_playlist)' and the
path_dir_playlist local that only existed to construct it. Both
unused since the constructor split. -Wunused-variable did not
flag the QDir because of its non-trivial constructor.
* MainWindow::setCustomThemeFile:
Replace QFile open / readAll / close with filestream_read_file
plus filestream_exists. Same five error paths preserved
(blank path, missing file, open failure, empty file, success).
* MainWindow::onFileBrowserTreeContextMenuRequested:
Replace the QDir / dir.exists() existence check with
path_is_directory(). Removes a Qt5/Qt6 ifdef ladder ('dir =
string' on Qt5 vs 'dir.setPath(string)' on Qt6) and silences
the QDir::operator=(QString) deprecation warning. The
QDir::toNativeSeparators(...) display path stays - that side is
fine on both Qt 5 and Qt 6.
* LoadCoreWindow::onCoreEnterPressed:
Replace 'QByteArray::append(QString)' (Qt 5) /
'QByteArray::append(QString::toStdString())' (Qt 6) ladder with
'path.toUtf8().constData()' inline. The temporary QByteArray's
lifetime extends through the loadCore() call so this is safe.
Silences the QByteArray::append(QString) deprecation warning.
* MainWindow::onPlaylistWidgetContextMenuRequested:
Replace 'm_listWidget->isItemHidden(item)' (deprecated since
Qt 4.8) with 'item->isHidden()' (available since Qt 4.x).
Removes a Qt5/Qt6 ifdef ladder. Silences the
QListWidget::isItemHidden() deprecation warning.
Tested under Qt 5.15 with -Wdeprecated-declarations enabled; all
four object files (ui_qt, ui_qt_widgets, moc_ui_qt,
moc_ui_qt_widgets) now build with zero warnings.
No behaviour change.
* Drop three unused Qt includes:
- ui_qt.h: QDialog, QStyledItemDelegate. Both are still needed in
ui_qt_widgets.h (PlaylistEntryDialog inherits QDialog,
ThumbnailDelegate inherits QStyledItemDelegate), and ui_qt.h
pulls in ui_qt_widgets.h, so consumers continue to get them
transitively for free.
- ui_qt_widgets.h: QTabWidget. Not used in this header at all.
* Split MainWindow::MainWindow body into helpers. The constructor
was 446 body lines of mixed widget creation, layout assembly,
model setup, file-system configuration, dock setup, and signal
wiring, with locals declared at the top and used hundreds of
lines later. Five private methods now own the chunks that don't
cross-depend:
- setupPlaylistFooter() builds the zoom slider, view-type and
thumbnail-type push buttons, items count label, and grid
progress widget, attaching them under the playlist views.
- setupModels() configures the playlist + filesystem proxy
models and the table / file-table / grid views.
- setupFileSystemBrowser() configures the QFileSystemModels
(filters, root paths) and the directory tree view.
- setupDockWidgets() owns the search / core-info / log dock
widgets, including the searchResetButton that previously lived
as a constructor local.
- setupSignalConnections() owns the bulk of the QObject::connect
block plus the m_dirTree initial-selection and m_thumbnailTimer
setup that was interleaved with it.
Constructor body: 446 -> 101 lines.
* Three small file-static helpers collapse repeated boilerplate:
- qt_button_set_action_label() replaces the
setDefaultAction(new QAction(msg_hash_to_str(label), btn)) +
setFixedSize(sizeHint()) pair (5 sites).
- qt_dock_add_to() replaces the
addDockWidget(static_cast<Qt::DockWidgetArea>(
dock->property("default_area").toInt()), dock)
idiom (7 sites).
- qt_dock_configure() replaces a setObjectName / setProperty(
"default_area") / setProperty("menu_text") / setWidget
four-line block (6 sites).
No behaviour change.
The proxy model's filterAcceptsRow is called once per (row, parent)
pair the source QFileSystemModel surfaces, which can run into the
tens of thousands during a directory load, sort, or filter pass on
a populous tree. The original implementation resolved the source
model's root path to a QModelIndex via sm->index(sm->rootPath()) on
every single call - cheap individually, but it adds up.
The root path itself only changes when the user navigates, so cache
both the path and its resolved index on the proxy. On each call,
compare the cached path against the current one (a QString equality
check, much cheaper than re-walking the model); refresh on mismatch.
No behaviour change.
Selecting an image file in the file browser ran the PNG/JPEG decode
synchronously on the UI thread via image_texture_load. For ordinary
scraper-sized boxart this is fine, but for the multi-GiB ARGB32
bitmaps that come out of decoding a high-resolution upscayl-style
PNG the UI froze for hundreds of milliseconds to several seconds.
The same stall fired on every selection change, including the
implicit ones triggered by collapsing a tree node when the current
selection became invisible.
Add a ThumbnailLoader owned by MainWindow (parallel to the one
PlaylistModel owns for the grid view) and route the preview-pane
decode through it. The panes are cleared immediately on selection
change; the loaded pixmap is installed when the worker's
imageLoaded signal fires. A pending-path token guards against rapid
selection changes flickering stale images in.
Playlist-thumbnail (scraper boxart) decoding stays synchronous
because those images are bounded in size; the only change there is
that m_pendingPreviewPath is cleared on entry so a previously-issued
preview request can't land in the panes after the playlist code
already painted boxart there.
Refactor pass on ui/drivers/ui_qt.{cpp,h} and ui_qt_widgets.{cpp,h}
to reduce Qt-specific surface, lift pure-C work out of C++ syntax,
and use libretro-common idioms in place of locally-rolled equivalents.
ui/drivers/ui_qt.cpp 5620 -> 5462 (-158)
ui/drivers/ui_qt_widgets.cpp 8888 -> 8783 (-105)
ui/drivers/ui_qt.h 761 -> 747 ( -14)
ui/drivers/ui_qt_widgets.h 1442 -> 1381 ( -61)
-----
TOTAL 16711 -> 16373 (-338)
No functional change. Build verified for ui_qt.o, ui_qt_widgets.o,
moc_ui_qt.o and moc_ui_qt_widgets.o; the only warnings emitted are
the same three pre-existing -Wdeprecated-declarations warnings the
original tree emits (QDir::operator=(const QString&),
QByteArray::append(const QString&), QListWidget::isItemHidden).
Iterative-build wall clock on the four targets drops ~9%
(28.1s -> 25.6s, hyperfine n=5), MOC-generated .cpp drops ~9%
(8666 -> 7874 lines), and total object size drops ~4%
(2.05 MiB -> 1.97 MiB).
Changes:
* Drop string_split_to_qt and use QString::split. The function was
declared inside RETRO_BEGIN_DECLS - i.e. with C linkage - but
returned QStringList, which is not a C-compatible type. The four
call sites become QString::split('|') / .split(' ').
* Replace MainWindow::getScrubbedString with a static scrub_qstring()
helper using string_replace_all_chars from libretro-common. Moves
the function out of MOC (one fewer slot to generate).
* Collapse 11 trivial OptionsPage subclasses (MenuSoundsPage,
UpdaterPage, QuickMenuPage, DriversPage, DirectoryPage,
ConfigurationPage, CorePage, LoggingPage, AIServicePage,
FrameThrottlePage, RewindPage) into one runtime-parameterised
SimplePage class. Each was a do-nothing constructor plus a
one-line widget() returning create_widget(DISPLAYLIST_X). The 12
instantiation sites in *Category::pages() become
new SimplePage(DISPLAYLIST_X, MENU_ENUM_LABEL_VALUE_X, this).
* Extract MainWindow::getCoreInfo() into a C helper qt_core_info_collect
that emits two parallel struct string_list outputs (keys, values)
with a status code (enum qt_core_info_row_status) packed into
values->elems[i].attr.i. The walking logic is now pure C using
libretro-common idioms (string_list_*, core_info_*,
fill_pathname_basedir, string_count_occurrences_single_character);
the C++ wrapper is a ~50-line loop applying status-driven HTML/CSS
styling for firmware rows. Both consumers
(onLaunchWithComboBoxIndexChanged, CoreInfoDialog::showCoreInfo)
work unchanged.
* Replace four parallel thumbnail paths with arrays. The
m_thumbnailPixmap{,2,3,4} pointers, onResizeThumbnail{One,Two,
Three,Four} slots, and thumbnailChanged{,2,3,4} signals are
consolidated into m_thumbnailPixmaps[4] and three static tables.
The four thumbnailChanged* signals were never emitted and never
connected anywhere - deleted. The four onResizeThumbnail* slots
were one-line wrappers around setThumbnail("thumbnailN", ...) -
deleted, callers use setThumbnail directly. Destructor,
onCurrentItemChanged and onThumbnailDropped collapse to short
loops; an explicit ThumbnailType -> widget-index mapping handles
the asymmetric ordering between the enum (BOXART, SCREENSHOT,
TITLE_SCREEN, LOGO) and the 1..4 widget naming (boxart, title,
screenshot, logo).
* Convert MainWindow::getSelectedCore (which returned
QHash<QString,QString> but only ever set a single field) to
getSelectedCorePath returning QString. Sole caller updated.
* Replace QFileInfo/QDir/QFile usage with file_path.h /
file_stream.h equivalents in three high-leverage spots:
- renamePlaylistItem: now uses path_basename, path_basedir,
path_get_extension, fill_pathname_slash,
string_is_equal_case_insensitive, and filestream_rename.
- onExtractArchive's per-entry delete-or-rename loop: now uses
filestream_exists, filestream_delete, filestream_rename. Also
fixes a pre-existing leak: the original code returned -1 on
three error paths without freeing file_list.
- loadContent's extension-filter block: collapses lastIndexOf('.')
+ .mid() onto path_get_extension, which the same block was
already using three lines below.
- Three QFileInfo(pathData).suffix() callers in
add-files-to-playlist code: replaced with
QString::fromUtf8(path_get_extension(pathData)).
* Split ui_companion_qt_init from a 516-line monolith into six
focused static helpers (qt_companion_build_menubar,
qt_companion_build_browser_dock, qt_companion_build_thumbnail_docks,
qt_companion_build_core_selection_dock, qt_companion_restore_settings,
qt_companion_select_initial_playlist) plus a ~75-line orchestrator.
Behaviour preserved verbatim. The thumbnail-dock setup that was
65 lines of 4-way copy-paste collapses to a 30-line loop using
qt_thumbnail_widget_names from above.
Three -Wformat= warnings surface on i386 Linux where size_t is unsigned
int and uint64_t is unsigned long long (both differ from x86_64, where
the existing specifiers happen to match):
display_linux.cpp:166 %ld on vector::size_type (size_t)
custom_video_xrandr.cpp:679 %lx on uint64_t platform_data
custom_video_xrandr.cpp:685 %lx on uint64_t platform_data
The size_t case is signedness-only and benign in practice; switch to
%zu, the C99 specifier intended for size_t. The uint64_t cases are an
actual 32-bit truncation bug -- %lx reads four bytes off the stack on
i386 cdecl, dropping the high 32 bits of platform_data and misaligning
any following format arguments. Use %llx with an explicit
(unsigned long long) cast, matching the raw-printf idiom Switchres
uses everywhere else (no <inttypes.h> / PRIx64 elsewhere in the tree).
deps/switchres/ is a verbatim mirror of antonioginer/switchres; this
fix should also be sent upstream so the next sync doesn't reintroduce
the warnings.
When HDR is enabled and the active shader chain doesn't emit HDR
natively, vulkan_frame() routes overlay-style content (menu, on-screen
messages, widgets, software overlays) through an offscreen SDR
buffer, then composites it back over the HDR swapchain image. The
gating condition for that detour listed every overlay source except
the statistics overlay.
The result: with HDR on, a non-HDR-emitting shader chain loaded, and
the statistics overlay enabled (with no menu/message/widget visible),
the main render pass was ended after the HDR composite but never
re-begun. The subsequent font_driver_render_msg() call for the stat
text recorded draw commands against a command buffer with no active
render pass.
Most desktop Vulkan drivers tolerate this with a validation error;
MoltenVK on macOS dereferences a null subpass in
MVKCommandEncoder::getSubpass() during vkCmdDrawIndexed encoding and
crashes with SIGSEGV. The HDR-native shaders under slang_shaders/hdr
were unaffected because they avoid the offscreen-buffer detour
entirely (use_offscreen_buffer stays false when the chain emits HDR10
or HDR16).
Add statistics_show to both gating conditions so the begin-redirect
and the matching post-blit composite stay balanced.
ui_qt.cpp wraps RetroArch headers in extern "C" { ... } when CXX_BUILD
is not defined. In non-unity builds the chain
ui_qt.cpp (extern "C") -> menu_driver.h -> gfx_thumbnail.h
-> retro_atomic.h -> <atomic>
ends up parsing libstdc++ <atomic> under C linkage, producing dozens of
"template with C linkage" errors on g++. The same hazard exists for any
C++ TU that wraps RetroArch headers in extern "C", and is currently
masked elsewhere only because no other such TU happens to pull in a
header that reaches retro_atomic.h.
Fix at the header level so every caller is protected, not just ui_qt.cpp:
- Add RETRO_BEGIN_DECLS_CXX / RETRO_END_DECLS_CXX to retro_common_api.h.
They expand to extern "C++" { ... } in C++ mode (with CXX_BUILD off)
and to nothing otherwise, mirroring the existing RETRO_BEGIN_DECLS /
RETRO_END_DECLS pair.
- Use them around the <atomic> / <cstddef> include in retro_atomic.h's
C++11 backend. The standard headers parse at C++ linkage regardless
of any surrounding extern "C" the caller imposes; the rest of the
file (typedefs and macros, no function declarations) is unaffected.
The previous comment claiming "no need for RETRO_BEGIN_DECLS / extern
\"C\" wrapping ... the wrapper would actively break that path" addressed
only half the problem -- the file's own wrapping -- and missed that the
caller's wrapper leaks into the <atomic> include. Updated to reflect
the real constraint.
The comment added in 276e4bc said gl3 'only ever advertises slang via
spirv-cross', which overstated things. Context drivers do only
advertise SLANG for glcore, but the gl3 fallback selector itself
accepts both GLSL and SLANG as first-class types -- and 42aa9fd just
made GLSL a valid GL3_DEFAULT_SHADER_TYPE on builds without slang.
Reword the comment to be specific about what is and isn't dead:
- Cg is dead (no context flag, ARB asm pipeline incompatible with
Core Profile)
- GLSL is alive (Core-compatible API surface, kept as fallback for
no-slang builds)
- Slang is the practical default
No code change.
The gl3 (glcore video driver) shader fallback selector and dispatcher
both had RARCH_SHADER_CG arms, but those branches were unreachable
in any build configuration:
1. The Cg fallback in gl3_get_fallback_shader_type gates on
GFX_CTX_FLAGS_SHADERS_CG. No context driver ever sets that
flag when the active video driver is "glcore" -- audited
x_ctx.c:1106-1116, wgl_ctx.c:778-788, drm_ctx.c, drm_go2_ctx.c,
orbis_ctx.c, switch_ctx.c, wayland_ctx.c, xegl_ctx.c (eight
context drivers, all of which the glcore branch advertises only
SHADERS_SLANG). The Cg flag is only set by ps3_ctx (which uses
the rsx driver, not glcore) and by the legacy / non-Core branch
of x_ctx and wgl_ctx (gated on !core_hw_context_enable, so only
for the gl driver, not glcore).
2. Even if the gating were bypassed, gl_cg_backend in
shader_gl_cg.c uses cgGLEnableProfile / cgGLBindProgram /
cgGLEnableClientState et al. These rely on the legacy ARB
asm program pipeline (GL_VERTEX_PROGRAM_ARB style) which is
removed from Core Profile contexts. NVIDIA's Cg runtime hasn't
been updated since 2012 and has no Core Profile support.
Remove:
- the RARCH_SHADER_CG case in gl3_get_fallback_shader_type
- the RARCH_SHADER_CG case in gl3_shader_driver_set_backend
- HAVE_CG from the function-level guard
- the now-unused 3rd loop iteration (the cycle is GLSL <-> SLANG)
The fallback chain becomes a 2-element cycle: GLSL falls back to
SLANG and vice versa, matching what the gating actually allows.
Add a comment at the top of gl3_get_fallback_shader_type explaining
the gl3-vs-Cg situation so future readers don't reintroduce the dead
arm. No behaviour change on any build: HAVE_CG was defined for some
Windows builds, but the gates above already prevented those branches
from executing.
Two related fixes for the GameMode startup behavior on Linux:
1. configuration.c: Only call frontend_driver_set_gamemode() during config
load when gamemode_enable is actually true. Previously the probe ran
unconditionally with the current setting value, meaning every user on
every config load triggered a dlopen of libgamemode.so even when the
feature was off - producing log spam ("[GameMode] GameMode cannot be
enabled on this system...") for the majority of users who don't have
libgamemode installed.
Also stop force-disabling gamemode_enable in the config when the probe
fails. Silently flipping the user's preference to false and persisting
it to retroarch.cfg meant a transient failure (sandbox path issue,
library not yet installed, multi-arch mismatch, etc.) would require
the user to manually re-enable the option on every subsequent launch.
The user's intent should survive across sessions; if libgamemode shows
up later, the next launch picks it up automatically.
2. frontend/drivers/platform_unix.c: Latch the unavailable state inside
frontend_unix_set_gamemode() once gamemode_query_status() reports
failure. The result will not change for the lifetime of the process,
so subsequent calls (deinit cleanup, menu toggles) short-circuit
without re-entering the Feral client code and without re-emitting the
warning. The warning now fires exactly once per process when the user
has gamemode enabled but libgamemode is missing.
The redundant "[Config] GameMode unsupported - disabling..." follow-up
warning is removed - it added no information beyond what the GameMode
warning already conveys.
* Add files via upload
* Update crop_borders.c
* Add EMMIR NTSC default "crt" variant
compiles and runs perfectly fine on linux Mint.
* Add files via upload
The MMX fast path of conv_rgba4444_argb8888 hardcoded the alpha
channel to 0xff instead of extracting it from the source's low
nibble. The scalar fallback (lines 488-501) correctly does
'a = (col >> 0) & 0xf; a = (a << 4) | a' to expand the 4-bit
source alpha into 8 bits. The MMX kernel kept the
'const __m64 a = _mm_set1_pi16(0x00ff)' constant from the
sibling conv_rgb565_argb8888 / conv_0rgb1555_argb8888 functions
(whose 16-bit sources have no alpha channel), so on x86 hosts
where __MMX__ is defined (which includes all x86_64 builds, since
gcc enables MMX by default alongside SSE2) RGBA4444 input was
silently converted with full opacity regardless of source alpha.
Same shape of bug as commit 9403719 (conv_rgb565_abgr8888 SSE2):
SIMD kernel inherited from a sibling without adapting all the
needed steps to the new format.
Fix: extract alpha from the low nibble (in & 0x000f), expand to
8 bits via _mm_mullo_pi16(a, 0x0011) which computes a * 17 ==
(a << 4) | a for any 4-bit value.
Verified bit-exact against the scalar fallback for every possible
RGBA4444 input. Brute-force test of all 65,536 16-bit values:
before fix: 61,440 of 65,536 outputs differ from scalar (94%;
all inputs whose low nibble != 0xf produce wrong
alpha)
after fix: 0 of 65,536 differ
Caller audit: hit by gfx/drivers/gl1.c:1810 (RGUI menu fallback
expansion when GL_EXT_packed_pixels is unsupported -- triggers on
ancient OpenGL implementations and the Vita gl1 build) and via
the scaler chain when SCALER_FMT_RGBA4444 is paired with
SCALER_FMT_ARGB8888 (libretro-common/gfx/scaler/scaler.c:183 and
:232). On x86_64 the MMX path is hot in both contexts.
Companion to 7e44a5e (which covered B5G6R5 -> R8G8B8A8 with
conv_rgb565_abgr8888). The B8G8R8A8_UNORM destination has byte
order [b g r a] in memory, which is exactly what pixconv's
conv_rgb565_argb8888 produces (uint32 LE
(a << 24) | (r << 16) | (g << 8) | b). Bit-identical output to
the existing scalar loop's
(r << 16) | (g << 8) | b | (255 << 24).
Same speedup characteristics as the abgr8888 case: ~2x at sub-4K
resolutions, ~1.4x at 4K. Sub-millisecond absolute savings at
typical emulator resolutions.
Other dxgi_copy pairs considered but skipped:
- B5G6R5 -> B8G8R8X8: pixconv conv_rgb565_argb8888 puts 0xff
in the X byte where the existing scalar writes 0; per-spec
'X' means undefined, but preserving bit-for-bit existing
behavior avoids a subtle change.
- R8G8B8A8 <-> B8G8R8A8 byte-swap pairs: pixconv's
conv_argb8888_abgr8888 is scalar only (3-op bit-twiddle).
Benchmarked at parity with the dxgi loop after compiler
auto-vectorization (~0.9x to 1.1x, within noise). No perf
win; code consolidation alone insufficient justification.
dxgi_copy's scalar loop for the B5G6R5_UNORM -> R8G8B8A8_UNORM
format pair is exactly conv_rgb565_abgr8888 in pixconv.c. The
output uint32 (a << 24) | (b << 16) | (g << 8) | r matches DXGI
R8G8B8A8_UNORM byte order [r g b a] and matches pixconv's abgr8888
output verbatim.
Replace the open-coded scalar loop with a call into pixconv,
removing one of dxgi_copy's many hand-rolled per-pixel converters
in favor of the canonical SIMD-aware implementation.
Code-consolidation patch with measured speedup as a side benefit:
Resolution scalar (ms/frame) pixconv SSE2 (ms/frame) speedup
256x224 0.03 0.02 2.3x
640x480 0.19 0.09 2.1x
1280x720 0.59 0.27 2.2x
1920x1080 1.37 0.69 2.0x
3840x2160 6.5 4.7 1.4x
(x86_64 Linux, gcc -O3 -msse2, average of 3 runs.) Absolute
savings sub-millisecond at typical emulator resolutions; modestly
helpful at 1080p/4K. ARM/NEON path produces equivalent output.
Other dxgi_copy format-pair branches have less clean pixconv
matches (e.g. B5G6R5 -> B8G8R8X8 nearly matches conv_rgb565_argb8888
but pixconv writes 0xff in the X byte where the existing scalar
writes 0; preserving the existing behavior bit-for-bit would
require a new pixconv variant, so leaving those alone for now).
The SSE2 fast path of conv_rgb565_abgr8888 produced ARGB byte order
in memory ([b g r a] per pixel), not ABGR ([r g b a]). The scalar
fallback (lines 387-396) and the NEON fast path (lines 369-385)
both produce the correct ABGR byte order. On x86 hosts with SSE2
the function silently returned a different byte layout than its
name promised and than its scalar fallback delivered.
Root cause: the SSE2 kernel at lines 358-365 was a literal copy of
the conv_rgb565_argb8888 SSE2 kernel at lines 247-255 (the two
were byte-for-byte identical). Both pair (b,g) into the low half
of each pixel and (r,a) into the high half shifted up 2 bytes,
producing [b g r a] byte order which is ARGB uint32 LE. That's
correct for argb8888 but wrong for abgr8888.
Fix: swap the unpack pairings so (r,g) goes into the low half and
(b,a) into the high half shifted up 2 bytes, producing [r g b a]
byte order == ABGR uint32 LE. Same instruction count (4 unpacks +
1 shift + 2 ors), same vector width (8 pixels per iteration), no
perf change.
Verified bit-exact against the scalar fallback for every possible
RGB565 input. Brute-force test of all 65,536 16-bit values:
before fix: 63,488 of 65,536 outputs differ from scalar (97%)
after fix: 0 of 65,536 differ
For pure red input 0xF800:
scalar: 0xFF0000FF (ABGR uint32, R in low byte)
SSE before: 0xFFFF0000 (ARGB uint32, R in second byte)
SSE after: 0xFF0000FF (matches scalar)
Caller audit: of the production sites that route to pixconv via
the SCALER_FMT_ABGR8888 dispatch in scaler.c:126, none are
currently observed to hit the RGB565 -> ABGR8888 pair on an x86
host running SSE2. vulkan.c and gl3.c readback paths use ABGR8888
-> BGR24 (different pair). switch_nx_gfx is ARM64 (NEON, not
affected). pipewire camera input could in principle hit it if a
camera produces RGB565 output, but modern webcams typically
produce YUYV / MJPEG. The bug has been hidden by absence of
production callers; this fix prevents future contributors from
inheriting it.
Two related changes to the VGA framebuffer driver, applying the
same pattern as the gl1 / fpga / gdi / network / sixel / caca
sequence (d654940 / 61ce292 / e675181 / 4668a2f / ad986a6) plus
the bug fix that the caca work surfaced.
Rename
------
The four vga_video_* fields on vga_t hold the libretro core's
emulated frame size: assigned from video->width / video->height
at init (line 201-202), updated from frame_width / frame_height
per-call (line 248-258), and consumed by the per-pixel scaling
loops at line 303-304 / 327-328 which scale the core's frame
down to fit the actual VGA framebuffer (VGA_WIDTH x VGA_HEIGHT =
320x200).
Rename, dropping the redundant vga_ prefix while we're at it
(the struct is already called vga_t):
vga_video_width -> frame_width
vga_video_height -> frame_height
vga_video_pitch -> frame_pitch
vga_video_bits -> frame_bits
The renamed fields parallel vga_t's existing vga_menu_* group:
both are "input frame from $source", just one is the menu
surface and the other is the running core.
Bug fix
-------
vga_gfx_alive called video_driver_set_output_size with the
renamed frame_width / frame_height -- the libretro core's frame
size, e.g. 256x224 for SNES. Per the docstring added in cecd19f,
set_output_size publishes the area where the active video driver
presents pixels. For this driver that area is the VGA hardware
framebuffer at VGA_WIDTH x VGA_HEIGHT (320x200, mode 13h). Lines
303-304 and 327-328 already scale the core's frame down to fit
it; those constants are the actual output area. Pass them to
set_output_size instead.
The bug was hidden behind the misleading old field name: when
the field was called vga_video_width and held the core's frame
size, the video_driver_set_output_size(vga->vga_video_width, ...)
call read like it made sense. After the rename, the same call
reads as video_driver_set_output_size(vga->frame_width, ...) --
visibly wrong at the call site, exactly the class of confusion
the rename pattern was meant to surface.
The existing TODO/FIXME comment immediately above the call --
"check if this is valid" -- was correctly placed; the original
author knew something was off. Comment dropped, it's resolved.
Same shape as the caca fix in ad986a6.
Risk: if any caller of video_driver_get_output_size on vga was
relying on seeing the core's frame size (the old buggy value),
they will now see VGA_WIDTH x VGA_HEIGHT instead. No such
caller is plausible: the API contract is the output area, and
the menu drivers / input drivers were getting nonsense input
under the old behaviour (a 256x224 output area on a 320x200
framebuffer would draw widgets clipped or off-screen). This is
a strict bug fix in the direction of the documented contract.
Incidentally fixes a stale comment in sdl2_gfx.c at line 258
that listed vga in "Most other drivers (vga, gx2, d3d8, d3d9
common) make this call too" -- before this commit, vga was
making the call but not correctly; after, the comment is true.
Same risk profile as the rest of the rename pattern: vga_t is
file-private (declared at the top of vga_gfx.c with no header
counterpart, verified by grep across the source tree). No
external callers besides the comment fix in sdl2_gfx.c.
video_driver_get_size and video_driver_set_size publish the size of
the area where the active video driver presents pixels to the user.
The old name says "size" without saying "size of what," and the
ambiguity has cost: the caca driver bug fixed in ad986a6 was hidden
behind exactly this confusion -- caca was passing the libretro
core's frame size to set_size when the API contract was the canvas
(surface) size, and the call site looked plausible until the field
was renamed to expose what it actually held.
Rename the public API to make the contract explicit at the call
site, and add the documentation comment that should have been on
these declarations from the start:
video_driver_get_size -> video_driver_get_output_size
video_driver_set_size -> video_driver_set_output_size
"output_size" was chosen over the alternatives:
- window_size is wrong on terminal drivers (caca, sixel) and
DRM-fullscreen-no-window
- surface_size collides with SDL_Surface and D3D surface concepts
- display_size implies the physical monitor, not the output area
- present_size is jargon and reads oddly as a noun
- viewport_size collides with the existing vp.{width,height}
aspect-corrected sub-rect
"output_size" describes purpose (size of the visible output area)
without implying mechanism, doesn't collide with existing terms,
and maps cleanly onto every driver: desktop window output, DRM
scanout output, libcaca canvas output, sixel terminal output.
The doc comment in gfx/video_driver.h spells out the per-driver
mapping (window client area for desktop GL/D3D/Vulkan/Metal,
framebuffer scanout for DRM/KMS, canvas grid for libcaca,
terminal output region for sixel), the consumer list (menu
drivers, CRT switcher, shader subsystem, input drivers, other
video drivers), threading (protected by video_st->display_lock),
and explicitly contrasts with the libretro core's frame size that
lives on each driver's local state (frame_width / frame_height
after the gl1 / fpga / gdi / network / sixel / caca renames; gl2
/ gl3 still keep their canonical video_width / video_height which
mean the backbuffer size).
74 call sites across 33 files updated by word-boundary sed; no
collisions (no other identifiers contain video_driver_get_size or
_set_size as a substring).
Validation: the file with the function definitions
(gfx/video_driver.o) and a representative sample of caller files
(menu/drivers/{xmb,ozone,materialui}.o, gfx/drivers/gl{1,2,3}.o,
gfx/video_crt_switch.o, gfx/video_shader_parse.o) all build clean
in this sandbox. The remaining 24 files hit unrelated
dependency errors (windows.h not on Linux, libcaca / libvg
headers not installed) before reaching anything rename-related;
none produced a rename-related diagnostic. Maintainer review on
a full build environment recommended before merge.
No behavior change. Pure rename + documentation.
