upgrade to ffmpeg 8#688
Conversation
Summary of ChangesHello @zgabi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on upgrading the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request upgrades FFmpeg.AutoGen to version 8.0.0, which brings in support for FFmpeg 6.0. The changes correctly adapt the codebase to the updated FFmpeg API by replacing or removing calls to deprecated and removed functions and flags. For instance, the usage of the removed AVFMT_FLAG_SHORTEST flag and functions like av_format_inject_global_side_data and av_stream_get_side_data have been addressed. The workaround to pass the packet size using the opaque field is a clever solution to the removal of AVFrame.pkt_size.
I have one suggestion to improve user feedback regarding a feature that is no longer supported by FFmpeg, which is currently being silently ignored. Other than that, the changes look solid and necessary for the upgrade.
There was a problem hiding this comment.
Pull request overview
This pull request upgrades the FFmpeg bindings from version 7.0.0 to 8.0.0 by updating the FFmpeg.AutoGen package dependency and adapting the code to handle API changes in FFmpeg 8.
Changes:
- Updated FFmpeg.AutoGen package from version 7.0.0 to 8.0.0 in project files
- Replaced deprecated FFmpeg APIs with their FFmpeg 8 equivalents
- Introduced workaround for removed pkt_size field using the opaque pointer
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Unosquare.FFME.Windows/Unosquare.FFME.Windows.csproj | Updates FFmpeg.AutoGen package reference to version 8.0.0 |
| Unosquare.FFME.Windows.Sample/Unosquare.FFME.Windows.Sample.csproj | Updates FFmpeg.AutoGen package reference to version 8.0.0 |
| Unosquare.FFME/Container/VideoComponent.cs | Migrates from deprecated av_stream_get_side_data to av_packet_side_data_get API; changes ScalerFlags from int to SwsFlags enum |
| Unosquare.FFME/Container/MediaFrame.cs | Replaces deprecated pkt_size field access with opaque pointer cast |
| Unosquare.FFME/Container/MediaContainer.cs | Comments out removed av_format_inject_global_side_data and AVFMT_FLAG_SHORTEST; implements packet size workaround via opaque field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ffmpeg.av_format_inject_global_side_data(InputContext); | ||
|
|
There was a problem hiding this comment.
The av_format_inject_global_side_data call has been commented out instead of being properly removed or documented. If this function is removed in FFmpeg 8 and no longer needed, the comment should explain why it's safe to remove. If it's been replaced by an alternative approach, that should be documented here.
| // ffmpeg.av_format_inject_global_side_data(InputContext); |
| InputContext->flags |= opts.FlagNoBuffer ? ffmpeg.AVFMT_FLAG_NOBUFFER : InputContext->flags; | ||
| InputContext->flags |= opts.FlagSortDts ? ffmpeg.AVFMT_FLAG_SORT_DTS : InputContext->flags; | ||
| InputContext->flags |= opts.FlagStopAtShortest ? ffmpeg.AVFMT_FLAG_SHORTEST : InputContext->flags; | ||
| ////InputContext->flags |= opts.FlagStopAtShortest ? ffmpeg.AVFMT_FLAG_SHORTEST : InputContext->flags; |
There was a problem hiding this comment.
The AVFMT_FLAG_SHORTEST flag usage has been commented out with four slashes instead of being properly removed or documented. Since the FlagStopAtShortest option still exists in DemuxerGlobalOptions, users may set this option expecting it to work, but it will now be silently ignored. Either document why this flag is no longer supported in FFmpeg 8, remove the option from DemuxerGlobalOptions, or provide an alternative implementation.
| ////InputContext->flags |= opts.FlagStopAtShortest ? ffmpeg.AVFMT_FLAG_SHORTEST : InputContext->flags; | |
| // NOTE: AVFMT_FLAG_SHORTEST is no longer supported in FFmpeg 8+. | |
| // The DemuxerGlobalOptions.FlagStopAtShortest option is kept for backward compatibility | |
| // but is currently a no-op and does not modify InputContext->flags. |
| if (Data.TryHandleDataPacket(this, readPacket)) | ||
| return MediaType.None; | ||
|
|
||
| readPacket.Pointer->opaque = (void*)readPacket.Pointer->size; |
There was a problem hiding this comment.
The packet size is being stored in the opaque field as a workaround for the deprecated pkt_size field. However, there's no documentation explaining this workaround pattern or any validation that the opaque field isn't already being used for another purpose. Consider adding a comment explaining that this is a workaround for the removed pkt_size field in FFmpeg 8, and verify that opaque is not used elsewhere in a conflicting way.
| readPacket.Pointer->opaque = (void*)readPacket.Pointer->size; | |
| // FFmpeg 8 removed AVPacket.pkt_size. As a workaround, store the packet size | |
| // in AVPacket.opaque for later retrieval, but only if opaque is not already | |
| // being used for some other purpose. | |
| if (readPacket.Pointer->opaque == null) | |
| readPacket.Pointer->opaque = (void*)readPacket.Pointer->size; |
|
@mariodivece Could you please check this PR and release a new version which supports ffmpeg 8? Thank you in advance. |
|
@mariodivece Any news about the ffmpeg 8 support? |
No description provided.