-
Notifications
You must be signed in to change notification settings - Fork 106
优化音频 #946
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
优化音频 #946
Conversation
Reviewer's GuideThis PR overhauls audio configuration and priority management by introducing persistent reduce-noise settings with null-sink support, refactoring port lookups and auto-switch logic to leverage the new PriorityManager APIs, redesigning priority_policy and priority_manager for type-based port queues with GetTheFirst/GetNextPort operations (and adding unit tests), and simplifying ConfigKeeper to use Card objects and add Mode support. Sequence diagram for setting reduce noise (persistent and module management)sequenceDiagram
participant A["Audio (a)"]
participant S["Source (source)"]
participant CK["ConfigKeeper"]
participant DC["audioDConfig"]
A->>S: setReduceNoise(enable)
S->>S: unload module-echo-cancel (if exists)
S->>S: load module-echo-cancel (if enable)
A->>CK: SetReduceNoise(card, portName, enable)
A->>DC: SetValue(dsgKeyReduceNoiseEnabled, enable)
Sequence diagram for auto-switching output port using new PriorityManagersequenceDiagram
participant A["Audio (a)"]
participant PM["PriorityManager"]
participant PP["PriorityPolicy"]
A->>PM: GetTheFirstPort(DirectionSink)
PM->>PP: GetTheFirstPort(availablePort)
PP-->>PM: PriorityPort, Position
PM-->>A: PriorityPort, Position
A->>A: setPort(cardId, portName, DirectionSink, true)
Sequence diagram for setting mono output (null-sink and module management)sequenceDiagram
participant A["Audio (a)"]
participant S["Sink (sink)"]
A->>S: SetMono(enable)
S->>S: unsetMono() (if exists)
S->>A: LoadModule("module-remap-sink", ...)
A->>A: SetDefaultSink("remap-sink-mono") (if enable)
ER diagram for CardConfig and PortConfig changeserDiagram
CardConfig {
string Name
}
PortConfig {
string Name
bool Enabled
float Volume
bool IncreaseVolume
float Balance
bool ReduceNoise
string Mode
}
CardConfig ||--o{ PortConfig : has
Class diagram for updated PriorityManager and PriorityPolicyclassDiagram
class PriorityManager {
+portList availablePort
+PriorityPolicy Output
+PriorityPolicy Input
+Init(cards)
+refreshPorts(cards)
+GetTheFirstPort(direction)
+SetTheFirstPort(cardName, portName, direction)
+GetNextPort(direction, pos)
}
class PriorityPolicy {
+map<int, PriorityPortList> Ports
+PriorityTypeList Types
+InsertPort(card, port)
+GetTheFirstPort(available)
+GetNextPort(available, pos)
+SetTheFirstPort(cardName, portName, available)
}
class PriorityPort {
+string CardName
+string PortName
+int PortType
}
class Position {
+int tp
+int index
}
PriorityManager "1" o-- "1" PriorityPolicy : Output
PriorityManager "1" o-- "1" PriorityPolicy : Input
PriorityPolicy "*" o-- "*" PriorityPort
PriorityPolicy "*" o-- "*" Position
PriorityManager "1" o-- "1" portList
PriorityPolicy "*" o-- "*" PriorityTypeList
PriorityPolicy "*" o-- "*" PriorityPortList
Class diagram for updated ConfigKeeper and Card/PortConfigclassDiagram
class ConfigKeeper {
+map<string, CardConfig> Cards
+GetCardAndPortConfig(card, portName)
+SetEnabled(card, portName, enabled)
+SetVolume(card, portName, volume)
+SetIncreaseVolume(card, portName, enhance)
+SetBalance(card, portName, balance)
+SetReduceNoise(card, portName, reduce)
+GetReduceNoise(card, portName)
+SetMode(card, portName, mode)
+GetMode(card, portName)
}
class CardConfig {
+string Name
+map<string, PortConfig> Ports
}
class PortConfig {
+string Name
+bool Enabled
+float Volume
+bool IncreaseVolume
+float Balance
+bool ReduceNoise
+string Mode
}
ConfigKeeper "1" o-- "*" CardConfig
CardConfig "1" o-- "*" PortConfig
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- There’s a lot of repeated code around fetching cards and ports (e.g. lookups, error checks, and config keeper calls) — consider extracting common lookup/error‐handling logic into shared helper functions to reduce duplication and improve readability.
- The refactored PriorityPolicy.SetTheFirstPort method has grown very complex — splitting it into smaller private methods (e.g. findAndRemoveTarget, insertBeforeFirstAvailable) or adding clear comments would make the intent and edge‐case handling easier to follow and maintain.
- Mono and echo-cancel module load/unload logic is sprinkled across multiple methods — centralize module management into a single component or manager to ensure consistent behavior and avoid mismatched load/unload sequences.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s a lot of repeated code around fetching cards and ports (e.g. lookups, error checks, and config keeper calls) — consider extracting common lookup/error‐handling logic into shared helper functions to reduce duplication and improve readability.
- The refactored PriorityPolicy.SetTheFirstPort method has grown very complex — splitting it into smaller private methods (e.g. findAndRemoveTarget, insertBeforeFirstAvailable) or adding clear comments would make the intent and edge‐case handling easier to follow and maintain.
- Mono and echo-cancel module load/unload logic is sprinkled across multiple methods — centralize module management into a single component or manager to ensure consistent behavior and avoid mismatched load/unload sequences.
## Individual Comments
### Comment 1
<location> `audio1/priority_policy.go:221` </location>
<code_context>
-// 将指定类型的优先级设为最高,此函数不会提高该类型的端口的优先级
-func (pp *PriorityPolicy) SetTheFirstType(portType int) bool {
- if portType < 0 || portType >= PortTypeCount {
+func (pp *PriorityPolicy) SetTheFirstPort(cardName string, portName string, available portList) bool {
+ if !available.isExists(cardName, portName) {
+ logger.Warningf("port <%v:%v> not avaiable", cardName, portName)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** SetTheFirstPort does not update the Types priority order.
Currently, moving a port to the front of its type's list does not adjust the Types slice, so the port's type may not have the highest priority. Please update Types to move the port's type to the front when SetTheFirstPort is called.
Suggested implementation:
```golang
func (pp *PriorityPolicy) SetTheFirstPort(cardName string, portName string, available portList) bool {
if !available.isExists(cardName, portName) {
logger.Warningf("port <%v:%v> not avaiable", cardName, portName)
return false
}
// 第一步:找到并删除指定的端口
var targetPort *PriorityPort
var targetType int
```
```golang
// 第一步:找到并删除指定的端口
var targetPort *PriorityPort
var targetType int
// 查找端口类型
for t, ports := range pp.Ports {
for i, p := range ports {
if p.CardName == cardName && p.PortName == portName {
targetPort = &pp.Ports[t][i]
targetType = t
break
}
}
if targetPort != nil {
break
}
}
if targetPort == nil {
logger.Warningf("port <%v:%v> not found in PriorityPolicy", cardName, portName)
return false
}
// 将端口移到其类型列表的最前面
ports := pp.Ports[targetType]
var newPorts []PriorityPort
for _, p := range ports {
if p.CardName == cardName && p.PortName == portName {
newPorts = append([]PriorityPort{p}, newPorts...)
} else {
newPorts = append(newPorts, p)
}
}
pp.Ports[targetType] = newPorts
// 将端口类型移到Types的最前面
var newTypes []int
newTypes = append(newTypes, targetType)
for _, t := range pp.Types {
if t != targetType {
newTypes = append(newTypes, t)
}
}
pp.Types = newTypes
```
</issue_to_address>
### Comment 2
<location> `audio1/audio.go:2163` </location>
<code_context>
+ a.context().LoadModule("module-null-sink", "")
+}
+
+func (a *Audio) setReduceNoise(enable bool) error {
+ if a.ReduceNoise == enable {
+ return nil
</code_context>
<issue_to_address>
**issue (bug_risk):** setReduceNoise does not check for Bluetooth devices before enabling noise reduction.
Add a check to prevent enabling noise reduction when the audio source is Bluetooth, as Bluetooth devices are not compatible with this feature.
</issue_to_address>
### Comment 3
<location> `audio1/audio.go:2106-2108` </location>
<code_context>
}
func (a *Audio) setMono(enable bool) error {
+ if a.Mono == enable {
+ return nil
</code_context>
<issue_to_address>
**suggestion:** setMono always sets the default sink even if Mono state is unchanged.
Consider adding an early return if the Mono state is already set to avoid redundant operations when setting the default sink.
```suggestion
func (a *Audio) setMono(enable bool) error {
if a.Mono == enable {
// Mono state is already set, avoid redundant operations
return nil
}
```
</issue_to_address>
### Comment 4
<location> `audio1/card.go:132` </location>
<code_context>
+}
+
+// sortPortsByPriority 根据端口的 Priority 属性排序,Priority 值小的在前(由低到高)
+func (c *Card) sortPortsByPriority(card *pulse.Card) {
+ sort.SliceStable(c.Ports, func(i, j int) bool {
+ return c.Ports[i].Priority < c.Ports[j].Priority
</code_context>
<issue_to_address>
**question (bug_risk):** sortPortsByPriority sorts ports by Priority, but Priority values may not be unique or comparable across types.
If Priority values are not unique or not designed for cross-type comparison, the sort may produce inconsistent results. Please review how Priority is assigned and used in sorting.
</issue_to_address>
### Comment 5
<location> `audio1/audio_events.go:365` </location>
<code_context>
}
-func (a *Audio) handleCardChanged(idx uint32) {
+func (a *Audio) handleCardChanged(idx uint32) (changed bool) {
// 数据更新在refreshSinks中统一处理,这里只做业务逻辑上的响应
logger.Debugf("card %d changed", idx)
</code_context>
<issue_to_address>
**issue:** handleCardChanged returns true if no change is detected, which may be counterintuitive.
The function currently returns true when no change is detected, which may mislead callers. Consider updating the logic so that true indicates a change.
</issue_to_address>
### Comment 6
<location> `audio1/audio_events.go:772` </location>
<code_context>
- // oldCard不存在
- logger.Warning(err)
- }
+func (a *Audio) notifyCardPortInsert(card *Card) {
+ logger.Debugf("notify card %d:%s", card.Id, card.core.Name)
</code_context>
<issue_to_address>
**suggestion:** notifyCardPortInsert no longer compares with oldCard, so port insertion logic may be less accurate.
Currently, notifications are sent for all enabled ports, which may cause duplicates. To notify only on new insertions, consider reintroducing the previous card state comparison.
Suggested implementation:
```golang
func (a *Audio) notifyCardPortInsert(card *Card, oldCard *Card) {
logger.Debugf("notify card %d:%s", card.Id, card.core.Name)
// Compare ports between oldCard and card, notify only for new insertions
newPorts := make(map[string]*Port)
if card != nil {
for _, port := range card.Ports {
newPorts[port.Name] = port
}
}
var oldPorts map[string]*Port
if oldCard != nil {
oldPorts = make(map[string]*Port)
for _, port := range oldCard.Ports {
oldPorts[port.Name] = port
}
} else {
oldPorts = make(map[string]*Port)
}
for name, port := range newPorts {
if _, existed := oldPorts[name]; !existed && port.Enabled {
// Only notify for newly inserted and enabled ports
logger.Infof("Port inserted: %s on card %d", name, card.Id)
// Place notification logic here (e.g., send event, callback, etc.)
}
}
```
You will need to:
1. Pass `oldCard` as an argument to `notifyCardPortInsert` wherever it is called.
2. Ensure that the previous card state is tracked and available at the call site.
3. Replace the placeholder notification logic with your actual notification/event code.
</issue_to_address>
### Comment 7
<location> `audio1/priority_manager.go:165` </location>
<code_context>
- pm.defaultInit(cards)
- pm.Save()
- return
+func (pm *PriorityManager) GetTheFirstPort(direction int) (*PriorityPort, *Position) {
+ switch direction {
+ case pulse.DirectionSink:
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated switch logic for direction-to-policy mapping into a helper method to simplify related methods.
```suggestion
// extract this helper to remove repeated switch logic
func (pm *PriorityManager) policyByDirection(dir int) *PriorityPolicy {
switch dir {
case pulse.DirectionSink:
return pm.Output
case pulse.DirectionSource:
return pm.Input
default:
return nil
}
}
var invalidPos = &Position{tp: PortTypeInvalid, index: -1}
// now GetTheFirstPort, SetTheFirstPort, GetNextPort can share a single pattern:
func (pm *PriorityManager) GetTheFirstPort(dir int) (*PriorityPort, *Position) {
pol := pm.policyByDirection(dir)
if pol == nil || len(pol.Ports) == 0 {
return nil, invalidPos
}
return pol.GetTheFirstPort(pm.availablePort)
}
func (pm *PriorityManager) SetTheFirstPort(card, port string, dir int) {
if pol := pm.policyByDirection(dir); pol != nil {
pol.SetTheFirstPort(card, port, pm.availablePort)
} else {
logger.Warningf("unexpected direction %d", dir)
}
pm.Print()
pm.Save()
}
func (pm *PriorityManager) GetNextPort(dir int, pos *Position) (*PriorityPort, *Position) {
pol := pm.policyByDirection(dir)
if pol == nil || len(pol.Ports) == 0 {
return nil, invalidPos
}
return pol.GetNextPort(pm.availablePort, pos)
}
```
- Removes duplicated `switch` blocks in three methods.
- Centralizes direction→policy logic.
- Keeps all existing functionality intact.
</issue_to_address>
### Comment 8
<location> `audio1/audio_switch.go:30` </location>
<code_context>
+// 因为dde的状态需要通过pulse事件同步,这个是有滞后性的,会有一个中间状态,上层未刷新信息,pulse的状态已经变化了。
+// 应该以收到的事件为准来更新dde音频的内存信息。
+// 只有dde内存信息刷新后,才算声卡准备完成。
+func (a *Audio) checkCardIsReady(id uint32) bool {
+ card := a.getCardById(id)
+ if card == nil {
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the port and device readiness logic into helper methods to simplify and clarify checkCardIsReady.
```go
// audio/card_helpers.go
func (c *Card) availablePorts() strv.Strv {
if c.ActiveProfile == nil {
return nil
}
var ap strv.Strv
for _, p := range c.Ports {
if p.Profiles.Exists(c.ActiveProfile.Name) {
ap = append(ap, p.Name)
}
}
return ap
}
func (c *Card) hasDirections() (hasSink, hasSource bool) {
for _, p := range c.Ports {
if !p.Profiles.Exists(c.ActiveProfile.Name) {
continue
}
switch p.Direction {
case pulse.DirectionSink:
hasSink = true
case pulse.DirectionSource:
hasSource = true
}
}
return
}
// audio/audio_helpers.go
func (a *Audio) deviceReady[T interface {
Card uint32
ActivePort struct{ Name string }
Ports []any
}](devices []T, cardID uint32, ap strv.Strv) bool {
for _, dev := range devices {
if dev.Card != cardID || len(dev.Ports) == 0 {
continue
}
if ap.Contains(dev.ActivePort.Name) {
return true
}
}
return false
}
// audio/check_ready.go (refactored)
func (a *Audio) checkCardIsReady(id uint32) bool {
card := a.getCardById(id)
if card == nil || len(card.Profiles) == 0 {
return true
}
if card.ActiveProfile == nil {
return false
}
ap := card.availablePorts()
hasSink, hasSource := card.hasDirections()
sinkOk := !hasSink || a.deviceReady(a.sinks, id, ap)
sourceOk := !hasSource || a.deviceReady(a.sources, id, ap)
return sinkOk && sourceOk
}
```
- Extracted the port‐filtering loop into `Card.availablePorts()`
- Pulled out sink/source detection into `Card.hasDirections()`
- Unified the sink/source loops into a single generic `deviceReady`
- Reduced `checkCardIsReady` to clear, linear logic without nested loops or flags.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // 将指定类型的优先级设为最高,此函数不会提高该类型的端口的优先级 | ||
| func (pp *PriorityPolicy) SetTheFirstType(portType int) bool { | ||
| if portType < 0 || portType >= PortTypeCount { | ||
| func (pp *PriorityPolicy) SetTheFirstPort(cardName string, portName string, available portList) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): SetTheFirstPort does not update the Types priority order.
Currently, moving a port to the front of its type's list does not adjust the Types slice, so the port's type may not have the highest priority. Please update Types to move the port's type to the front when SetTheFirstPort is called.
Suggested implementation:
func (pp *PriorityPolicy) SetTheFirstPort(cardName string, portName string, available portList) bool {
if !available.isExists(cardName, portName) {
logger.Warningf("port <%v:%v> not avaiable", cardName, portName)
return false
}
// 第一步:找到并删除指定的端口
var targetPort *PriorityPort
var targetType int // 第一步:找到并删除指定的端口
var targetPort *PriorityPort
var targetType int
// 查找端口类型
for t, ports := range pp.Ports {
for i, p := range ports {
if p.CardName == cardName && p.PortName == portName {
targetPort = &pp.Ports[t][i]
targetType = t
break
}
}
if targetPort != nil {
break
}
}
if targetPort == nil {
logger.Warningf("port <%v:%v> not found in PriorityPolicy", cardName, portName)
return false
}
// 将端口移到其类型列表的最前面
ports := pp.Ports[targetType]
var newPorts []PriorityPort
for _, p := range ports {
if p.CardName == cardName && p.PortName == portName {
newPorts = append([]PriorityPort{p}, newPorts...)
} else {
newPorts = append(newPorts, p)
}
}
pp.Ports[targetType] = newPorts
// 将端口类型移到Types的最前面
var newTypes []int
newTypes = append(newTypes, targetType)
for _, t := range pp.Types {
if t != targetType {
newTypes = append(newTypes, t)
}
}
pp.Types = newTypes
audio1/audio.go
Outdated
| a.context().LoadModule("module-null-sink", "") | ||
| } | ||
|
|
||
| func (a *Audio) setReduceNoise(enable bool) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): setReduceNoise does not check for Bluetooth devices before enabling noise reduction.
Add a check to prevent enabling noise reduction when the audio source is Bluetooth, as Bluetooth devices are not compatible with this feature.
| func (a *Audio) setMono(enable bool) error { | ||
| if a.Mono == enable { | ||
| return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: setMono always sets the default sink even if Mono state is unchanged.
Consider adding an early return if the Mono state is already set to avoid redundant operations when setting the default sink.
| func (a *Audio) setMono(enable bool) error { | |
| if a.Mono == enable { | |
| return nil | |
| func (a *Audio) setMono(enable bool) error { | |
| if a.Mono == enable { | |
| // Mono state is already set, avoid redundant operations | |
| return nil | |
| } |
| } | ||
|
|
||
| // sortPortsByPriority 根据端口的 Priority 属性排序,Priority 值小的在前(由低到高) | ||
| func (c *Card) sortPortsByPriority(card *pulse.Card) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (bug_risk): sortPortsByPriority sorts ports by Priority, but Priority values may not be unique or comparable across types.
If Priority values are not unique or not designed for cross-type comparison, the sort may produce inconsistent results. Please review how Priority is assigned and used in sorting.
| } | ||
|
|
||
| func (a *Audio) handleCardChanged(idx uint32) { | ||
| func (a *Audio) handleCardChanged(idx uint32) (changed bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: handleCardChanged returns true if no change is detected, which may be counterintuitive.
The function currently returns true when no change is detected, which may mislead callers. Consider updating the logic so that true indicates a change.
a3ed57e to
bf1fda8
Compare
基于音频pulse框架事件,作出如下优化: 1. 优化了优选配置,简化了音频端口优先算法 2. 重构了单声道配置逻辑 3. 重构了噪音抑制的配置逻辑 4. 优化了音频初始化 5. 简化了音频逻辑代码,删除了无用逻辑 PMS: https://pms.uniontech.com/bug-view-338471.html https://pms.uniontech.com/bug-view-338379.html https://pms.uniontech.com/bug-view-338267.html https://pms.uniontech.com/bug-view-338139.html https://pms.uniontech.com/bug-view-335547.html https://pms.uniontech.com/bug-view-331027.html https://pms.uniontech.com/bug-view-307827.html https://pms.uniontech.com/bug-view-300797.html https://pms.uniontech.com/bug-view-298977.html https://pms.uniontech.com/bug-view-297349.html https://pms.uniontech.com/bug-view-294205.html https://pms.uniontech.com/bug-view-292313.html https://pms.uniontech.com/bug-view-271629.html Influence: audio
bf1fda8 to
49f4d54
Compare
deepin pr auto review我来对这个diff进行代码审查。这是一个音频模块的代码变更,主要涉及降噪、端口切换、声卡管理等功能。我将从语法逻辑、代码质量、性能和安全四个方面进行分析。
建议改进:
建议改进:
建议改进:
建议改进:
具体改进建议:
func (a *Audio) setReduceNoise(source string, enable bool) error {
if source == "" {
return fmt.Errorf("source cannot be empty")
}
if isBluezAudio(source) {
logger.Debug("bluetooth audio device cannot open reduce-noise")
return nil
}
// ... 其余代码
}
func (a *Audio) checkPortAvailable(card *Card, portName string, direction int) bool {
port, err := card.Ports.Get(portName, direction)
if err != nil {
return false
}
return port.Available != pulse.AvailableTypeNo
}
type portInfo struct {
name string
cardName string
}
func (a *Audio) getPortInfoList() []portInfo {
var ports []portInfo
for _, sink := range a.sinks {
ports = append(ports, portInfo{
name: sink.Name,
cardName: a.getCardNameById(sink.Card),
})
}
return ports
}
func validateCommandArgs(args ...string) error {
for _, arg := range args {
if strings.Contains(arg, ";") || strings.Contains(arg, "&") {
return fmt.Errorf("invalid command argument: %s", arg)
}
}
return nil
}
func (a *Audio) safeExecCommand(cmd string, args ...string) error {
if err := validateCommandArgs(args...); err != nil {
return err
}
out, err := exec.Command(cmd, args...).CombinedOutput()
if err != nil {
return fmt.Errorf("%v: %s", err, string(out))
}
return nil
}
func (a *Audio) validateConfigPath(path string) error {
info, err := os.Stat(path)
if err != nil {
return err
}
if info.Mode().Perm()&0222 != 0 {
return fmt.Errorf("config file %s is writable by others", path)
}
return nil
}这些改进可以提高代码的健壮性、安全性和可维护性。建议在后续版本中逐步实施这些改进。 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602, robertkill 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 |
Summary by Sourcery
Optimize audio subsystem: refactor port prioritization and auto-switch logic, introduce persistent noise reduction and port Mode support, and streamline mono/noise modules via pulse modules
New Features:
Enhancements:
Tests: