Skip to content

refactor: decoder utilizing ffmpeg and ma decoders#1050

Open
mdydek wants to merge 3 commits intomainfrom
refactor/decoder
Open

refactor: decoder utilizing ffmpeg and ma decoders#1050
mdydek wants to merge 3 commits intomainfrom
refactor/decoder

Conversation

@mdydek
Copy link
Copy Markdown
Collaborator

@mdydek mdydek commented May 6, 2026

Closes #

⚠️ Breaking changes ⚠️

Introduced changes

  • decoder now utilizes already present ffmpeg and miniaudio decoding code

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added/Conducted relevant tests
  • Performed self-review of the code
  • Updated Web Audio API coverage
  • Added support for web
  • Updated old arch android spec file

Copy link
Copy Markdown
Contributor

@closetcaiman closetcaiman left a comment

Choose a reason for hiding this comment

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

lgtm,

nice job deduplicating this!

Comment on lines 53 to 59
bool needsFFmpeg(AudioFormat format) {
return format == AudioFormat::MP4 || format == AudioFormat::M4A || format == AudioFormat::AAC;
}

return result;
bool needsFFmpegByPath(const std::string &path) {
return AudioDecoder::pathHasExtension(path, {".mp4", ".m4a", ".aac"});
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't those be included in the header?

Comment on lines -86 to -91

if (initResult != MA_SUCCESS) {
return Err(
"Failed to initialize miniaudio decoder: " +
std::string(ma_result_description(initResult)));
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be cool to preserve decoder API errors (from ffmpeg/ma) provided they are informative. Not a must have as it would require to refactor decoding::IncrementalDecoder implementations.

@mdydek mdydek requested a review from closetcaiman May 8, 2026 10:14
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