using LGPL compiled ffmpeg shared for FFmpeg n5.0 from [here](https:/ #1
Conversation
…/github.com/BtbN/FFmpeg-Builds/releases) * specifically - ffmpeg-n5.0-latest-win64-lgpl-shared-5.0.zip targets openH264 rather than the GPL x264 encoder modifies FFmpeg.AutoGen.Example to output an MP4 rather than .h264 file by way of: * adding AVFormatContext and AVStream * using ffmpeg.avio_open to create / write the file instead of System.IO.xxx * writing a header and trailer * using ffmpeg.av_interleaved_write_frame() includes a somewhat mangled but updated and somewhat working version of the Transcoding gist shared by Ruslan-B
WalkthroughThe changes involve updates to the FFmpeg.AutoGen project, including the addition of a new transcoding example project, enhancements to error handling, and the removal of post-processing features. The Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 19
Configuration used: CodeRabbit UI
Files ignored due to filter (8)
- FFmpeg/bin/x64/avcodec-59.dll
- FFmpeg/bin/x64/avdevice-59.dll
- FFmpeg/bin/x64/avfilter-8.dll
- FFmpeg/bin/x64/avformat-59.dll
- FFmpeg/bin/x64/avutil-57.dll
- FFmpeg/bin/x64/postproc-56.dll
- FFmpeg/bin/x64/swresample-4.dll
- FFmpeg/bin/x64/swscale-6.dll
Files selected for processing (16)
- .gitignore (1 hunks)
- FFmpeg.AutoGen.Example.Transcoding/FFmpeg.AutoGen.Example.Transcoding.csproj (1 hunks)
- FFmpeg.AutoGen.Example.Transcoding/FFmpegBinariesHelper.cs (1 hunks)
- FFmpeg.AutoGen.Example.Transcoding/FFmpegHelper.cs (1 hunks)
- FFmpeg.AutoGen.Example.Transcoding/Program.cs (1 hunks)
- FFmpeg.AutoGen.Example.Transcoding/Transcoding.cs (1 hunks)
- FFmpeg.AutoGen.Example/FFmpegHelper.cs (1 hunks)
- FFmpeg.AutoGen.Example/H264VideoStreamEncoder.cs (2 hunks)
- FFmpeg.AutoGen.Example/Program.cs (6 hunks)
- FFmpeg.AutoGen.Example/VideoFrameConverter.cs (2 hunks)
- FFmpeg.AutoGen.sln (2 hunks)
- FFmpeg.AutoGen/FFmpeg.arrays.g.cs (1 hunks)
- FFmpeg.AutoGen/FFmpeg.functions.export.g.cs (1 hunks)
- FFmpeg.AutoGen/FFmpeg.libraries.g.cs (1 hunks)
- FFmpeg.AutoGen/FFmpeg.macros.g.cs (2 hunks)
- FFmpeg/include/libavutil/ffversion.h (1 hunks)
Files skipped from review due to trivial changes (7)
- .gitignore
- FFmpeg.AutoGen.Example.Transcoding/FFmpeg.AutoGen.Example.Transcoding.csproj
- FFmpeg.AutoGen.Example/FFmpegHelper.cs
- FFmpeg.AutoGen.Example/VideoFrameConverter.cs
- FFmpeg.AutoGen/FFmpeg.arrays.g.cs
- FFmpeg.AutoGen/FFmpeg.libraries.g.cs
- FFmpeg/include/libavutil/ffversion.h
Additional comments: 9
FFmpeg.AutoGen.Example.Transcoding/FFmpegHelper.cs (1)
- 13-13: The conversion from the buffer to a string assumes ANSI encoding, which is appropriate for FFmpeg error messages.
FFmpeg.AutoGen.Example/H264VideoStreamEncoder.cs (4)
19-88: The constructor has been significantly refactored to initialize FFmpeg structures and perform error checking. Ensure that all instances where
H264VideoStreamEncoderis instantiated are updated to pass a filename instead of aStreamobject.119-163: The
Encodemethod now takes an additional parameterbool skipFrameCheckand returns anint. Verify that all calls toEncodehave been updated to handle the new parameter and return type correctly.106-117: The
Flushmethod is a new addition to the class. Review this method to ensure it correctly handles the flushing of the stream encoder according to FFmpeg's requirements.19-88: Improved exception handling with more specific error messages is a good practice. Ensure that the error messages provided are informative and accurately describe the error condition.
FFmpeg.AutoGen.Example/Program.cs (1)
- 207-207: Verify that the "./images" directory exists before attempting to read files from it to avoid potential exceptions.
FFmpeg.AutoGen/FFmpeg.functions.export.g.cs (1)
- 15996-15998: The hunk appears to be incomplete, showing only the definition of a delegate and the start of a lambda function. Ensure that the lambda function is correctly implemented and that the delegate is properly used within the codebase.
FFmpeg.AutoGen/FFmpeg.macros.g.cs (2)
1466-1471: The constants related to
LIBAVUTILandLIBSWRESAMPLEremain unchanged and appear to be correctly defined.1522-1527: The parser flags are defined correctly, and the commented-out macros for UTF encoding and rounded division do not affect the functionality. If these macros are no longer needed, it's good to keep them commented out or remove them entirely for clarity.
| { | ||
| FFmpegBinariesHelper.RegisterFFmpegBinaries(); | ||
| var transcoding = new Transcoding(); | ||
| transcoding.main(args.Length, args); |
There was a problem hiding this comment.
The method name main does not follow C# naming conventions. Consider renaming it to Main or another appropriate PascalCase name to align with best practices.
| FFmpegBinariesHelper.RegisterFFmpegBinaries(); | ||
| var transcoding = new Transcoding(); | ||
| transcoding.main(args.Length, args); |
There was a problem hiding this comment.
There is no error handling for exceptions that might be thrown by RegisterFFmpegBinaries or transcoding.main. Consider adding try-catch blocks to handle potential exceptions gracefully.
| { | ||
| public static unsafe string av_strerror(int error) | ||
| { | ||
| var bufferSize = 1024; |
There was a problem hiding this comment.
Consider defining the buffer size as a constant to improve code readability and maintainability.
- var bufferSize = 1024;
+ const int ErrorBufferSize = 1024;
+ var bufferSize = ErrorBufferSize;Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| var bufferSize = 1024; | |
| const int ErrorBufferSize = 1024; | |
| var bufferSize = ErrorBufferSize; |
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "FFmpeg.AutoGen.Examples.ResamplingAudio", "FFmpeg.AutoGen.Examples.ResamplingAudio\FFmpeg.AutoGen.Examples.ResamplingAudio.csproj", "{A192914F-DAA8-400B-B5CA-BA188AEBB42B}" | ||
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "FFmpeg.AutoGen.Examples.ResamplingAudio", "FFmpeg.AutoGen.Examples.ResamplingAudio\FFmpeg.AutoGen.Examples.ResamplingAudio.csproj", "{A192914F-DAA8-400B-B5CA-BA188AEBB42B}" | ||
| EndProject | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "FFmpeg.AutoGen.Example.Transcoding", "FFmpeg.AutoGen.Example.Transcoding\FFmpeg.AutoGen.Example.Transcoding.csproj", "{50CC883F-A967-4880-BF1A-0DBBB78B9454}" |
There was a problem hiding this comment.
The project type GUID for "FFmpeg.AutoGen.Example.Transcoding" should be updated to match the new format used by other projects in the solution: "{9A19103F-16F7-4668-BE54-9A1E7A4F7556}".
- Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "FFmpeg.AutoGen.Example.Transcoding", "FFmpeg.AutoGen.Example.Transcoding\FFmpeg.AutoGen.Example.Transcoding.csproj", "{50CC883F-A967-4880-BF1A-0DBBB78B9454}"
+ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "FFmpeg.AutoGen.Example.Transcoding", "FFmpeg.AutoGen.Example.Transcoding\FFmpeg.AutoGen.Example.Transcoding.csproj", "{50CC883F-A967-4880-BF1A-0DBBB78B9454}"Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "FFmpeg.AutoGen.Example.Transcoding", "FFmpeg.AutoGen.Example.Transcoding\FFmpeg.AutoGen.Example.Transcoding.csproj", "{50CC883F-A967-4880-BF1A-0DBBB78B9454}" | |
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "FFmpeg.AutoGen.Example.Transcoding", "FFmpeg.AutoGen.Example.Transcoding\FFmpeg.AutoGen.Example.Transcoding.csproj", "{50CC883F-A967-4880-BF1A-0DBBB78B9454}" |
| public static int ThrowExceptionIfError(this int error) | ||
| { | ||
| if (error < 0) throw new ApplicationException(av_strerror(error)); | ||
| return error; | ||
| } |
There was a problem hiding this comment.
The method name ThrowExceptionIfError could be improved to follow .NET naming conventions for methods that can throw exceptions. Additionally, consider using a more specific exception type than ApplicationException.
- public static int ThrowExceptionIfError(this int error)
+ public static int EnsureSuccessOrThrow(this int error)
{
if (error < 0) throw new FFmpegException(av_strerror(error));
return error;
}Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| public static int ThrowExceptionIfError(this int error) | |
| { | |
| if (error < 0) throw new ApplicationException(av_strerror(error)); | |
| return error; | |
| } | |
| public static int EnsureSuccessOrThrow(this int error) | |
| { | |
| if (error < 0) throw new FFmpegException(av_strerror(error)); | |
| return error; | |
| } |
| if (in_stream->codecpar->codec_type == AVMediaType.AVMEDIA_TYPE_VIDEO) | ||
| filter_spec = "null"; /* passthrough (dummy) filter for video */ | ||
| else | ||
| filter_spec = "anull"; /* passthrough (dummy) filter for audio */ |
There was a problem hiding this comment.
Magic strings like "null" and "anull" used as filter specifications should be defined as constants to improve code readability and maintainability.
| if ((ret = ffmpeg.av_read_frame(ifmt_ctx, packet)) < 0) | ||
| break; |
There was a problem hiding this comment.
Ensure that all possible error codes returned by ffmpeg.av_read_frame are handled appropriately, not just AVERROR_EOF.
| if ((ret = open_input_file(argv[1])) < 0) | ||
| goto end; | ||
| if ((ret = open_output_file(argv[2])) < 0) | ||
| goto end; |
There was a problem hiding this comment.
Consider refactoring the use of goto statements for error handling to a more structured approach, such as try-catch blocks or other control flow mechanisms.
| ffmpeg.avformat_alloc_output_context2(&ofmt_ctx_fixed, null, null, filename); | ||
| if (ofmt_ctx_fixed == null) |
There was a problem hiding this comment.
The return value of ffmpeg.avformat_alloc_output_context2 is not checked. Ensure that the function succeeded and handle the case where it returns a null pointer.
| ffmpeg.av_packet_unref(packet); | ||
| ffmpeg.av_packet_free(&packet); | ||
| ffmpeg.av_frame_free(&frame); | ||
| for (i = 0; i < ifmt_ctx->nb_streams; i++) | ||
| { | ||
| ffmpeg.avcodec_free_context(&stream_ctx[i].dec_ctx); | ||
| if (ofmt_ctx != null && (ofmt_ctx->nb_streams > i) && ofmt_ctx->streams[i] != null && stream_ctx[i].enc_ctx != null) | ||
| ffmpeg.avcodec_free_context(&stream_ctx[i].enc_ctx); | ||
| if (filter_ctx != null && filter_ctx[i].filter_graph != null) | ||
| ffmpeg.avfilter_graph_free(&filter_ctx[i].filter_graph); | ||
| } | ||
|
|
||
| ffmpeg.av_free(filter_ctx); | ||
| ffmpeg.av_free(stream_ctx); | ||
|
|
||
| AVFormatContext* ifmt_ctx_fixed = ifmt_ctx; | ||
| ffmpeg.avformat_close_input(&ifmt_ctx_fixed); | ||
|
|
||
| if (ofmt_ctx != null && (ofmt_ctx->oformat->flags & ffmpeg.AVFMT_NOFILE) == 0) | ||
| ffmpeg.avio_closep(&ofmt_ctx->pb); | ||
| ffmpeg.avformat_free_context(ofmt_ctx); | ||
|
|
||
| if (ret < 0) |
There was a problem hiding this comment.
Before freeing resources in the 'end' label section, check if the pointers are null to avoid potential undefined behavior.
targets openH264 rather than the GPL x264 encoder
modifies FFmpeg.AutoGen.Example to output an MP4 rather than .h264 file by way of:
includes a somewhat mangled but updated and somewhat working version of the Transcoding gist shared by Ruslan-B
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Chores