RFR: 8337960: Improve performance of mfwrapper by reusing GStreamer media buffers for decoded video

[Kevin Rushforth]

On Wed, 5 Feb 2025 03:00:17 GMT, Alexander Matveev <almatvee at 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