feat: add FFmpeg local recording and playback helpers - #685
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Castiron custom code✅ No new custom-code files detected. 50 mixed files remain; 0 existing customizations changed. Compared 50 existing customizations unchanged
10 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 34272889817 --repo openai/openai-ruby \
--name castiron-custom-code-34272889817-1 --dir /tmp/castiron-custom-code-34272889817-1
git apply --stat /tmp/castiron-custom-code-34272889817-1/custom-code.patch
cat /tmp/castiron-custom-code-34272889817-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 0e4c9062dbeea705f42495cfa25d9fea2e919874 e9ae562e5ff6e45cef98fb043feef4c9e81e9704
python3 scripts/castiron/custom_code_report.py report \
--base 0e4c9062dbeea705f42495cfa25d9fea2e919874 \
--head e9ae562e5ff6e45cef98fb043feef4c9e81e9704 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-e9ae562e5ff6
cat /tmp/castiron-custom-code-e9ae562e5ff6/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2dfeb4d078
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed all 44 changed files against merge base 21b8a70d48e2623e32aa01f95503adcf3fa2ba75, with actual main/base c368a06db374892273812f64c823f8d7b21871a2. Included the documentation-only update from 2dfeb4d0 to current head 69f432e9.
One additional P2 finding below: a natural-drain/interruption race can retire an item before its terminal notification and fail the session. The separate device/process and conversation layers are appropriate; no file crosses 1,000 lines. The remaining structural problem here is duplicated terminal ownership between the output ledger, session ledger and notification queue. Give that transition one owner rather than adding another late-event special case.
Fresh validation: 249 Ruby tests / 1,378 assertions passed on Ruby 4.0.6, plus three synthetic startup/teardown probes (8 assertions), C++ ring/clock checks, example Sorbet checks, lazy-loading and main-gem file-inclusion checks. A separate deterministic probe using the real AudioOutput with simulated playback reproduces the inline race. The Ruby subprocess fixtures needed this environment's Ruby launcher; no source was changed. The broader initial run had local subprocess-launcher, Sorbet PATH and proxy-test errors; the corrected passing run excludes the unchanged network-invariants file.
Ruby 3.3/3.4, native worker integration/builds (development headers unavailable), physical devices and live API were not exercised. Author-reported platform/live/type/package results remain author evidence. No devices, recordings or live API were accessed.
jbeckwith-oai
changed the title
cdb8569 to
6cace08
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cace08938
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8944082aef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9695513 to
fd074f1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd074f1fa2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Follow-up review of the narrowed finite FFmpeg/FFplay scope at fd074f1f, against main/base/merge base 0e4c9062. Read all 13 changed files and relevant process, IO, FilePart, type and example contracts. The managed-session/native layer is removed and existing Realtime code is unchanged; my previous terminal-ownership finding no longer applies.
Two P2 issues remain, independently reproduced below and corroborating the existing bot findings: child exit does not wake playback waiting on its source, and a boolean diagnostics check retains unbounded stderr on disk. The scope reduction substantially simplifies ownership; keeping process status and a bounded diagnostics signal with the existing process owner would further simplify these paths. No file crosses 1,000 lines; new production files are 161, 136 and 27 lines.
Fresh checks on Ruby 4.0.6: focused suite 13 tests / 47 assertions passed (including real FFplay dummy output and simulated unsupported TERM); four adversarial probes / 15 assertions characterized the two defects and verified cancellation cleanup, WAV metadata and argument boundaries. Actual FFmpeg synthetic 50 ms capture produced the expected mono 24 kHz PCM16 WAV and played through FFplay's dummy driver. Example Sorbet, RBS syntax, lazy loading, package inclusion and diff checks passed. GitHub currently reports 18 successful and two skipped checks; those are CI evidence, not local executions.
Ruby 3.3/3.4 and native Windows/macOS behavior were not run locally. No physical devices, private audio or live API were used; author-reported live E2E remains separate evidence.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Follow-up review at e9ae562e, against main/base/merge base 0e4c9062. Revalidated all 13 changed files, the delta from fd074f1f, and the author responses to my two P2 findings. Both are fixed: playback observes child exit while its source remains open, and stderr is drained into a bounded boolean signal without diagnostic files or another thread. Non-nil timeout validation also correctly rejects false.
The process owner now contains the diagnostic lifecycle, which removes the unnecessary tempfile lifecycle. Recording still drains child stdout completely. Existing Realtime APIs are unchanged, and the removed managed/native layer remains out of scope. No remaining substantive correctness or structural findings; no file crosses 1,000 lines (new production files: 157, 158 and 27 lines).
Fresh Ruby 4.0.6 validation: 16 tests / 56 assertions passed, including real FFplay dummy output and simulated unsupported TERM; six independent probes / 15 assertions passed for noisy exit with an open source, failed startup, cancellation, descriptor/reaping cleanup, complete recording output, no diagnostic files and timeout validation. Actual FFmpeg synthetic capture/FFplay dummy playback, example Sorbet, RBS syntax, lazy loading, package inclusion and diff checks also passed. GitHub reports 18 successful checks and two skips.
Ruby 3.3/3.4 and native Windows/macOS behavior were not exercised locally. No physical devices, private recordings or live API were used; author-reported live E2E remains separate evidence.
2 participants
Summary
Add opt-in FFmpeg/FFplay helpers for recording a finite microphone clip and playing generated speech. A recording returns a finalized mono 24 kHz PCM16 WAV as an
OpenAI::FilePart, ready for the existing transcription API. Playback consumes caller-provided audio streams without closing them.OpenAI::LocalAudio.record(duration:, device: nil, timeout: nil)and.play(source, format: :auto, timeout: nil)with subprocess cleanup, optional deadlines and typed errors.Existing Realtime API helpers and examples are unchanged. This PR adds no native audio library, companion gem, continuous device API, managed conversation session or playback-clock abstraction.
Validation
gpt-transcribeandgpt-4o-mini-tts/marinthrough real FFmpeg/FFplay and synthetic media.Physical microphone permissions, speaker output and platform/device compatibility were not validated by the synthetic E2E test.