Repository navigation
Build fpcalc against the system FFmpeg in CI - #165
Conversation
The test jobs pass BUILD_TOOLS=OFF, so the FFmpeg audio reader is never compiled there, and the packaging jobs build it against a pinned FFmpeg 8.0. Nothing covered the combination distributions actually use: tools enabled, system FFmpeg. That left the FFmpeg detection in CMakeLists.txt and FindFFmpeg.cmake untested for older releases. Add a job that builds with BUILD_TOOLS=ON against the FFmpeg that ubuntu-22.04 ships, runs the test suite, and checks fpcalc still produces the reference fingerprint. It prints the FFmpeg package versions first so a failure says which ones it was built against.
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughChangesFFmpeg compatibility and Linux tools validation
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yml:
- Around line 36-39: Add a job-level permissions block to test-linux-tools
granting only contents: read for its actions/checkout step, leaving all other
token permissions disabled.
- Line 39: Update the checkout step using the actions/checkout symbol to
reference a full immutable commit SHA instead of the mutable v3 tag, and set
persist-credentials to false. Keep the workflow’s existing checkout behavior
unchanged otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 07f94b5b-1d12-41d8-a52c-cb29d7165ce7
📒 Files selected for processing (1)
.github/workflows/build.yml
| test-linux-tools: | ||
| runs-on: ubuntu-22.04 | ||
| steps: | ||
| - uses: actions/checkout@v3 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow files =="
git ls-files '.github/workflows/*' || true
echo "== build.yml around referenced job =="
sed -n '1,140p' .github/workflows/build.yml 2>/dev/null || true
echo "== all permissions blocks in build.yml =="
rg -n '^\s*permissions:' .github/workflows/build.yml || true
echo "== checkout v3 docs/security? local metadata if present? =="
git ls-files | rg 'actions/.*checkout|\.gitmodules|actionlint|tflint|github-actions' || true
echo "== local workflow syntax/tooling evidence =="
git ls-files | rg '(^|/)(actionlint|gha|github-actions|github-action|\.github)' || trueRepository: acoustid/chromaprint
Length of output: 4532
🌐 Web query:
GitHub Actions workflow syntax permissions writes contents read GITHUB_TOKEN default permissions
💡 Result:
In GitHub Actions, the permissions granted to the GITHUB_TOKEN are determined by a hierarchical configuration that starts with default settings at the enterprise, organization, or repository level and can be refined within the workflow file using the permissions key [1][2]. Key points regarding permissions and syntax: Default Behavior: The GITHUB_TOKEN permissions are initially set based on the configuration of the parent enterprise, organization, or repository [1][2]. As of early 2023, GitHub shifted toward a read-only default for new enterprises, organizations, and repositories to enhance security [3]. Workflow Syntax: You can use the permissions key to modify the GITHUB_TOKEN permissions at either the workflow level or for individual jobs [1][4]. - If you specify access for any permission, all other permissions not explicitly listed are automatically set to none [1][2]. - Available access levels are read, write, and none [1][2]. Note that write access inherently includes read access [1][2]. Security Adjustments: - Workflow configuration overrides the repository/organization/enterprise defaults [1][2]. - Regardless of the configuration, if a workflow is triggered by a pull request from a forked repository (and the Send write tokens to workflows from pull requests setting is disabled), the GITHUB_TOKEN is automatically downgraded to read-only for security reasons [1][2]. - It is a recommended security practice to use the principle of least privilege, explicitly defining only the minimum permissions necessary for your workflow [4].
Citations:
- 1: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax
- 2: https://docs.github.com/en/enterprise-cloud@latest/actions/reference/workflows-and-actions/workflow-syntax
- 3: https://github.blog/changelog/2023-02-02-github-actions-updating-the-default-github_token-permissions-to-read-only/
- 4: https://docs.github.com/en/actions/tutorials/authenticate-with-github_token
🌐 Web query:
actions checkout v3 metadata permissions contents read required
💡 Result:
Yes, the contents: read permission is required for the actions/checkout action to function correctly when using the default GITHUB_TOKEN [1][2][3]. In GitHub Actions, if you explicitly define the permissions block in your workflow file, all unspecified permissions default to none [1][4]. Because actions/checkout requires access to the repository's contents to perform a clone, failing to grant contents: read will cause the action to fail with an error (typically indicating that repository access was not granted) [1]. It is recommended to explicitly set the following in your workflow if you are restricting permissions: permissions: contents: read This requirement applies unless you are using an alternative authentication method, such as a Personal Access Token (PAT) or an SSH key provided via the token or ssh-key inputs, which bypass the GITHUB_TOKEN constraints [2][3]. Note that for public repositories, GITHUB_TOKEN generally has read access by default, but for private and internal repositories, explicit configuration is necessary if the permissions block is used [1].
Citations:
- 1: https://mickeygousset.com/posts/github-actions-checkout-fails-with-two-possible-error-messages/
- 2: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 3: https://github.com/marketplace/actions/checkout
- 4: https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/
Declare the required GITHUB_TOKEN permissions explicitly.
This job only needs actions/checkout access, so add a job-level permissions block granting contents: read. Once any permissions is declared, other permissions default to none, keeping the token scope to the workflow’s actual needs.
Proposed fix
test-linux-tools:
+ permissions:
+ contents: read
runs-on: ubuntu-22.04📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test-linux-tools: | |
| runs-on: ubuntu-22.04 | |
| steps: | |
| - uses: actions/checkout@v3 | |
| test-linux-tools: | |
| permissions: | |
| contents: read | |
| runs-on: ubuntu-22.04 | |
| steps: | |
| - uses: actions/checkout@v3 |
🧰 Tools
🪛 actionlint (1.7.12)
[error] 39-39: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 zizmor (1.26.1)
[warning] 39-39: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 36-54: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 39-39: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 36-36: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build.yml around lines 36 - 39, Add a job-level
permissions block to test-linux-tools granting only contents: read for its
actions/checkout step, leaving all other token permissions disabled.
Sources: MCP tools, Linters/SAST tools
| test-linux-tools: | ||
| runs-on: ubuntu-22.04 | ||
| steps: | ||
| - uses: actions/checkout@v3 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
if rg -nP 'uses:\s*actions/checkout@(v\d+|main|master)' .github/workflows; then
echo "Mutable checkout reference found"
exit 1
fiRepository: acoustid/chromaprint
Length of output: 701
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== workflow relevant sections =="
sed -n '1,220p' .github/workflows/build.yml | nl -ba | sed -n '1,220p'
echo
echo "== GitHub workflows checkout references summary =="
python3 - <<'PY'
import re
from pathlib import Path
p=Path('.github/workflows/build.yml')
for i,l in enumerate(p.read_text().splitlines(),1):
if re.search(r'uses:\s*actions/checkout@(v\d+|main|master|[\w-]+)', l):
print(f"{i}:{l}")
PY
echo
echo "== action metadata if present =="
fd -a 'action.yml|action.yaml' . | sed 's#^\./##' | while read -r f; do
printf "\n-- %s --\n" "$f"
sed -n '1,220p' "$f"
doneRepository: acoustid/chromaprint
Length of output: 231
🌐 Web query:
actions/checkout v3 persist-credentials default true README
💡 Result:
The persist-credentials input for the actions/checkout action has a default value of true [1][2][3]. This setting configures the repository to use the GitHub Actions authentication token (or SSH key) in the local Git configuration [2][3], allowing your workflow scripts to run authenticated Git commands (such as git push or git fetch) without needing to manually configure additional credentials [2][3]. If you wish to opt out of this behavior—for instance, to prevent the token from being persisted for security reasons—you can explicitly set persist-credentials: false in your workflow step [2][3]. It is worth noting that while the default remains true across major versions, security improvements have been implemented over time. In later versions (specifically starting with v6), the action was updated to persist these credentials to a separate, isolated configuration file using Git's includeIf mechanism, rather than writing them directly to the repository's primary .git/config file [4][5][6]. This change was designed to improve security by isolating the credentials [5][6]. Discussions regarding changing the default value to false to further reduce potential risk have occurred in the repository, but the default remains true [7][8].
Citations:
- 1: https://github.com/actions/checkout/blob/34e114876b0b11c390a56381ad16ebd13914f8d5/action.yml
- 2: https://github.com/marketplace/actions/checkout?version=v3.6.0
- 3: https://github.com/actions/checkout/blob/2541b1294d2704b0964813337f33b291d3f8596b/README.md
- 4: actions/checkout@v5.0.1...v6.0.0
- 5: Persist creds to a separate file actions/checkout#2286
- 6: https://github.com/actions/checkout/pull/2286/files
- 7: [security] Escalate concerning default
persist-credentials=trueplease actions/checkout#2312 - 8: Change the default value of persist-credentials to false actions/checkout#1687
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== .github/workflows/build.yml checkout steps =="
python3 - <<'PY'
from pathlib import Path
for i, line in enumerate(Path('.github/workflows/build.yml').read_text().splitlines(), 1):
if 'actions/checkout' in line or 'persist-credentials' in line:
print(f'{i}: {line}')
PY
echo
echo "== workflow permissions and checkout steps =="
python3 - <<'PY'
from pathlib import Path
lines = Path('.github/workflows/build.yml').read_text().splitlines()
for i, line in enumerate(lines, 1):
if line.strip().startswith('permissions:') or line.strip().startswith('- uses:'):
print(f'{i}: {line}')
PY
echo
echo "== search for other checkout pins =="
python3 - <<'PY'
import re
from pathlib import Path
for path in Path('.github/workflows').iterdir():
if path.is_file():
for i, line in enumerate(path.read_text().splitlines(), 1):
m = re.search(r'uses:\s*actions/checkout@([^ #\s\n]+)', line)
if m:
ref = m.group(1)
print(f'{path}:{i}: {ref}')
PYRepository: acoustid/chromaprint
Length of output: 1316
🌐 Web query:
actions/checkout releases v3 latest full commit SHA
💡 Result:
The latest release of the actions/checkout v3 branch is v3.7.0, which was published on July 20, 2026 [1][2]. The commit SHA associated with the v3.7.0 tag is 8e5e7e5ab8b370d6c329ec480221332ada57f0ab [1]. Please note that as of July 28, 2026, the actions/checkout repository has newer major versions available, such as v4, v5, v6, and v7 [2]. It is generally recommended to use the latest stable version of the action [3].
Citations:
- 1: https://github.com/actions/checkout/releases/tag/v3.7.0
- 2: https://github.com/actions/checkout/releases
- 3: https://github.com/actions/checkout
🌐 Web query:
actions/checkout v3.7.0 latest supported v3 release commit 8e5e7e5ab8b370d6c329ec480221332ada57f0ab
💡 Result:
As of July 28, 2026, actions/checkout v3.7.0 is the latest released version for the v3 major release line [1][2][3]. The specific commit hash 8e5e7e5ab8b370d6c329ec480221332ada57f0ab corresponds to the release of v3.5.2, not v3.7.0 [4][5][6]. The v3.5.2 release was published prior to v3.7.0 [4][3]. The v3.7.0 release is the most recent v3 version, having been released on July 20, 2026 [1]. Users are advised that higher major versions of actions/checkout, such as v7, are currently available and maintained [7][8].
Citations:
- 1: https://github.com/actions/checkout/releases/tag/v3.7.0
- 2: https://github.com/actions/checkout/releases
- 3: https://releasealert.dev/github/actions/checkout
- 4: actions/checkout@ac59398...8e5e7e5
- 5: https://gitea.dev.zamar.aero/actions/checkout/src/commit/8e5e7e5ab8b370d6c329ec480221332ada57f0ab/CHANGELOG.md
- 6: https://git.liteyuki.org/actions/checkout/src/commit/8e5e7e5ab8b370d6c329ec480221332ada57f0ab/CHANGELOG.md
- 7: https://github.com/actions/checkout/?tab=readme-ov-file
- 8: https://github.com/actions/checkout/releases/tag/v7.0.0
Pin checkout and disable persisted credentials.
actions/checkout@v3 is an outdated mutable reference, and persist-credentials defaults to true for build/test steps. Pin the checkout action to a full commit SHA and set persist-credentials: false; this workflow only checks out the repo, so authenticated Git commands are not needed.
Proposed fix
- - uses: actions/checkout@v3
+ - uses: actions/checkout@<reviewed-full-commit-sha>
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v3 | |
| - uses: actions/checkout@<reviewed-full-commit-sha> | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 actionlint (1.7.12)
[error] 39-39: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 zizmor (1.26.1)
[warning] 39-39: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 39-39: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build.yml at line 39, Update the checkout step using the
actions/checkout symbol to reference a full immutable commit SHA instead of the
mutable v3 tag, and set persist-credentials to false. Keep the workflow’s
existing checkout behavior unchanged otherwise.
Sources: MCP tools, Linters/SAST tools
The audio reader uses AVChannelLayout, ch_layout and av_channel_layout_*, all of which arrived in libavutil 57.24.100 (FFmpeg 5.1). Building the tools against anything older fails outright, so despite #158 the 4.2 to 5.0 range that CMake accepts could not actually be compiled. Add ffmpeg_compat.h with the version check and a macro for the channel count, and give both call sites in the reader and both setters in the swresample processor a pre-5.1 path using the bitmask layout, av_get_default_channel_layout and the in_channel_layout/out_channel_layout options. The new test-linux-tools job is what found this, building against the FFmpeg that ubuntu-22.04 ships.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/audio/ffmpeg_audio_reader.h`:
- Around line 207-225: Update the converter configuration used by Read() to
derive input sample format, sample rate, and channel layout from m_frame rather
than only m_codec_ctx. Before converting each frame, compare those properties
with the current FFmpegAudioProcessor configuration and reset/rebuild
m_converter when they change, while preserving the existing output settings and
channel-layout API branches.
- Line 342: Update the av_samples_alloc call in the converter buffer allocation
path to use m_output_channels rather than
CHROMAPRINT_CODEC_CHANNELS(m_codec_ctx), matching the channel count produced by
m_converter->Convert(...). Keep the existing packed S16 format and sample-count
allocation behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a228d104-bded-420a-a8ca-fb6a0c570073
📒 Files selected for processing (3)
src/audio/ffmpeg_audio_processor_swresample.hsrc/audio/ffmpeg_audio_reader.hsrc/audio/ffmpeg_compat.h
| if (m_codec_ctx->sample_fmt != AV_SAMPLE_FMT_S16 || CHROMAPRINT_CODEC_CHANNELS(m_codec_ctx) != m_output_channels || m_codec_ctx->sample_rate != m_output_sample_rate) { | ||
| m_converter.reset(new FFmpegAudioProcessor()); | ||
| m_converter->SetCompatibleMode(); | ||
| m_converter->SetInputSampleFormat(m_codec_ctx->sample_fmt); | ||
| m_converter->SetInputSampleRate(m_codec_ctx->sample_rate); | ||
| #if CHROMAPRINT_HAVE_AV_CHANNEL_LAYOUT | ||
| m_converter->SetInputChannelLayout(&(m_codec_ctx->ch_layout)); | ||
| #else | ||
| m_converter->SetInputChannelLayout(m_codec_ctx->channel_layout | ||
| ? m_codec_ctx->channel_layout | ||
| : av_get_default_channel_layout(m_codec_ctx->channels)); | ||
| #endif | ||
| m_converter->SetOutputSampleFormat(AV_SAMPLE_FMT_S16); | ||
| m_converter->SetOutputSampleRate(m_output_sample_rate); | ||
| #if CHROMAPRINT_HAVE_AV_CHANNEL_LAYOUT | ||
| m_converter->SetOutputChannelLayout(&output_channel_layout); | ||
| #else | ||
| m_converter->SetOutputChannelLayout(output_channel_layout); | ||
| #endif |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate ffmpeg_audio_reader.h =="
fd -a 'ffmpeg_audio_reader\.h$' . || true
echo "== outline =="
file=$(fd 'ffmpeg_audio_reader\.h$' . | head -n1)
if [ -n "${file:-}" ]; then
wc -l "$file"
ast-grep outline "$file" --view compact || true
echo "== relevant sections =="
sed -n '1,280p' "$file" | cat -n
fi
echo "== related symbols =="
rg -n "FFmpegAudioProcessor|FFmpegAudioReader|m_converter|SetInput|SetOutput|Reinitializ|AVRational|Read\\(|Open\\(" -S src include . 2>/dev/null | head -n 200Repository: acoustid/chromaprint
Length of output: 26479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(fd 'ffmpeg_audio_reader\.h$' . | head -n1)
echo "== Read() implementation =="
sed -n '278,370p' "$file" | cat -n
echo "== FFmpegAudioProcessor implementation =="
processor_file=$(fd 'ffmpeg_audio_processor_swresample\.h$|ffmpeg_audio_processor.*\.h$|ffmpeg_audio_processor.*\.cpp$' src/audio | head -n3 | tr '\n' ' ')
for p in $processor_file; do
echo "-- $p --"
wc -l "$p"
sed -n '1,220p' "$p" | cat -n
done
echo "== compat macro definitions =="
fd -a 'ffmpeg_compat\.h$' src/audio | xargs -r -I{} sh -c 'echo "-- {} --"; sed -n "1,140p" "{}" | cat -n'
echo "== FFmpeg channel layout doc extraction from system headers if present =="
if [ -f /usr/include/libavutil/channel_layout.h ]; then
rg -n "channel_layout|ChannelLayout" /usr/include/libavutil/channel_layout.h | head -n 60
fi
echo "== deterministic model: current Read() path vs frame property deltas =="
python3 - <<'PY'
from pathlib import Path
p=list(Path('.').rglob('ffmpeg_audio_reader.h'))[0]
s=p.read_text()
print("Open creates converter from m_codec_ctx:", "m_converter.reset(new FFmpegAudioProcessor())" in s[s.find("FFmpegAudioReader::Open"):s.find("return m_opened = true;")])
print("Read sets input layout from m_frame before using converter:", "SetInputChannelLayout" in s[s.find("FFmpegAudioReader::Read"):s.find("return false;", s.find("FFmpegAudioReader::Read"))] or "m_frame->ch_layout" in s[s.find("FFmpegAudioReader::Read"):s.find("m_converter->Convert")] or "av_channel_layout_compare" in s)
read = s[s.find("FFmpegAudioReader::Read"):s.find("return false;")]
print("Read contains format/rate/layout reinit or compare before Convert:", any(x in read for x in ["Reinit", "avcodec_reinit2", "av_frame_side_data_create", "Convert", "Flush"]))
PYRepository: acoustid/chromaprint
Length of output: 8557
🌐 Web query:
FFmpeg AVCodecContext channels sample_rate ch_layout decoder may override context frame input samples
💡 Result:
In FFmpeg, the AVCodecContext fields 'ch_layout' (which replaced the deprecated 'channel_layout') and 'sample_rate' are used to describe the audio stream's configuration [1][2][3]. When decoding, these fields may be set by the caller (e.g., initialized from container metadata) [1][2]. However, the decoder is permitted to override these values during the decoding process if the actual stream properties differ from the initial assumptions or if the decoder dynamically updates its state based on the input packets [1][2][3]. Key points regarding this behavior: 1. Dynamic Updates: Decoders often perform this override to reflect the true stream parameters discovered during parsing or decoding of the bitstream [1][2]. 2. Side Data: Libavcodec also handles parameter changes during decoding via AV_SIDE_DATA_PARAM_CHANGE events [4][5]. If a packet contains side data indicating a change in channel count, channel layout, or sample rate, the decoder may update the AVCodecContext accordingly [4][5]. 3. Frame Propagation: When a decoder produces a decoded frame, it populates the AVFrame with the current audio properties (like 'ch_layout' and 'sample_rate') [4][5]. Applications should generally rely on the parameters attached to the output AVFrame [4][5]. 4. API Usage: The 'ch_layout' field (using the AVChannelLayout structure) is the modern, preferred API [3]. Older code using 'channel_layout' is deprecated [3]. In summary, the AVCodecContext fields in the decoder context represent a best-effort configuration that can be modified by the codec during operation; therefore, robust applications should read the actual format information from the output frames rather than assuming the values in the AVCodecContext remain static [4][5].
Citations:
- 1: https://ffmpeg.org/doxygen/7.1/structAVCodecContext.html
- 2: https://ffmpeg.org/doxygen/trunk/structAVCodecContext.html
- 3: FFmpeg/FFmpeg@548aeb9
- 4: https://github.com/FFmpeg/FFmpeg/blob/6161eacc748e255fee5af0e1d56845afc27b2e7f/libavcodec/decode.c
- 5: https://ffmpeg.org/doxygen/6.1/decode_8c_source.html
Reconfigure the resampler from m_frame audio properties.
Open() builds FFmpegAudioProcessor with m_codec_ctx, but decoded frames can carry the actual sample_fmt, sample_rate, and channel layout. Read() checks nb_samples only and then sends m_frame data through converters initialized with m_codec_ctx, so changed format/later dynamic audio properties get converted with stale layout/rate settings. Compare to m_frame before conversion and reset m_converter when these properties change, or reconfigure the swresample context accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/audio/ffmpeg_audio_reader.h` around lines 207 - 225, Update the converter
configuration used by Read() to derive input sample format, sample rate, and
channel layout from m_frame rather than only m_codec_ctx. Before converting each
frame, compare those properties with the current FFmpegAudioProcessor
configuration and reset/rebuild m_converter when they change, while preserving
the existing output settings and channel-layout API branches.
| av_freep(&m_convert_buffer[0]); | ||
| m_convert_buffer_nb_samples = std::max(1024 * 8, m_frame->nb_samples); | ||
| ret = av_samples_alloc(m_convert_buffer, &linsize, m_codec_ctx->ch_layout.nb_channels, m_convert_buffer_nb_samples, AV_SAMPLE_FMT_S16, 1); | ||
| ret = av_samples_alloc(m_convert_buffer, &linsize, CHROMAPRINT_CODEC_CHANNELS(m_codec_ctx), m_convert_buffer_nb_samples, AV_SAMPLE_FMT_S16, 1); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '300,375p' src/audio/ffmpeg_audio_reader.h
echo
echo "== relevant symbols/usages =="
rg -n "m_convert_buffer|m_convert_buffer_nb_samples|m_output_channels|CHROMAPRINT_CODEC_CHANNELS|av_samples_alloc|swr_convert|SwrContext" src/audio/ffmpeg_audio_reader.h src/audio || true
echo
echo "== diff stat/name status if available =="
git diff --stat || true
git diff -- src/audio/ffmpeg_audio_reader.h | sed -n '1,220p' || trueRepository: acoustid/chromaprint
Length of output: 7178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ffmpeg_audio_processor_swresample.h =="
sed -n '1,120p' src/audio/ffmpeg_audio_processor_swresample.h
echo
echo "== broader av_samples_alloc usage =="
rg -n "av_samples_alloc|CHROMAPRINT_CODEC_CHANNELS\\(m_codec_ctx\\)|m_output_channels" src/audio -g '*.h' -g '*.cpp'
echo
echo "== FFmpeg doxygen evidence via shell curl if available =="
if command -v curl >/dev/null 2>&1; then
curl -fsSL --max-time 15 "https://ffmpeg.org/doxygen/5.1/group__lswr__audio__conversion__util.html" | \
grep -C 5 "av_samples_alloc" || true
else
echo "curl not available"
fiRepository: acoustid/chromaprint
Length of output: 3854
🌐 Web query:
FFmpeg av_samples_alloc nb_channels parameter documentation lswr
💡 Result:
In the FFmpeg API, the nb_channels parameter in av_samples_alloc represents the number of audio channels to be allocated in the buffer [1][2][3]. The function is part of the libavutil samples manipulation API [1][4]. Its full signature is: int av_samples_alloc(uint8_t audio_data, int *linesize, int nb_channels, int nb_samples, enum AVSampleFormat sample_fmt, int align); Parameter Details: - nb_channels: The number of audio channels for which memory should be allocated [1][3]. - audio_data: An output array that will be populated with the pointers to the data for each channel [1][3]. - nb_samples: The number of samples per channel [1][3]. - sample_fmt: The audio sample format (e.g., AV_SAMPLE_FMT_S16, AV_SAMPLE_FMT_FLTP) [1][3]. - linesize: An output pointer to store the aligned size of the audio buffer(s); this may be set to NULL if not needed [1][3]. - align: The alignment requirement for the buffer size (0 for default, 1 for no specific alignment) [1][3]. While av_samples_alloc is frequently used alongside libswresample (lswr) to manage buffers for converted audio [4][5], it is defined in libavutil/samplefmt.h [6][3]. Libswresample handles the conversion logic, such as resampling or channel layout changes, and its configuration is managed via SwrContext [4][7]. When working with channel layouts in newer FFmpeg versions, developers are encouraged to use the AVChannelLayout API in functions like swr_alloc_set_opts2 for context initialization, while av_samples_alloc remains the standard tool for the low-level memory allocation of the sample buffers themselves [1][4][8].
Citations:
- 1: https://ffmpeg.org/doxygen/8.0/group__lavu__sampmanip.html
- 2: https://www.ffmpeg.org/doxygen/trunk/group__lavu__sampmanip.html
- 3: https://github.com/FFmpeg/FFmpeg/blob/master/libavutil/samplefmt.h
- 4: https://ffmpeg.org/doxygen/7.0/group__lswr.html
- 5: https://www.ffmpeg.org/doxygen/5.1/group__lswr.html
- 6: https://ffmpeg.org/doxygen/trunk/samplefmt_8h.html
- 7: https://ffmpeg.org/doxygen/7.1/swresample_8h_source.html
- 8: https://github.com/FFmpeg/FFmpeg/blob/master/libswresample/swresample.h
Allocate the converter output buffer by output channels.
m_convert_buffer is packed AV_SAMPLE_FMT_S16, and this allocation currently uses the codec input channels even though m_converter outputs m_output_channels. If mono input is requested as stereo, m_converter->Convert(...) can write 2 channels into size for 1.
Proposed fix
- ret = av_samples_alloc(m_convert_buffer, &linsize, CHROMAPRINT_CODEC_CHANNELS(m_codec_ctx), m_convert_buffer_nb_samples, AV_SAMPLE_FMT_S16, 1);
+ ret = av_samples_alloc(m_convert_buffer, &linsize, m_output_channels, m_convert_buffer_nb_samples, AV_SAMPLE_FMT_S16, 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ret = av_samples_alloc(m_convert_buffer, &linsize, CHROMAPRINT_CODEC_CHANNELS(m_codec_ctx), m_convert_buffer_nb_samples, AV_SAMPLE_FMT_S16, 1); | |
| ret = av_samples_alloc(m_convert_buffer, &linsize, m_output_channels, m_convert_buffer_nb_samples, AV_SAMPLE_FMT_S16, 1); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/audio/ffmpeg_audio_reader.h` at line 342, Update the av_samples_alloc
call in the converter buffer allocation path to use m_output_channels rather
than CHROMAPRINT_CODEC_CHANNELS(m_codec_ctx), matching the channel count
produced by m_converter->Convert(...). Keep the existing packed S16 format and
sample-count allocation behavior unchanged.
avformat_open_input() and av_find_best_stream() took non-const pointers before libavformat 59 (FFmpeg 5.0), so the const declarations the reader uses do not compile there. Qualify the two declarations through a macro that is empty below 59.
2 participants
test-linuxandtest-macosboth pass-DBUILD_TOOLS=OFF, so the FFmpeg audio reader is never compiled there — FFmpeg is only pulled in as an FFT provider for theavfftmatrix entry. The packaging jobs do buildfpcalc, butpackage/build.shpinsFFMPEG_VERSION=8.0.So the combination distributions actually use — tools enabled, system FFmpeg — is never built. That is also the configuration the FFmpeg detection in
CMakeLists.txtandcmake/modules/FindFFmpeg.cmakeexists for, which means changes to it (such as #158, which touched only those two files) land without CI covering what they fix.This adds a
test-linux-toolsjob that builds with-DBUILD_TOOLS=ONagainst the FFmpegubuntu-22.04ships, runs the test suite, and checksfpcalcstill producestests/data/test.mp3.fpcalc.out— the same assertion the Linux packaging job makes, but against a different FFmpeg. It prints the FFmpeg package versions first, so a failure reports which ones it built against rather than needing a rerun to find out.FFT_LIB=fftw3so the job tests FFmpeg as the audio reader rather than as the FFT.What I could not check locally
There are no FFmpeg development libraries on the machine I wrote this on, so I could not build
fpcalcbefore pushing — this PR's own CI run is the first real test of it. Two things it will settle:fpcalcbinary path is right (build.test.tools/src/cmd/fpcalc);test.mp3with the older FFmpeg gives the same fingerprint as the pinned 8.0 the reference was generated with. If it does not, the output check needs relaxing to a build-and-run smoke test, and the difference is itself worth knowing about.Happy to adjust once it has run.