Fix build with FFmpeg 8.0 #153
Conversation
WalkthroughReplaced explicit codec close and manual nulling with a single FFmpeg API call: Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/audio/ffmpeg_audio_reader.h (7)
221-234: Close() leaves m_opened and runtime state stale; also keeps converter/buffer alive.After Close(), IsOpen() can still return true, and buffers persist across re-opens. Reset state and release converter/buffer here.
inline void FFmpegAudioReader::Close() { av_frame_free(&m_frame); av_packet_free(&m_packet); m_stream_index = -1; - if (m_codec_ctx) { - avcodec_free_context(&m_codec_ctx); - } + avcodec_free_context(&m_codec_ctx); if (m_format_ctx) { avformat_close_input(&m_format_ctx); } + + // Reset runtime state + m_opened = false; + m_has_more_packets = false; + m_has_more_frames = false; + m_error.clear(); + m_error_code = 0; + m_nb_packets = 0; + m_decode_error = 0; + + // Release converter and its buffer + m_converter.reset(); + av_freep(&m_convert_buffer[0]); + m_convert_buffer_nb_samples = 0; }
176-176: Remove or guard av_dump_format; hardcoded "foo" is noisy.Either drop the call or compile-gate it and print the real filename.
- av_dump_format(m_format_ctx, 0, "foo", 0); +#if defined(CHROMAPRINT_DEBUG) + av_dump_format(m_format_ctx, 0, file_name.c_str(), 0); +#endif
148-148: Fix typo in error message.- SetError("Coud not find stream information in the file", ret); + SetError("Could not find stream information in the file", ret);
268-274: Flush decoder at EOF to avoid dropping trailing frames.On AVERROR_EOF from av_read_frame, send a NULL packet to trigger decoder drain.
- if (ret < 0) { + if (ret < 0) { if (ret == AVERROR_EOF) { needs_packet = false; m_has_more_packets = false; + // Flush decoder to drain delayed frames + (void)avcodec_send_packet(m_codec_ctx, nullptr); break; }Please verify with a file that has codec delay (e.g., AAC/MP3) to ensure the last frames are delivered.
114-117: Use snprintf instead of sprintf to avoid potential overflow.- char buf[64]; - sprintf(buf, "%d", sample_rate); + char buf[64]; + snprintf(buf, sizeof(buf), "%d", sample_rate);
120-128: ch_layout string for arbitrary N channels: prefer "%dc" and snprintf."3 channels" may not parse; "%dc" (e.g., "3c") is the generic unknown layout form. Also switch to snprintf.
- char buf[64]; - if (channels == 1) - sprintf(buf, "%s", "mono"); - else if (channels == 2) - sprintf(buf, "%s", "stereo"); - else - sprintf(buf, "%d channels", channels); - return av_dict_set(&m_input_opts, "ch_layout", buf, 0) >= 0; + char buf[64]; + if (channels == 1) { + snprintf(buf, sizeof(buf), "%s", "mono"); + } else if (channels == 2) { + snprintf(buf, sizeof(buf), "%s", "stereo"); + } else { + snprintf(buf, sizeof(buf), "%dc", channels); + } + return av_dict_set(&m_input_opts, "ch_layout", buf, 0) >= 0;Confirm your targeted FFmpeg versions accept the "%dc" form for ch_layout.
317-321: Nit: rename linsize → linesize for clarity.- int linsize; + int linesize; - 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, &linesize, m_codec_ctx->ch_layout.nb_channels, m_convert_buffer_nb_samples, AV_SAMPLE_FMT_S16, 1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/audio/ffmpeg_audio_reader.h(1 hunks)
🔇 Additional comments (1)
src/audio/ffmpeg_audio_reader.h (1)
227-229: FFmpeg 8 migration: correct switch to avcodec_free_context.Good replacement; it closes, frees, and NULLs the context and fixes the FFmpeg 8.0 removal of avcodec_close().
|
Thank you! |
2 participants
See FFmpeg/FFmpeg@0d48da2 for
avcodec_close()removal in FFmpeg 8.0The deprecation comment for
avcodec_close()said:avcodec_free_context()has been around since FFmpeg 2.3 (lavc 55.52.0, FFmpeg/FFmpeg@fd05602). Seems old enough for most versions of FFmpeg is active use but can add conditional if older versions are needed.Documentation at https://ffmpeg.org/doxygen/2.3/group__lavc__core.html#gaf869d0829ed607cec3a4a02a1c7026b3 says: