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

[Alexander Matveev]

On Wed, 12 Feb 2025 21:25:15 GMT, Kevin Rushforth <kcr 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.
>
> 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`

Fixed.

> 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?

Yes, based on documentation Lock() can be called multiple times to acquire pointer and it should be valid until last Unlock() is called. The caller is responsible to match Lock / Unlock calls. So, current implementation is not correct. I will fixed it. Also, based on documentation Lock() can be called from separate threads, so it should be thread safe. I will fix it as well.

> 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?

Yes, fixed.

> 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" ?

Should be "or". Fixed.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1695#discussion_r1953718149
PR Review Comment: https://git.openjdk.org/jfx/pull/1695#discussion_r1953719959
PR Review Comment: https://git.openjdk.org/jfx/pull/1695#discussion_r1953718066
PR Review Comment: https://git.openjdk.org/jfx/pull/1695#discussion_r1953718323