Skip to content

[pull] master from MusicPlayerDaemon:master #32

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

Merged
merged 7 commits into from
Jun 2, 2025

Conversation

pull[bot]
Copy link

@pull pull bot commented Jun 2, 2025

See Commits and Changes for more details.


Created by pull[bot] (v2.0.0-alpha.1)

Can you help keep this open source service alive? 💖 Please sponsor : )

Summary by Sourcery

Improve PipeWire output plugin with proper drain handling and safer buffer operations, enhance ring buffer API, bump game-music-emu dependency and modernize build settings, and fix minor typos in client messaging.

Bug Fixes:

  • Correct typo in client log and protocol error messages.

Enhancements:

  • Flush PipeWire stream immediately when drain is requested and ring buffer is empty.
  • Replace raw buffer pointers with std::span in PipeWireOutput for safer buffer handling.
  • Add an IsEmptyRelaxed() method to the ring buffer for thread-safe emptiness checks.
  • Upgrade game-music-emu to version 0.6.4 and disable its tests, examples, and zlib integration.
  • Set the gtest C++ standard to C++23 in the Meson build configuration.

Build:

  • Update python/build/libs.py to pull game-music-emu 0.6.4 with revised CMake build flags.

fschlich and others added 7 commits May 25, 2025 19:18
protcol -> protocol

comand -> command

Found by Debian's lintian spellchecker
…" mode

This fixes another cause for the "single" mode drain/resume bug.  The
first (generic) cause was fixed by commit 9ab928a, and this is
the PipeWire-specific cause for another bug with the same symptoms:

When draining the PipeWire output at the end of a song in "single"
mode, PipeWireOutput::Process() returns without queueing the buffer
that was allocated by pw_stream_dequeue_buffer().  This leaves
libpipewire stuck and playback cannot be resumed.

The solution is to move the `drain_requested` check and the
pw_stream_flush() to the beginning of the method before the
pw_stream_dequeue_buffer() call.  That way, we can return without
still owning a buffer that we will never give back.

Closes #2315
@pull pull bot added the ⤵️ pull label Jun 2, 2025
@pull pull bot merged commit daaea3f into CartoonFan:master Jun 2, 2025
1 check was pending
Copy link

sourcery-ai bot commented Jun 2, 2025

Reviewer's Guide

This PR pulls upstream changes, enhancing the PipeWire output plugin with drain and buffer-safety improvements, adds a relaxed empty check to RingBuffer, bumps the game-music-emu dependency and CMake flags, updates Meson build settings, and fixes minor typos.

Sequence Diagram for Drain Request Handling in PipeWireOutput

sequenceDiagram
    participant C as Caller
    participant PWO as PipeWireOutput
    participant RB as ring_buffer
    participant PWS as pw_stream

    C->>PWO: Process()
    PWO->>PWO: check drain_requested
    alt drain_requested is true
        PWO->>RB: IsEmptyRelaxed()
        RB-->>PWO: isEmpty
        alt isEmpty is true
            PWO->>PWS: pw_stream_flush(stream, true)
            PWO-->>C: return
        end
    end
    Note over PWO: Continues if not drained or not empty
    PWO-->>C: (continues processing or returns)
Loading

Sequence Diagram for Buffer Underrun Handling in PipeWireOutput

sequenceDiagram
    participant C as Caller
    participant PWO as PipeWireOutput
    participant RB as ring_buffer
    participant PWS as pw_stream

    C->>PWO: Process()
    Note over PWO: Assuming not handling drain as per previous diagram
    PWO->>PWS: pw_stream_dequeue_buffer(stream)
    PWS-->>PWO: buffer b
    alt buffer b is not null and dest.data() is not null
        PWO->>PWO: Prepare dest from buffer b
        PWO->>RB: ReadFramesTo(dest, chunk_size)
        RB-->>PWO: nbytes
        alt nbytes == 0 (Buffer Underrun)
            PWO->>PWO: PcmSilence(dest.first(nbytes), sample_format)
            PWO->>PWO: LogWarning("Decoder is too slow...")
        end
        PWO->>PWS: pw_stream_queue_buffer(stream, b)
    else buffer b is null or dest.data() is null
        PWO->>PWO: Log warning or return
    end
    PWO-->>C: (return)
Loading

Class Diagram for PipeWireOutput and RingBuffer Updates

classDiagram
    class PipeWireOutput {
        +Process() : void
    }

    class RingBuffer {
        +IsEmptyRelaxed() : bool
    }

    PipeWireOutput ..> RingBuffer : uses
Loading

File-Level Changes

Change Details Files
Enhanced stream draining and buffer management in PipeWireOutputPlugin
  • Added early flush when drain_requested and buffer empty
  • Replaced raw dest pointer with std::span for buffer data
  • Streamlined ReadFramesTo to accept span directly
  • Updated silence generation via span.first(nbytes)
  • Adjusted PostProcessDsd to use span data pointer
src/output/plugins/PipeWireOutputPlugin.cxx
Introduced relaxed empty check in RingBuffer
  • Added IsEmptyRelaxed() using relaxed atomics
  • Documented thread-safe consumer-thread usage
src/util/RingBuffer.hxx
Updated game-music-emu dependency to 0.6.4 with refined CMake flags
  • Changed source URL and checksum for v0.6.4
  • Disabled testing, examples, and zlib in CMake args
  • Adjusted base parameter to new tag directory
python/build/libs.py
Fixed minor typos in client and command processing
  • Corrected 'comand' to 'command' in ProcessLine warning
  • Fixed 'protcol' to 'protocol' in ProtocolError message
src/client/Process.cxx
src/command/ClientCommands.cxx
Extended Meson build config for gtest C++ standard
  • Set gtest:cpp_std to c++23
meson.build

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

guardrails bot commented Jun 2, 2025

⚠️ We detected 2 security issues in this pull request:

Hard-Coded Secrets (2)
Severity Details Docs
Medium Title: Hex High Entropy String
source_hash = f196539ae448ecb304f05598d8614b125ace159a63a980dca691e066250d7fa2
📚
Medium Title: Hex High Entropy String
patch_hash = f2b8dd45f803dffb27bb771785a23ed3d1d28159f7d440a3db64b9df02d01658
📚

More info on how to fix Hard-Coded Secrets in General.


👉 Go to the dashboard for detailed results.

📥 Happy? Share your feedback with us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants