Skip to content

LibMedia: Implement proper PCM format detection in FFmpegLoader - #6528

Closed
boris-chu wants to merge 2 commits into
LadybirdBrowser:masterfrom
boris-chu:feature/ffmpeg-pcm-format-detection
Closed

boris-chu wants to merge 2 commits into
LadybirdBrowser:masterfrom
boris-chu:feature/ffmpeg-pcm-format-detection

Conversation

@boris-chu

Copy link
Copy Markdown

Summary

This PR implements proper PCM format detection in FFmpegLoader::pcm_format(), which previously always returned Float32 regardless of the actual audio format.

Changes:

  • Maps FFmpeg's sample format (AVSampleFormat) to our PcmSampleFormat enum
  • Supports formats that extract_samples_from_frame() already handles: U8, S16, S32, Float32
  • Falls back to Float32 for unsupported formats to maintain backward compatibility
  • Added comprehensive tests to verify format detection works correctly

Why This Matters

The pcm_format() method was marked with a FIXME comment indicating it was unused and always returned Float32. This implementation:

  • Provides accurate format information to consumers of the API
  • Enables proper handling of different audio sample formats
  • Maintains consistency with what the decoder actually produces
  • Prepares the codebase for future optimizations based on sample format

Technical Details

The implementation uses FFmpeg's av_get_packed_sample_fmt() to normalize planar and interleaved formats, then maps the packed format to our enum:

  • AV_SAMPLE_FMT_U8 → PcmSampleFormat::Uint8
  • AV_SAMPLE_FMT_S16 → PcmSampleFormat::Int16
  • AV_SAMPLE_FMT_S32 → PcmSampleFormat::Int32
  • AV_SAMPLE_FMT_FLT → PcmSampleFormat::Float32

For formats not currently handled by extract_samples_from_frame(), we fall back to Float32 since that's what everything gets converted to anyway.

Test Plan

Automated Tests:
Created Tests/LibMedia/TestFFmpegLoader.cpp with 3 test cases:

  1. Vorbis Format Test: Verifies Vorbis OGG files report Float32 format
  2. WAV Format Test: Verifies WAV files report valid PCM formats
  3. Basic Functionality Test: Ensures loading and decoding still works correctly

Test Execution:

# Build and run tests
cmake -B Build -S .
cmake --build Build --target TestFFmpegLoader
./Build/bin/TestFFmpegLoader

Expected Results:

  • All tests pass
  • Vorbis files correctly report Float32 format
  • WAV files report one of the supported formats (Uint8, Int16, Int24, Int32, Float32)
  • Sample loading continues to work without errors

Files Changed

@ladybird-bot

Copy link
Copy Markdown
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

Previously, the pcm_format() method always returned Float32
regardless of the actual audio format. This commit implements
proper format detection by mapping FFmpeg's sample format to our
PcmSampleFormat enum.

The implementation supports the formats that
extract_samples_from_frame() already handles (U8, S16, S32,
Float32) and falls back to Float32 for any unsupported formats,
maintaining backward compatibility.
This adds comprehensive tests for the FFmpegLoader pcm_format()
method to verify that it correctly reports the audio format for
different file types.

Test coverage includes:
- Vorbis OGG files (Float32 format)
- WAV files (validates format is one of the supported types)
- Basic functionality test for loading and decoding samples

The tests follow the existing pattern established by TestWav.cpp
and integrate with the existing test infrastructure.
@boris-chu
boris-chu force-pushed the feature/ffmpeg-pcm-format-detection branch from 74e5c4d to 323c011 Compare October 21, 2025 05:07

PcmSampleFormat FFmpegLoaderPlugin::pcm_format()
{
// FIXME: pcm_format() is unused, always return Float for now

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You remove this FIXME that hints at this method not being used. What did you do to fix it?

@boris-chu

Copy link
Copy Markdown
Author

I found that the PlaybackStream implementations are hardcoding PcmSampleFormat::Float32 instead of using the loader's format:

  • PlaybackStreamAudioUnit.cpp:198
  • PlaybackStreamOboe.cpp:26
  • AudioCodecPluginAgnostic.cpp:46

This PR implements the format detection in FFmpegLoader, but doesn't update those other files yet. Should I expand this PR to update them as well?

@Zaggy1024

Copy link
Copy Markdown
Contributor

FYI, the audio loader system is likely to be removed, it'll be unused after #6410 is merged. It's still present in that PR simply because WAV support isn't there yet, since it requires passing around channel maps, but once that's working, I won't waste any time in deleting the audio loader system.

I found that the PlaybackStream implementations are hardcoding PcmSampleFormat::Float32 instead of using the loader's format:

* PlaybackStreamAudioUnit.cpp:198

* PlaybackStreamOboe.cpp:26

* AudioCodecPluginAgnostic.cpp:46

This PR implements the format detection in FFmpegLoader, but doesn't update those other files yet. Should I expand this PR to update them as well?

I want to say that we probably won't have a reason to use any other format in the PlaybackStream implementations, actually, following the PR mentioned above. We have to mix multiple tracks into one output, so currently the AudioDecoder interface just outputs audio blocks in f32 before they reach the mixer, which will then mix the audio into the PlaybackStream.

@boris-chu

Copy link
Copy Markdown
Author

Thanks for the feedback, Gregory!

Given that the audio loader system will be deprecated after #6410 and the new architecture will standardize on f32, this PR won't provide long-term value.

I'll close this PR. Thanks for explaining the architectural direction!

@boris-chu boris-chu closed this Oct 25, 2025
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.

4 participants