On Wed, 5 Feb 2025 03:00:17 GMT, Alexander Matveev <almat...@openjdk.org> wrote:
> - Added new class `CMFGSTBuffer` which can allocate memory internally or > provide GStreamer allocated memory to Media Foundation. > - Added `GstBufferPool` to limit allocation of output buffers used for > rendering (memory will not be allocated for each buffer, but instead will be > reused from pool). Limits are 3 min buffers and 6 max buffers. During testing > 3 buffers was enough. > - Changed `CoInitializeEx` to `COINIT_MULTITHREADED` as per Media Foundation > requirements. > - Added error handling for `ProcessOutput` in case of > https://bugs.openjdk.org/browse/JDK-8329227. With error handling > `MediaPlayer` fails nicely instead of silent hang. The code looks reasonable. I left a few minor questions / comments. I'll reapprove if you make any changes. All my testing looks good (I only tested on Windows, since this is in Windows-only code). modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfgstbuffer.cpp line 215: > 213: return E_INVALIDARG; > 214: > 215: // If we have GStreamer get buffer cllback set, then call it to get Minor typo: `cllback` --> `callback` modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfgstbuffer.cpp line 242: > 240: } > 241: // Lock can be called multiple times, so if we have GStreamer buffer > 242: // allocated just return it. Is it actually possible that Lock can be called multiple times? If it is, then I see that you don't keep track of the number of calls to Lock, so is it correct to assume that the caller does not match Lock / Unlock calls, but rather calls Unlock only once regardless of the number of calls to Lock? modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfgstbuffer.h line 69: > 67: HRESULT AllocateOrGetBuffer(BYTE **ppbBuffer); > 68: > 69: private: Minor: isn't this redundant? modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 911: > 909: > 910: // Gets max length of configured media buffer we using for final > rendering from > 911: // decoder ot color convert. Minor: "ot" --> "of" ? ------------- Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1695#pullrequestreview-2613223582 PR Review Comment: https://git.openjdk.org/jfx/pull/1695#discussion_r1953422605 PR Review Comment: https://git.openjdk.org/jfx/pull/1695#discussion_r1953457463 PR Review Comment: https://git.openjdk.org/jfx/pull/1695#discussion_r1953409208 PR Review Comment: https://git.openjdk.org/jfx/pull/1695#discussion_r1953447469
