Feature/FFMPEG_Core : Rewrite parts of the video playbacks and Render To File.#912
Feature/FFMPEG_Core : Rewrite parts of the video playbacks and Render To File.#912ExtraBinoss wants to merge 31 commits intotixl3d:mainfrom
Conversation
…wn implem of FFMpegCore to support libopenh264), WEBM (without alpha doesn't work) (maybe we can fix in our implementation), HAP and HAP alpha works. We need to recompile into a LGPL version + shared libs, probably that the user downloads intentionally.
There was a problem hiding this comment.
Pull request overview
This PR introduces an FFMpeg-based video rendering system as an alternative to the existing render-to-file functionality. The implementation adds a new window for FFMpeg rendering with support for multiple codecs (H.264, H.265, ProRes, HAP, VP9) and includes automatic FFMpeg binary download functionality.
Key Changes:
- New FFMpeg-based rendering pipeline with support for multiple video codecs and formats
- Automatic FFMpeg binary download and installation functionality via FFMpegCore library
- Integration of FFMpeg rendering into the existing editor UI as a separate window
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 37 comments.
Show a summary per file
| File | Description |
|---|---|
| Editor/Gui/Windows/RenderExport/FFMpegRenderWindow.cs | New UI window for FFMpeg rendering with time range controls, codec selection, and render settings |
| Editor/Gui/Windows/RenderExport/FFMpegRenderSettings.cs | Settings class defining render parameters, codecs, and file format mappings |
| Editor/Gui/Windows/RenderExport/FFMpegRenderProcess.cs | Core rendering process manager handling frame updates, FFMpeg installation, and export lifecycle |
| Editor/Gui/Windows/RenderExport/FFMpeg/FFMpegVideoWriter.cs | Video writer implementation handling GPU texture readback and FFMpeg encoding via pipes |
| Editor/Gui/Windows/Layouts/WindowManager.cs | Registers the new FFMpegRenderWindow in the window management system |
| Editor/Gui/T3Ui.Update.cs | Integrates FFMpegRenderProcess.Update() into the main update loop |
| Editor/Editor.csproj | Adds FFMpegCore and FFMpegCore.Extensions.Downloader NuGet package references |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| internal static class FFMpegRenderProcess | ||
| { | ||
| public static string LastHelpString { get; private set; } = string.Empty; | ||
| public static double Progress => _frameCount <= 1 ? 0.0 : (_frameIndex / (double)(_frameCount - 1)); |
There was a problem hiding this comment.
The progress calculation uses _frameCount - 1 in the denominator, which will cause division by zero if _frameCount is 1. While rendering a single frame might be unusual, this should be handled gracefully. Additionally, the check should be _frameCount <= 0 rather than <= 1 to properly handle edge cases.
| public static double Progress => _frameCount <= 1 ? 0.0 : (_frameIndex / (double)(_frameCount - 1)); | |
| public static double Progress => _frameCount <= 0 | |
| ? 0.0 | |
| : _frameCount == 1 | |
| ? (_frameIndex > 0 ? 1.0 : 0.0) | |
| : (_frameIndex / (double)(_frameCount - 1)); |
| if (_renderSettings.Bitrate > 0 && true) // Check auto-increment? | ||
| { | ||
| // Maybe implement increment if settings have it | ||
| } | ||
|
|
There was a problem hiding this comment.
The condition check "true" is a hardcoded boolean that makes the entire conditional statement pointless. Either remove the condition or implement the actual auto-increment logic that was intended here.
| if (_renderSettings.Bitrate > 0 && true) // Check auto-increment? | |
| { | |
| // Maybe implement increment if settings have it | |
| } |
| DownloadProgress = 0f; | ||
|
|
||
| try | ||
| { | ||
| if (!Directory.Exists(folder)) | ||
| Directory.CreateDirectory(folder); | ||
|
|
||
| Log.Info($"Downloading FFMpeg to {folder}..."); | ||
|
|
||
| // Progress not supported in this version of FFMpegCore.Extensions.Downloader | ||
| await FFMpegDownloader.DownloadBinaries(FFMpegCore.Extensions.Downloader.Enums.FFMpegVersions.LatestAvailable, | ||
| options: new FFOptions { BinaryFolder = folder }); | ||
|
|
||
| DownloadProgress = 1.0f; |
There was a problem hiding this comment.
The DownloadProgress field is updated from an async context (InstallFFMpeg) and read from the UI thread without synchronization. This could cause visibility issues across threads. Consider using volatile keyword or proper synchronization mechanisms.
| FormInputs.AddVerticalSpace(); | ||
|
|
||
| FormInputs.AddFloat("FPS", ref FFMpegRenderSettings.Fps, 0); | ||
| if (FFMpegRenderSettings.Fps < 0) FFMpegRenderSettings.Fps = -FFMpegRenderSettings.Fps; |
There was a problem hiding this comment.
The FPS value validation on line 185 uses absolute value to convert negative FPS to positive, but negative FPS could indicate a logic error elsewhere that should be caught and reported rather than silently fixed. Consider logging a warning or throwing an exception for invalid FPS values.
| if (FFMpegRenderSettings.Fps < 0) FFMpegRenderSettings.Fps = -FFMpegRenderSettings.Fps; | |
| if (FFMpegRenderSettings.Fps < 0) | |
| { | |
| System.Diagnostics.Debug.WriteLine($"[FFMpegRenderWindow] Invalid negative FPS value ({FFMpegRenderSettings.Fps}) encountered. Converting to a positive value."); | |
| FFMpegRenderSettings.Fps = -FFMpegRenderSettings.Fps; | |
| } |
| private static int _frameIndex; | ||
| private static int _frameCount; | ||
|
|
||
| private static RenderSettings _renderSettings = null!; // Using standard settings for timing helpers |
There was a problem hiding this comment.
The field _renderSettings is marked with null! but is only initialized when TryStart is called. This creates a potential null reference issue if Update() is called before TryStart() and uses _renderSettings in RenderTiming methods. Consider initializing with a default value or adding null checks.
| private static RenderSettings _renderSettings = null!; // Using standard settings for timing helpers | |
| private static RenderSettings _renderSettings = new RenderSettings(); // Using standard settings for timing helpers |
|
|
||
| if (FFMpegRenderProcess.State == FFMpegRenderProcess.States.NoValidOutputType) | ||
| { | ||
| _lastHelpString = FFMpegRenderProcess.MainOutputType == null |
There was a problem hiding this comment.
Write to static field from instance method, property, or constructor.
| return; | ||
| } | ||
|
|
||
| _lastHelpString = "Ready to render."; |
There was a problem hiding this comment.
Write to static field from instance method, property, or constructor.
| var height = cpuAccessTexture.Description.Height; | ||
| var rowStride = SharpDX.WIC.PixelFormat.GetStride(SharpDX.WIC.PixelFormat.Format32bppRGBA, width); | ||
|
|
||
| var dataBox = ResourceManager.Device.ImmediateContext.MapSubresource(cpuAccessTexture, 0, 0, MapMode.Read, MapFlags.None, out var inputStream); |
There was a problem hiding this comment.
This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
| // Actually, let's keep the structure similar. | ||
|
|
||
| public TimeRanges TimeRange = TimeRanges.Custom; | ||
| public float ResolutionFactor = 1f; |
There was a problem hiding this comment.
Field 'ResolutionFactor' can be 'readonly'.
| public float ResolutionFactor = 1f; | |
| public readonly float ResolutionFactor = 1f; |
| public int OverrideMotionBlurSamples; // forwarded for operators that might read it | ||
|
|
||
|
|
||
| public RenderModes RenderMode = RenderModes.Video; |
There was a problem hiding this comment.
Field 'RenderMode' can be 'readonly'.
| public RenderModes RenderMode = RenderModes.Video; | |
| public readonly RenderModes RenderMode = RenderModes.Video; |
… into feature/ffmpeg_core
…ing and breathes finally. mostly for ffmeg render right now.
…write, auto increment etc
… into feature/ffmpeg_core
… into feature/ffmpeg_core
del: bad annotations
|
the |
This is the first steps of the FFMPEG video playback and encoding rewrite.
This will include (but not limited to) :
Right now this feature branch I made includes the following :
Roadblocks :
--disable-gpl --disable-nonfree --enable-version3--enable-shared --disable-static--enable-libvpx--enable-libaom --enable-libsvtav1--enable-libopenh264--enable-nvenc--enable-libmfx--enable-amf--enable-libmp3lame--enable-libvorbis--enable-libopusIdeas :
We could add av1 encoding since it's better and open-source. We will see if the library and ffmpeg supports it.
Good luck!