feat: enable the configuration for the preferred resolution.#464
Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:release/eaglefrom Mar 19, 2026
Merged
Conversation
Reviewer's GuideAdds support for configuring a preferred camera resolution via DConfig, plumbing the setting from the Qt application layer into the v4l2 core and using it (when valid) to select the stream resolution, otherwise falling back to the maximum valid resolution. Sequence diagram for configuring preferred resolution via DConfigsequenceDiagram
participant Main
participant DConfig
participant DataManager
participant V4L2Formats
participant V4L2Core
Main->>DConfig: value(preferredResolution)
DConfig-->>Main: "WIDTHxHEIGHT" string or empty
alt preferredResolution present
Main->>DataManager: setPreferredResolution(resolution)
DataManager->>DataManager: parse "WxH" to width, height
DataManager->>V4L2Formats: is_valid_resolution(width, height)
alt resolution valid
V4L2Formats-->>DataManager: true
DataManager->>DataManager: m_preferredResolution = QSize(width, height)
else resolution invalid
V4L2Formats-->>DataManager: false
DataManager-->>Main: preferred resolution rejected
end
Main->>DataManager: getPreferredResolution()
DataManager-->>Main: QSize(width, height) or null
alt QSize not null
Main->>V4L2Formats: set_preferred_resolution(width, height)
V4L2Formats->>V4L2Formats: store preferred_resolution_width/height
end
end
Note over V4L2Core,V4L2Formats: Later, when preparing stream resolution
V4L2Core->>V4L2Formats: get_preferred_resolution_width()
V4L2Formats-->>V4L2Core: prep_w
V4L2Core->>V4L2Formats: get_preferred_resolution_height()
V4L2Formats-->>V4L2Core: prep_h
V4L2Core->>V4L2Core: iterate resolutions, track preferred and max
alt preferred found and valid
V4L2Core->>V4L2Core: my_width = prep_w, my_height = prep_h
else
V4L2Core->>V4L2Core: my_width = max_width, my_height = max_height
end
Updated class diagram for DataManager and v4l2 formats preferred resolutionclassDiagram
class DataManager {
-static DataManager* m_dataManager
-QSize m_preferredResolution
-CApplication* m_app
-QStringList m_deviceIds
-GridType m_gridType
-EncodeEnv m_encodeEnv
-DeviceStatus m_devStatus
-bool m_H264EncoderExists
-bool m_enableUsbGroup
-bool m_enable8kPreview
-QSet<QString> m_deviceBlacklistSet
-DataManager()
+static DataManager* instance()
+bool isDeviceValid(QString vid, QString pid, QString name)
+void setPreferredResolution(QString resolution)
+QSize getPreferredResolution()
}
class V4L2Formats {
-static int enable_8k_preview
-static int preferred_resolution_width
-static int preferred_resolution_height
+void set_enable_8k_preview(int enable)
+int is_enable_8k_preview()
+int is_valid_resolution(int width, int height)
+void set_preferred_resolution(int width, int height)
+int get_preferred_resolution_width()
+int get_preferred_resolution_height()
}
class Main {
+int main(int argc, char* argv[])
}
Main --> DataManager : uses
Main --> V4L2Formats : calls
DataManager --> V4L2Formats : uses is_valid_resolution
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
v4l2core_prepare_valid_resolution, the logicif (w >= max_width && h >= max_height)may not actually pick the "largest" resolution when resolutions differ in aspect ratio or ordering; consider using a clearer comparison strategy (e.g. prefer larger width then height, or compare total pixel count) to avoid unintuitive selections. - When no resolution passes
is_valid_resolution,max_width/max_heightremain 0 and will be used as the selected resolution; it would be safer to explicitly handle the "no valid resolution" case (e.g. keep the previousmy_width/my_heightor fall back to a known safe default) instead of potentially selecting 0x0. - In
DataManager::setPreferredResolution, invalid or empty input leaves any previousm_preferredResolutionvalue untouched, which may be surprising; consider either clearing the stored preference on invalid input or explicitly documenting this behavior, and optionally trimming whitespace around the parsed width/height to be more tolerant of configuration values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `v4l2core_prepare_valid_resolution`, the logic `if (w >= max_width && h >= max_height)` may not actually pick the "largest" resolution when resolutions differ in aspect ratio or ordering; consider using a clearer comparison strategy (e.g. prefer larger width then height, or compare total pixel count) to avoid unintuitive selections.
- When no resolution passes `is_valid_resolution`, `max_width`/`max_height` remain 0 and will be used as the selected resolution; it would be safer to explicitly handle the "no valid resolution" case (e.g. keep the previous `my_width`/`my_height` or fall back to a known safe default) instead of potentially selecting 0x0.
- In `DataManager::setPreferredResolution`, invalid or empty input leaves any previous `m_preferredResolution` value untouched, which may be surprising; consider either clearing the stored preference on invalid input or explicitly documenting this behavior, and optionally trimming whitespace around the parsed width/height to be more tolerant of configuration values.
## Individual Comments
### Comment 1
<location path="libcam/libcam_v4l2core/v4l2_core.c" line_range="1699" />
<code_context>
- // 由于上游guvcview项目在解码3840x2160分辨率以上时会出现异常（出现条纹、崩溃等），所以我们需要限制可用分辨率到3840x2160
- printf("V4L2_CORE: valid resolution counts: %d\n", vd->list_stream_formats[format_index].numb_res);
- for(int i=0; i < vd->list_stream_formats[format_index].numb_res; i++)
+ int found_preferred = 0; // 是否找到首选分辨率
+ int prep_w = get_preferred_resolution_width();
+ int prep_h = get_preferred_resolution_height();
</code_context>
<issue_to_address>
**issue (bug_risk):** Max-resolution selection logic can skip valid candidates depending on list ordering.
The condition `if (w >= max_width && h >= max_height)` doesn’t define a consistent ordering: e.g., 4096x1440 and 3840x2160 can each block the other depending on iteration order. As a result, the chosen “max” depends on `list_stream_cap` order rather than a well-defined maximum. If you want a true maximum, consider comparing by area or lexicographically, e.g. `if (w > max_width || (w == max_width && h > max_height))`.
</issue_to_address>
### Comment 2
<location path="libcam/libcam_v4l2core/v4l2_core.c" line_range="1706-1715" />
<code_context>
for (int i = 0; i < vd->list_stream_formats[format_index].numb_res; i++)
</code_context>
<issue_to_address>
**issue (bug_risk):** No valid resolutions leaves `max_width`/`max_height` at 0 and selects 0x0.
If `is_valid_resolution(w, h)` is false for all entries (or `numb_res == 0`), `max_width`/`max_height` stay 0, so later `my_width`/`my_height` become 0 and you end up selecting a 0x0 resolution. Please handle the “no valid resolution” case explicitly (e.g., keep the previous values, use a safe default, or signal an error) instead of propagating 0x0 downstream.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
lichaofan2008
force-pushed
the
release/eagle
branch
4 times, most recently
from
2133542 to
2a556d7
Compare
Use the DConfig mechanism to enable the configuration for the preferred resolution. 使用DConfig机制，开放首选分辨率的配置功能。 Task: https://pms.uniontech.com/task-view-387315.html
lichaofan2008
force-pushed
the
release/eagle
branch
from
2a556d7 to
dfe5563
Compare
deepin pr auto reviewGit Diff 代码审查报告总体评估这段代码实现了一个新功能：允许通过配置文件设置摄像头的"首选分辨率"。当摄像头支持多种分辨率时，系统会优先使用配置的首选分辨率，如果未设置或摄像头不支持该分辨率，则使用最大分辨率。 代码整体结构清晰，实现了从配置文件到核心库的完整数据流，但存在一些可以改进的地方。 详细审查意见1. 语法和逻辑问题v4l2_core.c 中的逻辑问题if (found_preferred) {
// 有首选分辨率，使用首选分辨率
printf("V4L2_CORE: use preferred resolution, width: %d, height: %d\n", pref_w, pref_h);
my_width = pref_w;
my_height = pref_h;
} else {
// 没有首选分辨率，使用最大分辨率
printf("V4L2_CORE: use max resolution, width: %d, height: %d\n", max_width, max_height);
my_width = max_width;
my_height = max_height;
// 极端情况，没有可用分辨率，发出错误提示
if (max_width == 0 || max_height == 0) {
printf("V4L2_CORE: ERROR: no valid resolution found!\n");
}
}问题：当没有找到任何有效分辨率时（max_width和max_height为0），代码仍然会设置my_width和my_height为0，这可能导致后续操作出现问题。 建议：应该在没有有效分辨率时采取更明确的错误处理措施，例如： if (found_preferred) {
// 有首选分辨率，使用首选分辨率
printf("V4L2_CORE: use preferred resolution, width: %d, height: %d\n", pref_w, pref_h);
my_width = pref_w;
my_height = pref_h;
} else if (max_width > 0 && max_height > 0) {
// 没有首选分辨率，使用最大分辨率
printf("V4L2_CORE: use max resolution, width: %d, height: %d\n", max_width, max_height);
my_width = max_width;
my_height = max_height;
} else {
// 极端情况，没有可用分辨率，发出错误提示
printf("V4L2_CORE: ERROR: no valid resolution found!\n");
// 可以考虑设置一个默认的安全分辨率或返回错误状态
// 例如设置一个最低分辨率作为后备
my_width = 640;
my_height = 480;
}2. 代码质量问题日志输出过多v4l2_core.c中添加了大量的printf语句，这可能会影响性能，尤其是在高频调用的场景中。 建议：考虑使用条件编译或日志级别控制，例如： #ifdef DEBUG_V4L2_RESOLUTION
printf("V4L2_CORE: check valid resolution, width: %d, height: %d, preferred width: %d, preferred height: %d\n",
w, h, pref_w, pref_h);
#endif魔法数字DataManager.cpp中使用了硬编码的MAX_RESOLUTION = 16384，这应该定义为常量或配置项： // 在类中定义常量
static const int MAX_RESOLUTION = 16384;3. 性能问题分辨率遍历效率v4l2_core.c中的分辨率遍历逻辑是O(n)的，这在分辨率数量不多时没有问题，但如果分辨率列表很长，可以考虑优化。 建议：如果分辨率列表很大，可以考虑使用哈希表或其他数据结构来快速查找特定分辨率。 4. 安全问题输入验证DataManager::setPreferredResolution中对分辨率字符串的解析和验证做得较好，但可以进一步增强： void DataManager::setPreferredResolution(const QString &resolution)
{
if (resolution.isEmpty()) {
qInfo() << "Preferred resolution is empty";
return;
}
// 添加对格式更严格的检查
QRegularExpression re("^\\d+x\\d+$");
if (!re.match(resolution).hasMatch()) {
qWarning() << "Invalid resolution format:" << resolution << "Expected format: WIDTHxHEIGHT";
return;
}
QStringList parts = resolution.split("x");
bool widthOk, heightOk;
int width = parts[0].toInt(&widthOk);
int height = parts[1].toInt(&heightOk);
if (!widthOk || !heightOk || width <= 0 || height <= 0) {
qWarning() << "Invalid resolution values:" << resolution;
return;
}
// 添加合理的上限检查，例如16K
static const int MAX_RESOLUTION = 16384;
if (width > MAX_RESOLUTION || height > MAX_RESOLUTION) {
qWarning() << "Resolution exceeds maximum limit:" << resolution;
return;
}
if (!is_valid_resolution(width, height)) {
qWarning() << "Resolution does not meet validation requirements:" << resolution;
return;
}
m_preferredResolution = QSize(width, height);
}线程安全v4l2_formats.c中的preferred_resolution_width和preferred_resolution_height是静态全局变量，没有线程安全保护。 建议：添加互斥锁保护这些变量的访问： #include <pthread.h>
static pthread_mutex_t preferred_resolution_mutex = PTHREAD_MUTEX_INITIALIZER;
static int preferred_resolution_width = 0;
static int preferred_resolution_height = 0;
void set_preferred_resolution(int width, int height)
{
if (width <= 0 || height <= 0) {
printf("V4L2_CORE: Invalid preferred resolution: %dx%d\n", width, height);
return;
}
pthread_mutex_lock(&preferred_resolution_mutex);
preferred_resolution_width = width;
preferred_resolution_height = height;
pthread_mutex_unlock(&preferred_resolution_mutex);
}
int get_preferred_resolution_width()
{
int width;
pthread_mutex_lock(&preferred_resolution_mutex);
width = preferred_resolution_width;
pthread_mutex_unlock(&preferred_resolution_mutex);
return width;
}
int get_preferred_resolution_height()
{
int height;
pthread_mutex_lock(&preferred_resolution_mutex);
height = preferred_resolution_height;
pthread_mutex_unlock(&preferred_resolution_mutex);
return height;
}5. 其他建议配置文件解析main.cpp中对配置文件的解析可以更加健壮： if (dconfig && dconfig->isValid() && dconfig->keyList().contains("preferredResolution")) {
QString preferredResolution = dconfig->value("preferredResolution").toString();
qInfo() << "preferred resolution:" << preferredResolution;
if (!preferredResolution.isEmpty()) {
DataManager::instance()->setPreferredResolution(preferredResolution);
QSize size = DataManager::instance()->getPreferredResolution();
if (!size.isNull()) {
set_preferred_resolution(size.width(), size.height());
qInfo() << "Successfully set preferred resolution to" << size.width() << "x" << size.height();
} else {
qWarning() << "Failed to set preferred resolution";
}
}
}总结这段代码实现了一个有用的功能，允许用户配置摄像头的首选分辨率。整体实现较为完整，但在错误处理、线程安全和性能方面还有改进空间。建议在合并前解决上述问题，特别是错误处理和线程安全问题，以提高代码的健壮性和可靠性。 |
max-lvs
approved these changes
Mar 19, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lichaofan2008, max-lvs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/merge |
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
3 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.
Use the DConfig mechanism to enable the configuration for the preferred resolution.
使用DConfig机制，开放首选分辨率的配置功能。
Task: https://pms.uniontech.com/task-view-387315.html
Summary by Sourcery
Add configurable preferred camera resolution selection via DConfig and propagate it into the V4L2 core resolution choice.
New Features: