Skip to content

Conversation

@CarterLi
Copy link
Member

No description provided.

CarterLi and others added 30 commits October 18, 2025 18:38
@CarterLi CarterLi requested a review from Copilot November 11, 2025 03:03
Copilot finished reviewing on behalf of CarterLi November 11, 2025 03:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This release (v2.55.0) introduces parallel command execution by default to improve performance, refines disk detection by moving folder/filesystem filtering to the detection stage, and adds support for using media cover art as logos. It also implements native GPU detection for OpenBSD and NetBSD without requiring libpciaccess.

  • Parallel command execution is now enabled by default with a new queue-based implementation
  • Media cover art can be used as logo source with support for local files
  • GPU detection improvements including native BSD support and refined device ID handling

Reviewed Changes

Copilot reviewed 69 out of 69 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/modules/media/media.c Adds cover field to JSON output but contains a bug copying wrong field
src/detection/command/command.c New file implementing parallel command execution queue
src/common/processing.h Refactors process spawning to separate spawn and read operations
src/common/processing_linux.c Splits process handling into spawn and read functions
src/common/processing_windows.c Implements async process handling with thread-safety concern
src/detection/gpu/gpu.h Adds helper functions for GPU device ID standardization
src/detection/gpu/gpu_*.c Updates GPU detection across platforms to use new device ID format
src/detection/disk/disk*.c Moves folder/filesystem filtering to detection stage for performance
src/logo/logo.c Adds media cover art logo support with potential performance issue
src/logo/builtin.c Adds MacaroniOS and VincentOS logos with minor formatting issue
CMakeLists.txt Updates version to 2.55.0 and refines library configuration
debian/publish.sh Complete rewrite with improved safety and multi-distribution support

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

.colors = {
FF_COLOR_FG_YELLOW,
FF_COLOR_FG_WHITE,
},
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation: tabs are used instead of spaces. This file appears to use spaces for indentation elsewhere.

Copilot uses AI. Check for mistakes.
src/logo/logo.c Outdated
Comment on lines 498 to 504
if (ffStrbufIgnCaseEqualS(&options->source, "mediacover"))
{
const FFMediaResult* media = ffDetectMedia(true);
if (media->cover.length == 0)
return false;
ffStrbufSet(&options->source, &media->cover);
if (media->removeCoverAfterUse) atexit(removeMediaCoverFile);
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling ffDetectMedia(true) twice may be inefficient. The first call at line 500 already retrieves the media result, which includes the removeCoverAfterUse flag. Consider storing the result and reusing it to avoid redundant detection.

Copilot uses AI. Check for mistakes.
Comment on lines +53 to +54
static unsigned pidCounter = 0;
swprintf(pipeName, ARRAY_SIZE(pipeName), L"\\\\.\\pipe\\FASTFETCH-%u-%u", GetCurrentProcessId(), ++pidCounter);
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pidCounter variable is not thread-safe. If multiple threads call ffProcessSpawn concurrently, race conditions could occur leading to duplicate pipe names. Consider using atomic operations or adding thread safety documentation.

Suggested change
static unsigned pidCounter = 0;
swprintf(pipeName, ARRAY_SIZE(pipeName), L"\\\\.\\pipe\\FASTFETCH-%u-%u", GetCurrentProcessId(), ++pidCounter);
static volatile LONG pidCounter = 0;
swprintf(pipeName, ARRAY_SIZE(pipeName), L"\\\\.\\pipe\\FASTFETCH-%u-%u", GetCurrentProcessId(), InterlockedIncrement(&pidCounter));

Copilot uses AI. Check for mistakes.
yyjson_mut_obj_add_strbuf(doc, song, "album", &media->album);
yyjson_mut_obj_add_strbuf(doc, song, "status", &media->status);
if (media->cover.length > 0)
yyjson_mut_obj_add_strbuf(doc, song, "cover", &media->status);
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cover field is incorrectly assigned &media->status instead of &media->cover. This will output the wrong data in the JSON result.

Copilot uses AI. Check for mistakes.
@CarterLi CarterLi requested a review from Copilot November 11, 2025 06:47
Copilot finished reviewing on behalf of CarterLi November 11, 2025 06:58
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 71 out of 71 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

debian/changelog.tpl:12

  • The changelog template contains entries for versions 2.54.1 and 2.54.0, but the CMakeLists.txt sets the version to 2.55.0 and the PR title is "Releases: v2.55.0". The changelog template should be updated with a 2.55.0 entry to match the release version.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

static inline const char* ffProcessAppendStdOut(FFstrbuf* buffer, char* const argv[])
{
const char* error = ffProcessAppendOutput(buffer, argv, false);
FFProcessHandle handle;
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uninitialized struct: The FFProcessHandle handle is declared without initialization. On Windows, if ffProcessSpawn returns early with an error, the handle's fields could contain garbage values. While the current code doesn't use the handle if spawn fails, it's safer to initialize it (e.g., FFProcessHandle handle = {};) to avoid potential issues if the code changes in the future.

Copilot uses AI. Check for mistakes.
static inline const char* ffProcessAppendStdErr(FFstrbuf* buffer, char* const argv[])
{
const char* error = ffProcessAppendOutput(buffer, argv, true);
FFProcessHandle handle;
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uninitialized struct: The FFProcessHandle handle is declared without initialization. On Windows, if ffProcessSpawn returns early with an error, the handle's fields could contain garbage values. While the current code doesn't use the handle if spawn fails, it's safer to initialize it (e.g., FFProcessHandle handle = {};) to avoid potential issues if the code changes in the future.

Copilot uses AI. Check for mistakes.
Comment on lines 84 to 91
if (path.chars[i] == '%')
{
if (i + 2 >= path.length)
break;
char str[] = { path.chars[i + 1], path.chars[i + 2], 0 };
const char decodedChar = (char) strtoul(str, NULL, 16);
i += 2;
ffStrbufAppendC(&result->cover, decodedChar);
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential buffer overflow: The URL decoding logic doesn't validate that strtoul successfully parsed the hex value. If the hex string is invalid, this could append incorrect characters. Consider checking the result or using safer parsing.

Copilot uses AI. Check for mistakes.
Comment on lines +40 to +41
if (saveCover && result.removeCoverAfterUse)
atexit(removeMediaCoverFile);
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential issue with atexit: Registering removeMediaCoverFile with atexit means it will only be called once at program termination. However, if ffDetectMedia(true) is called multiple times with different cover files, only the last cover file path will be stored in the static result.cover, and previous temporary cover files won't be cleaned up. Consider tracking multiple cover files or cleaning up the previous file before creating a new one.

Copilot uses AI. Check for mistakes.
Comment on lines +11 to +14
// FIFO, non-thread-safe list of running commands
static FFlist commandQueue = {
.elementSize = sizeof(FFCommandResultBundle),
};
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says "non-thread-safe list" which is good documentation, but the global static commandQueue could be problematic if commands are executed concurrently from multiple threads. Consider adding a mutex or documenting that this module should only be used from a single thread.

Copilot uses AI. Check for mistakes.
Comment on lines 89 to 92
//fish, version 4.0.2-1 (Built by MSYS2 project) // version can be localized if LC_ALL is set
if (version->length < strlen("fish, v")) return false;
uint32_t index = ffStrbufNextIndexC(version, strlen("fish, "), ' ');
ffStrbufSubstrAfter(version, index);
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing validation: The fish version detection assumes the string has at least strlen("fish, v") characters but doesn't validate the actual format after finding the space. If the output format changes or is localized differently, the index could point beyond the string bounds when passed to ffStrbufSubstrAfter. Consider adding bounds checking after finding the space index.

Copilot uses AI. Check for mistakes.
@CarterLi CarterLi requested a review from Copilot November 11, 2025 07:58
Copilot finished reviewing on behalf of CarterLi November 11, 2025 08:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 71 out of 71 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +48 to +49
state = (FFNetworkingState){};
status = FF_UNITIALIZED;
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resetting state after reading response prevents module reuse. The module resets state and status to their initial values after reading the response, which contradicts the error message in ffPrepareWeather stating "Weather module can only be used once due to internal limitations". Either remove the reset (lines 48-49) or remove the restriction in ffPrepareWeather.

Copilot uses AI. Check for mistakes.

static inline void cfReleaseWrapper(void* type)
{
assert(type);
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary assert statement. The assert(type) is redundant because type is a pointer to the stack variable and can never be NULL. The actual null check needed is on line 28 for *(CFTypeRef*) type. Consider removing line 27.

Copilot uses AI. Check for mistakes.
Comment on lines +37 to +39
FFCommandResultBundle* bundle = ffListAdd(&commandQueue);
bundle->error = spawnProcess(options, &bundle->handle);

Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential resource leak when parallel execution is enabled. If spawnProcess fails on line 38, the bundle is added to the queue with an error, but the process handle may contain allocated resources that are never cleaned up. The handle should be initialized or the bundle should not be added to the queue if spawning fails.

Suggested change
FFCommandResultBundle* bundle = ffListAdd(&commandQueue);
bundle->error = spawnProcess(options, &bundle->handle);
FFCommandResultBundle bundle = {};
bundle.error = spawnProcess(options, &bundle.handle);
if (bundle.error)
return false;
*(FFCommandResultBundle*)ffListAdd(&commandQueue) = bundle;

Copilot uses AI. Check for mistakes.
@CarterLi CarterLi merged commit 7a2bf4e into master Nov 11, 2025
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants