Skip to content

Conversation

@fly602
Copy link
Contributor

@fly602 fly602 commented Oct 27, 2025

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:

  • Add null-sink support for echo-cancel to enable noise suppression and persist user noise reduction settings
  • Introduce a Mode property for audio ports with SetMode/GetMode in ConfigKeeper to handle codec/profile selection
  • Automatically load/unload pulse modules for mono output and echo-cancel noise suppression based on user settings

Enhancements:

  • Refactor PriorityManager to use typed port queues and an available-port map for streamlined auto-switch logic
  • Simplify auto-pause and auto-switch functionality by consolidating port selection through the updated PriorityManager
  • Revamp ConfigKeeper to key configurations by Card object and include new Mode field, unifying getters and setters
  • Consolidate and remove duplicate port-finding and SetPort code paths, improving maintainability
  • Streamline mono sink handling to use pactl module remap-sink under the hood

Tests:

  • Add unit tests for PriorityPolicy.SetTheFirstPort covering various prioritization scenarios
  • Add unit tests for ConfigKeeper's SetMode/GetMode and persistence of port Mode
  • Add unit tests to verify stable sorting of Card ports by priority value

@sourcery-ai
Copy link

sourcery-ai bot commented Oct 27, 2025

Reviewer's Guide

This 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)
Loading

Sequence diagram for auto-switching output port using new PriorityManager

sequenceDiagram
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)
Loading

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)
Loading

ER diagram for CardConfig and PortConfig changes

erDiagram
CardConfig {
  string Name
}
PortConfig {
  string Name
  bool Enabled
  float Volume
  bool IncreaseVolume
  float Balance
  bool ReduceNoise
  string Mode
}
CardConfig ||--o{ PortConfig : has
Loading

Class diagram for updated PriorityManager and PriorityPolicy

classDiagram
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
Loading

Class diagram for updated ConfigKeeper and Card/PortConfig

classDiagram
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
Loading

File-Level Changes

Change Details Files
Introduce persistent reduce-noise and null-sink handling
  • Add dsgKeyDefaultReduceNoise and dsgKeyReduceNoiseEnabled keys
  • Detect and load module-null-sink when missing
  • Implement Audio.setReduceNoise with DConfig persistence
  • Add echo-cancel-source handling
  • Update mono sink logic to use LoadNullSinkModule
audio1/audio.go
audio1/source.go
audio1/sink.go
Refactor Audio port lookups and auto-switch logic
  • Remove oldCards tracking
  • Replace find*ByCardIndexPortName with findSink/findSource
  • Use PausePlayer flag in autoPause
  • Use PriorityManager.GetTheFirstPort/GetNextPort for port selection
  • Simplify SetPort, SetPortEnabled and related handlers
audio1/audio.go
audio1/audio_events.go
audio1/audio_switch.go
Redesign priority_policy with type-based queues
  • Change Ports from list to map[type][]PriorityPort
  • Introduce Position struct
  • Implement InsertPort, GetTheFirstPort, GetNextPort, SetTheFirstPort
  • Remove unstable bubble-sort and old APIs
  • Add comprehensive SetTheFirstPort unit tests
audio1/priority_policy.go
audio1/priority_test.go
Simplify priority_manager to track available ports
  • Maintain availablePort map of card→ports
  • Unify Init and refreshPorts logic
  • Use new PriorityPolicy InsertPort and SetTheFirstPort
  • Expose GetTheFirstPort/GetNextPort
audio1/priority_manager.go
Simplify ConfigKeeper and add Mode support
  • Use Card object as key instead of string
  • Remove PreferPort/PreferProfile fields
  • Add PortConfig.Mode, SetMode/GetMode and persistence
  • Update SetVolume/Balance/IncreaseVolume/ReduceNoise to use Card
  • Add ConfigKeeper mode tests
audio1/config_keeper.go
audio1/config_keeper_test.go
Enhance Card port sorting
  • Remove filterProfile parameter
  • Add sortPortsByPriority stable sort by Priority
  • Adjust update to call sortPortsByPriority
audio1/card.go
audio1/card_test.go
Introduce checkCardIsReady for reliable port switching
  • Add audio_switch.go with getCardById and checkCardIsReady
  • Use checkCardIsReady in event handlers before autoSwitch
audio1/audio_switch.go

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a 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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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 {
Copy link

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 {
Copy link

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.

Comment on lines 2106 to +1907
func (a *Audio) setMono(enable bool) error {
if a.Mono == enable {
return nil
Copy link

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.

Suggested change
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) {
Copy link

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) {
Copy link

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.

@fly602 fly602 force-pushed the optimize-audio branch 8 times, most recently from a3ed57e to bf1fda8 Compare October 29, 2025 10:08
@deepin-ci-robot
Copy link

deepin pr auto review

我来对这个diff进行代码审查。这是一个音频模块的代码变更,主要涉及降噪、端口切换、声卡管理等功能。我将从语法逻辑、代码质量、性能和安全四个方面进行分析。

  1. 语法逻辑方面:
  • 代码整体结构清晰,函数命名规范
  • 错误处理完善,大部分函数都有适当的错误处理
  • 使用了适当的并发控制(sync.Mutex)
  • 类型定义和常量定义合理

建议改进:

  • 在setReduceNoise函数中,对source为空的情况处理不够完善,应该返回明确的错误
  • 部分函数的注释不够详细,特别是新增的函数如LoadNullSinkModule等
  1. 代码质量方面:
    优点:
  • 代码组织良好,相关功能被合理分组到不同文件中
  • 使用了适当的日志记录
  • 配置管理统一,使用了ConfigKeeper

建议改进:

  • 部分函数过长,如autoSwitchOutputPort和autoSwitchInputPort,建议拆分
  • 一些重复代码可以提取为公共函数,如端口检查逻辑
  • 某些变量命名可以更具描述性,如misc
  1. 代码性能方面:
    优点:
  • 使用了map等高效数据结构
  • 避免了不必要的循环嵌套

建议改进:

  • 在handleCardChanged中,可以使用更高效的方式比较端口变化
  • refreshDefaultSinkSource中的多次字符串比较可以优化
  • 可以考虑使用对象池来减少内存分配
  1. 代码安全方面:
    优点:
  • 对外部输入进行了适当的验证
  • 使用了适当的权限控制

建议改进:

  • exec.Command的使用需要更谨慎,建议对命令参数进行验证
  • 配置文件的读写需要加强权限检查
  • 某些敏感操作需要添加额外的安全检查

具体改进建议:

  1. setReduceNoise函数改进:
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
    }
    // ... 其余代码
}
  1. 端口检查逻辑提取:
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
}
  1. 优化字符串比较:
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
}
  1. 加强exec.Command的安全性:
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
}
  1. 配置文件安全检查:
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
}

这些改进可以提高代码的健壮性、安全性和可维护性。建议在后续版本中逐步实施这些改进。

@deepin-ci-robot
Copy link

[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.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fly602 fly602 merged commit 2fad281 into linuxdeepin:master Oct 30, 2025
13 of 16 checks passed
@fly602 fly602 deleted the optimize-audio branch November 7, 2025 03:12
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.

3 participants