feat: enable the configuration for 8K preview.#462
feat: enable the configuration for 8K preview.#462deepin-bot[bot] merged 1 commit intolinuxdeepin:release/eaglefrom
Conversation
Reviewer's GuideEnables configurable 8K preview via DConfig, loosens resolution validation when 8K preview is enabled, and disables recording UI/behavior at 8K while wiring configuration through DataManager and the V4L2 layer. Sequence diagram for 8K preview configuration on startupsequenceDiagram
participant Main
participant DConfig
participant DataManager
participant V4L2Formats
Main->>DConfig: isValid()
alt DConfig valid
Main->>DConfig: keyList()
DConfig-->>Main: list
Main->>DConfig: value(enable8kPreview)
DConfig-->>Main: bool enable
Main->>DataManager: setEnable8kPreview(enable)
Main->>V4L2Formats: set_enable_8k_preview(enable ? 1 : 0)
else DConfig invalid or key missing
Main-->>Main: use default 8K preview disabled
end
note over V4L2Formats: enable_8k_preview affects is_valid_resolution
Updated class diagram for 8K preview configuration and UI controlclassDiagram
class DataManager {
-DataManager *m_dataManager
-bool m_isInstallFilter
-bool m_bSupportSnapshot
-bool m_isSupportCameraSwitch
-bool m_isPreviewNoDelay
-bool m_enableUsbGroup
-bool m_enable8kPreview
+static DataManager *instance()
+bool isInstallFilter()
+bool isSupportSnapshot()
+bool isSupportCameraSwitch()
+bool isPreviewNoDelay()
+bool isEnableUsbGroup()
+void setEnableUsbGroup(bool enable)
+void setEnable8kPreview(bool enable)
+bool isEnable8kPreview()
}
class CMainWindow {
+~CMainWindow()
+void onSwitchCameraSuccess(QString cameraName)
+void onSettingsDlgClose()
+void showChildWidget()
+void showWidget(DWidget *widget, bool bShow)
+bool checkResolutionTooHigh()
-DWidget *m_snapshotLabel
-DWidget *m_cameraSwitchBtn
-RollingBox *m_modeSwitchBox
-DWidget *m_photoRecordBtn
-DWidget *m_takePhotoSettingArea
-DWidget *m_filterName
-bool m_bSwitchCameraShowEnable
}
class RollingBox {
-int m_currentIndex
-int m_rangeMin
-int m_rangeMax
-int m_deviation
+int getCurrentValue()
+void reset()
+void setDeviation(int n)
+void wheelEvent(QWheelEvent *event)
+void mousePressEvent(QMouseEvent *event)
+void mouseMoveEvent(QMouseEvent *event)
+void mouseReleaseEvent(QMouseEvent *event)
}
class V4L2Formats {
-int enable_8k_preview
+int is_valid_resolution(int width, int height)
+void set_enable_8k_preview(int enable)
+int is_enable_8k_preview()
}
class V4L2DeviceHandler {
+v4l2_dev_t *get_v4l2_device_handler()
+int v4l2core_get_frame_width(v4l2_dev_t *fd)
}
CMainWindow --> RollingBox : uses
CMainWindow --> V4L2DeviceHandler : queries_width
CMainWindow ..> V4L2Formats : affected_by_is_valid_resolution
DataManager ..> V4L2Formats : configures_8K_preview
Flow diagram for resolution validation and UI behavior with 8K previewflowchart TD
A[Start_resolution_validation] --> B{enable_8k_preview?}
B -->|Yes| C{width > 0 and height > 0?}
C -->|No| Z[Resolution_invalid]
C -->|Yes| D{width % 16 == 0 and height % 4 == 0?}
D -->|Yes| Y[Resolution_valid]
D -->|No| Z
B -->|No| E{width > 0 and height > 0?}
E -->|No| Z
E -->|Yes| F{width <= MAX_WIDTH_LIMIT and height <= MAX_HEIGHT_LIMIT?}
F -->|No| Z
F -->|Yes| G{width % 16 == 0 and height % 8 == 0?}
G -->|Yes| Y
G -->|No| Z
subgraph UI_behavior_in_CMainWindow
H[Check_resolution_too_high] --> I[get_v4l2_device_handler]
I --> J{handler_exists?}
J -->|No| K[Assume_width_0]
J -->|Yes| L[v4l2core_get_frame_width]
L --> M[Compare_width_to_MAX_WIDTH_LIMIT]
K --> M
M --> N{currentWidth > MAX_WIDTH_LIMIT?}
N -->|Yes| O[isResolutionTooHigh = true]
N -->|No| P[isResolutionTooHigh = false]
O --> Q[showChildWidget_hides_modeSwitchBox]
P --> R[showChildWidget_shows_modeSwitchBox]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
enable_8k_previewflag and its accessors inv4l2_formats.c/.hare modeled asint; consider usingboolin the C++-facing API and a more explicit name (e.g.g_enable_8k_preview) to better convey semantics and global scope, and keep naming consistent withDataManager::setEnable8kPreview. checkResolutionTooHigh()only checks width againstMAX_WIDTH_LIMITand ignores height; if the intent is to mirror the resolution constraints used inis_valid_resolution, consider checking height and alignment constraints as well so UI behavior matches backend capability checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `enable_8k_preview` flag and its accessors in `v4l2_formats.c/.h` are modeled as `int`; consider using `bool` in the C++-facing API and a more explicit name (e.g. `g_enable_8k_preview`) to better convey semantics and global scope, and keep naming consistent with `DataManager::setEnable8kPreview`.
- `checkResolutionTooHigh()` only checks width against `MAX_WIDTH_LIMIT` and ignores height; if the intent is to mirror the resolution constraints used in `is_valid_resolution`, consider checking height and alignment constraints as well so UI behavior matches backend capability checks.
## Individual Comments
### Comment 1
<location path="libcam/libcam_v4l2core/v4l2_formats.c" line_range="122-123" />
<code_context>
0
};
+// 是否开启8K预览
+int enable_8k_preview = 0;
+
/* FIXME: doesn't support bigendian formats=> fourcc | (1 << 31)
</code_context>
<issue_to_address>
**suggestion:** Limit the 8K preview flag’s linkage scope to this translation unit
Given `set_enable_8k_preview`/`is_enable_8k_preview` already provide access, this variable doesn’t need external linkage. Please make `enable_8k_preview` `static` (or move it into an internal struct) to avoid exporting a global symbol and reduce risk of name collisions or unintended external modification.
```suggestion
/* 是否开启8K预览
* Restrict linkage to this translation unit; external access should go
* through set_enable_8k_preview()/is_enable_8k_preview().
*/
static int enable_8k_preview = 0;
```
</issue_to_address>
### Comment 2
<location path="src/src/mainwindow.cpp" line_range="2362-2373" />
<code_context>
+ if (fd) {
+ currentWidth = v4l2core_get_frame_width(fd);
+ }
+ bool isTrue = currentWidth > MAX_WIDTH_LIMIT;
+ qInfo() << __func__ << isTrue << currentWidth;
+
+ return isTrue;
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid noisy logging in a frequently called helper
If `checkResolutionTooHigh()` is invoked on every UI update (e.g. via `showChildWidget()`), this `qInfo()` will produce excessive log output in normal use. Consider logging only on state changes (when the result flips), lowering this to a debug-level log, or removing it once initial debugging is done.
```suggestion
bool CMainWindow::checkResolutionTooHigh()
{
int currentWidth = 0;
v4l2_dev_t *fd = get_v4l2_device_handler();
if (fd) {
currentWidth = v4l2core_get_frame_width(fd);
}
bool isTrue = currentWidth > MAX_WIDTH_LIMIT;
// Log only when the result changes to avoid noisy logging in frequent calls.
static bool lastIsTrue = false;
if (isTrue != lastIsTrue) {
qInfo() << __func__ << "resolutionTooHigh changed:" << isTrue << "width:" << currentWidth;
lastIsTrue = isTrue;
}
return isTrue;
}
```
</issue_to_address>
### Comment 3
<location path="src/src/mainwindow.h" line_range="207" />
<code_context>
+ /**
+ * @brief 检测分辨率是否太高,8K分辨率不显示录像功能
+ */
+ bool checkResolutionTooHigh();
+
private slots:
</code_context>
<issue_to_address>
**suggestion:** Mark `checkResolutionTooHigh` as `const` since it doesn’t modify state
`checkResolutionTooHigh()` only queries the V4L2 device and doesn’t modify any members, so it can be declared `const`. This improves clarity and allows calls on `const` instances of the class.
Suggested implementation:
```c
/**
* @brief 检测分辨率是否太高,8K分辨率不显示录像功能
*/
bool checkResolutionTooHigh() const;
```
You will also need to update the corresponding definition in the `.cpp` file (likely `mainwindow.cpp`) to match the new signature by adding `const`:
- Change `bool MainWindow::checkResolutionTooHigh()` to `bool MainWindow::checkResolutionTooHigh() const`.
Ensure any overrides or interface declarations are updated similarly if this method is virtual or declared in a base class.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/src/mainwindow.cpp
Outdated
| bool CMainWindow::checkResolutionTooHigh() | ||
| { | ||
| int currentWidth = 0; | ||
| v4l2_dev_t *fd = get_v4l2_device_handler(); | ||
| if (fd) { | ||
| currentWidth = v4l2core_get_frame_width(fd); | ||
| } | ||
| bool isTrue = currentWidth > MAX_WIDTH_LIMIT; | ||
| qInfo() << __func__ << isTrue << currentWidth; | ||
|
|
||
| return isTrue; | ||
| } |
There was a problem hiding this comment.
suggestion (performance): Avoid noisy logging in a frequently called helper
If checkResolutionTooHigh() is invoked on every UI update (e.g. via showChildWidget()), this qInfo() will produce excessive log output in normal use. Consider logging only on state changes (when the result flips), lowering this to a debug-level log, or removing it once initial debugging is done.
| bool CMainWindow::checkResolutionTooHigh() | |
| { | |
| int currentWidth = 0; | |
| v4l2_dev_t *fd = get_v4l2_device_handler(); | |
| if (fd) { | |
| currentWidth = v4l2core_get_frame_width(fd); | |
| } | |
| bool isTrue = currentWidth > MAX_WIDTH_LIMIT; | |
| qInfo() << __func__ << isTrue << currentWidth; | |
| return isTrue; | |
| } | |
| bool CMainWindow::checkResolutionTooHigh() | |
| { | |
| int currentWidth = 0; | |
| v4l2_dev_t *fd = get_v4l2_device_handler(); | |
| if (fd) { | |
| currentWidth = v4l2core_get_frame_width(fd); | |
| } | |
| bool isTrue = currentWidth > MAX_WIDTH_LIMIT; | |
| // Log only when the result changes to avoid noisy logging in frequent calls. | |
| static bool lastIsTrue = false; | |
| if (isTrue != lastIsTrue) { | |
| qInfo() << __func__ << "resolutionTooHigh changed:" << isTrue << "width:" << currentWidth; | |
| lastIsTrue = isTrue; | |
| } | |
| return isTrue; | |
| } |
Use the DConfig mechanism to enable the configuration for 8K preview. 使用DConfig机制,开放8K预览功能,同时8K不支持录像。 代码来自xiwo分支。 Bug: https://pms.uniontech.com/bug-view-349401.html
85d7dc5 to
db852b6
Compare
deepin pr auto review这段代码主要实现了针对8K分辨率(特别是希沃品牌的8192x2772分辨率)的预览支持,并通过配置开关控制是否启用该功能。同时,为了防止在高分辨率下录像出现问题,代码增加了在8K分辨率下禁用录像功能的逻辑。 以下是对该代码的审查意见,分为语法逻辑、代码质量、代码性能和代码安全四个方面: 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
总结与改进建议
总体而言,这段代码逻辑清晰,注释完善,能够较好地解决特定硬件(希沃8K)的兼容性问题,同时通过配置开关控制风险,实现方式是合理的。 |
|
[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 |
|
/merge |
Use the DConfig mechanism to enable the configuration for 8K preview.
使用DConfig机制,开放8K预览功能,同时8K不支持录像。
代码来自xiwo分支。
Summary by Sourcery
Enable configurable 8K camera preview support while disabling recording at 8K resolutions.
New Features:
Enhancements: