feat(mpv): prioritize gpuinfo VO recommendation in AUTO decode mode#841
feat(mpv): prioritize gpuinfo VO recommendation in AUTO decode mode#841wyu71 merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntegrates a new gpuinfo_get_vo function pointer into MpvProxy so that in AUTO decode mode, mpv prioritizes the VO (video output) recommended by libgpuinfo over the existing heuristic, while ensuring proper initialization and null-handling of the new function pointer. Sequence diagram for AUTO decode VO selection with gpuinfo_get_vosequenceDiagram
actor User
participant Application
participant MpvProxy
participant LibGpuInfo as libgpuinfo_so
participant MpvCore as mpv_core
User ->> Application: Start playback
Application ->> MpvProxy: mpv_init()
activate MpvProxy
MpvProxy ->> MpvProxy: initGpuInfoFuns()
alt libgpuinfo.so exists
MpvProxy ->> LibGpuInfo: resolve vdp_Iter_decoderInfo
MpvProxy ->> LibGpuInfo: resolve gpuinfo_get_vo
MpvProxy ->> MpvProxy: store m_gpuInfo and m_gpuInfoVo
else libgpuinfo.so missing
MpvProxy ->> MpvProxy: set m_gpuInfo = NULL
MpvProxy ->> MpvProxy: set m_gpuInfoVo = NULL
end
MpvProxy ->> MpvCore: create mpv_handle
MpvProxy ->> MpvProxy: check m_decodeMode
alt DecodeMode AUTO
MpvProxy ->> MpvCore: my_set_property(vo, heuristic_vo)
MpvProxy ->> MpvProxy: m_sInitVo = heuristic_vo
alt m_gpuInfoVo is not NULL
MpvProxy ->> LibGpuInfo: gpuinfo_get_vo()
LibGpuInfo -->> MpvProxy: vo
alt vo is non empty
MpvProxy ->> MpvCore: my_set_property(vo, vo)
MpvProxy ->> MpvProxy: m_sInitVo = vo
end
end
else DecodeMode HARDWARE or SOFTWARE
MpvProxy ->> MpvCore: set vo based on other logic
end
MpvProxy -->> Application: return initialized mpv_handle
deactivate MpvProxy
Application ->> MpvCore: start playback
Class diagram for MpvProxy updates with gpuinfo_get_vo integrationclassDiagram
class MpvProxy {
// Members
void* m_gpuInfo
void* m_gpuInfoVo
MpvHandle m_handle
QString m_sInitVo
DecodeMode m_decodeMode
// Methods
void initGpuInfoFuns()
mpv_handle* mpv_init()
void initMember()
void play()
}
class LibGpuInfo {
<<library>>
const char* vdp_Iter_decoderInfo()
const char* gpuinfo_get_vo()
}
class MpvCore {
<<library>>
void my_set_property(mpv_handle* handle, const char* name, const char* value)
}
MpvProxy ..> LibGpuInfo : dynamic_link
MpvProxy ..> MpvCore : uses
class DecodeMode {
<<enum>>
AUTO
HARDWARE
SOFTWARE
}
MpvProxy --> DecodeMode
Flow diagram for AUTO decode VO selection priorityflowchart TD
A["Start mpv_init in AUTO decode mode"] --> B["Set VO using existing heuristic"]
B --> C{"Is m_gpuInfoVo non NULL?"}
C -- "No" --> F["Keep heuristic VO"]
C -- "Yes" --> D["Call gpuinfo_get_vo() to get vo"]
D --> E{"Is vo non empty?"}
E -- "No" --> F
E -- "Yes" --> G["Set VO to gpuinfo recommended vo"]
F --> H["m_sInitVo reflects final VO"]
G --> H
H --> I["Return initialized mpv_handle"]
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 2 issues, and left some high level feedback:
- Storing function pointers from a QLibrary that is created as a local variable in
initGpuInfoFuns()is risky because the library may be unloaded when the QLibrary object goes out of scope; consider making the QLibrary a member (or disabling auto-unload) som_gpuInfo/m_gpuInfoVoremain valid when used later inmpv_init(). - Since the gpuinfo VO is only meaningful in AUTO decode mode, it may be safer to guard the
m_gpuInfoVocall with an explicit decode-mode check (and/or log when gpuinfo is available but not used) to make the behavior clearer and avoid accidental use under other modes in future refactors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Storing function pointers from a QLibrary that is created as a local variable in `initGpuInfoFuns()` is risky because the library may be unloaded when the QLibrary object goes out of scope; consider making the QLibrary a member (or disabling auto-unload) so `m_gpuInfo`/`m_gpuInfoVo` remain valid when used later in `mpv_init()`.
- Since the gpuinfo VO is only meaningful in AUTO decode mode, it may be safer to guard the `m_gpuInfoVo` call with an explicit decode-mode check (and/or log when gpuinfo is available but not used) to make the behavior clearer and avoid accidental use under other modes in future refactors.
## Individual Comments
### Comment 1
<location path="src/backends/mpv/mpv_proxy.cpp" line_range="218" />
<code_context>
}
QLibrary mpvLibrary(SysUtils::libPath("libgpuinfo.so"));
m_gpuInfo = reinterpret_cast<void *>(mpvLibrary.resolve("vdp_Iter_decoderInfo"));
+ m_gpuInfoVo = reinterpret_cast<void *>(mpvLibrary.resolve("gpuinfo_get_vo"));
qInfo() << "GPU info functions initialized successfully";
}
</code_context>
<issue_to_address>
**issue (bug_risk):** QLibrary lifetime may invalidate stored function pointers after `initGpuInfoFuns` returns.
Because `QLibrary mpvLibrary` is local to `initGpuInfoFuns`, the library may be unloaded when the function returns, leaving `m_gpuInfo` and `m_gpuInfoVo` as dangling function pointers. This issue already existed for `m_gpuInfo`, and extending it to `m_gpuInfoVo` increases the risk. Please keep a `QLibrary` instance alive (e.g., as a member) for as long as these function pointers are used so they remain valid.
</issue_to_address>
### Comment 2
<location path="src/backends/mpv/mpv_proxy.h" line_range="453" />
<code_context>
mpvinitialize m_initialize;
mpv_freeNode_contents m_freeNodecontents;
void *m_gpuInfo; //解码探测函数指针
+ void *m_gpuInfoVo; //gpuinfo_get_vo函数指针
</code_context>
<issue_to_address>
**suggestion:** Use a typed function pointer instead of `void*` for `m_gpuInfoVo` to avoid repeated unsafe casts.
Because `m_gpuInfoVo` is always used as `const char* (*)(void)`, declare it with that function pointer type instead of `void*`. This removes the need for `reinterpret_cast` at the call site and makes the interface safer and harder to misuse.
Suggested implementation:
```c
mpvinitialize m_initialize;
mpv_freeNode_contents m_freeNodecontents;
void *m_gpuInfo; //解码探测函数指针
const char* (*m_gpuInfoVo)(void); //gpuinfo_get_vo函数指针
```
1. Wherever `m_gpuInfoVo` is assigned (likely from `mpv_get_wakeup_client`-style APIs or dlsym-like lookups), ensure the right-hand side is cast to `const char* (*)(void)` instead of `void*`, e.g.:
- Before: `m_gpuInfoVo = reinterpret_cast<void*>(gpuinfo_get_vo);`
- After: `m_gpuInfoVo = reinterpret_cast<const char* (*)(void)>(gpuinfo_get_vo);` (or preferably no cast if the symbol already has that type).
2. Wherever `m_gpuInfoVo` is used, remove any `reinterpret_cast<const char* (*)(void)>(m_gpuInfoVo)` and call it directly:
- Before: `auto vo = reinterpret_cast<const char* (*)(void)>(m_gpuInfoVo)();`
- After: `auto vo = m_gpuInfoVo();`
3. If there are null checks, they remain valid (`if (m_gpuInfoVo) { ... }`) because function pointers are nullable.
</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/backends/mpv/mpv_proxy.cpp
Outdated
| } | ||
| QLibrary mpvLibrary(SysUtils::libPath("libgpuinfo.so")); | ||
| m_gpuInfo = reinterpret_cast<void *>(mpvLibrary.resolve("vdp_Iter_decoderInfo")); | ||
| m_gpuInfoVo = reinterpret_cast<void *>(mpvLibrary.resolve("gpuinfo_get_vo")); |
There was a problem hiding this comment.
issue (bug_risk): QLibrary lifetime may invalidate stored function pointers after initGpuInfoFuns returns.
Because QLibrary mpvLibrary is local to initGpuInfoFuns, the library may be unloaded when the function returns, leaving m_gpuInfo and m_gpuInfoVo as dangling function pointers. This issue already existed for m_gpuInfo, and extending it to m_gpuInfoVo increases the risk. Please keep a QLibrary instance alive (e.g., as a member) for as long as these function pointers are used so they remain valid.
Use gpuinfo_get_vo() from libgpuinfo to get recommended video output for current GPU, overriding heuristic result in AUTO decode mode. 在自动解码模式下,优先使用gpuinfo提供的VO配置,覆盖硬件启发式判断。 Log: 自动解码模式优先使用gpuinfo的VO配置 PMS: BUG-356131 Influence: 当gpuinfo数据库中配置了GPU的推荐VO时,自动解码模式将优先使用该配置,提升特定GPU的兼容性。
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, wyu71 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 |
Use gpuinfo_get_vo() from libgpuinfo to get recommended video output for current GPU, overriding heuristic result in AUTO decode mode.
在自动解码模式下,优先使用gpuinfo提供的VO配置,覆盖硬件启发式判断。
Log: 自动解码模式优先使用gpuinfo的VO配置
PMS: BUG-356131
Influence: 当gpuinfo数据库中配置了GPU的推荐VO时,自动解码模式将优先使用该配置,提升特定GPU的兼容性。
Summary by Sourcery
Prioritize libgpuinfo's recommended video output (VO) in mpv's AUTO decode mode when available.
New Features:
Enhancements: