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