Re: RFR: 8342913: Remove calls to doPrivileged in media [v2]
On Wed, 30 Oct 2024 18:24:26 GMT, Andy Goryachev wrote: >> Removes doPrivileged in the following module: >> >> - javafx.media >> >> See JDK-8342441 for details. >> >> As a helpful hint for reviewers, I recommend reviewing this using the "Hide >> whitespace" option. > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - review comments > - Merge remote-tracking branch 'origin/master' into > 8342913.do.privileged.media > - Merge remote-tracking branch 'origin/master' into > 8342913.do.privileged.media > - 8342913: Remove calls to doPrivileged in media Looks good. - Marked as reviewed by almatvee (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1612#pullrequestreview-2406352464
[jfx23u] Integrated: 8338701: Provide media support for libavcodec version 61
On Wed, 4 Sep 2024 00:40:22 GMT, Alexander Matveev wrote: > Clean backport of JDK-8338701. This pull request has now been integrated. Changeset: aba60fda Author: Alexander Matveev URL: https://git.openjdk.org/jfx23u/commit/aba60fda1c82f00e8e685107592305c403a31287 Stats: 56 lines in 6 files changed: 45 ins; 0 del; 11 mod 8338701: Provide media support for libavcodec version 61 Backport-of: 6115b396bacf62f39dcaa93c7c0adcd60b428b8c - PR: https://git.openjdk.org/jfx23u/pull/18
[jfx23u] RFR: 8338701: Provide media support for libavcodec version 61
Clean backport of JDK-8338701. - Commit messages: - Backport 6115b396bacf62f39dcaa93c7c0adcd60b428b8c Changes: https://git.openjdk.org/jfx23u/pull/18/files Webrev: https://webrevs.openjdk.org/?repo=jfx23u&pr=18&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8338701 Stats: 56 lines in 6 files changed: 45 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jfx23u/pull/18.diff Fetch: git fetch https://git.openjdk.org/jfx23u.git pull/18/head:pull/18 PR: https://git.openjdk.org/jfx23u/pull/18
Integrated: 8338701: Provide media support for libavcodec version 61
On Thu, 29 Aug 2024 21:02:41 GMT, Alexander Matveev wrote: > - Added support for libavcodec 61. > - Updated AV plugins to use new APIs instead of APIs which were removed in 61. > - We still using some deprecated APIs. > - `reordered_opaque` replacement exist in 61 which is used to pass user data > between compressed frame and uncompressed. In 61 this variable is pointer to > `void` instead of `int64_t`. We can use new `opaque` to pass PTS, but it is > not documented way to pass PTS between compressed and uncompressed frames. > Updated code will be using `AVPacket.pts/AVFrame.pts` which is documented way > to provide PTS. > - Tested on Ubuntu 24.10 with 61 and 24.04 with 60 with all supported formats. This pull request has now been integrated. Changeset: 6115b396 Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/6115b396bacf62f39dcaa93c7c0adcd60b428b8c Stats: 56 lines in 6 files changed: 45 ins; 0 del; 11 mod 8338701: Provide media support for libavcodec version 61 Reviewed-by: sykora, arapte - PR: https://git.openjdk.org/jfx/pull/1552
RFR: 8338701: Provide media support for libavcodec version 61
- Added support for libavcodec 61. - Updated AV plugins to use new APIs instead of APIs which were removed in 61. - We still using some deprecated APIs. - `reordered_opaque` replacement exist in 61 which is used to pass user data between compressed frame and uncompressed. In 61 this variable is pointer to `void` instead of `int64_t`. We can use new `opaque` to pass PTS, but it is not documented way to pass PTS between compressed and uncompressed frames. Updated code will be using `AVPacket.pts/AVFrame.pts` which is documented way to provide PTS. - Tested on Ubuntu 24.10 with 61 and 24.04 with 60 with all supported formats. - Commit messages: - 8338701: Provide media support for libavcodec version 61 Changes: https://git.openjdk.org/jfx/pull/1552/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1552&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8338701 Stats: 56 lines in 6 files changed: 45 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/1552.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1552/head:pull/1552 PR: https://git.openjdk.org/jfx/pull/1552
[jfx23u] Integrated: 8336940: Update GStreamer to 1.24.6
On Fri, 23 Aug 2024 22:26:56 GMT, Alexander Matveev wrote: > Clean backport of JDK-8336940 and JDK-8336939. This pull request has now been integrated. Changeset: 4030ad96 Author: Alexander Matveev URL: https://git.openjdk.org/jfx23u/commit/4030ad96ba6717e4a3a8f0591aef27659f3ec080 Stats: 24630 lines in 329 files changed: 13062 ins; 6878 del; 4690 mod 8336940: Update GStreamer to 1.24.6 8336939: Update Glib to 2.80.4 Backport-of: b88ac0495650bd033ba11e3131e9bffc517872eb - PR: https://git.openjdk.org/jfx23u/pull/14
[jfx23u] RFR: 8336940: Update GStreamer to 1.24.6
Clean backport of JDK-8336940 and JDK-8336939. - Commit messages: - Backport b88ac0495650bd033ba11e3131e9bffc517872eb Changes: https://git.openjdk.org/jfx23u/pull/14/files Webrev: https://webrevs.openjdk.org/?repo=jfx23u&pr=14&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8336940 Stats: 24630 lines in 329 files changed: 13062 ins; 6878 del; 4690 mod Patch: https://git.openjdk.org/jfx23u/pull/14.diff Fetch: git fetch https://git.openjdk.org/jfx23u.git pull/14/head:pull/14 PR: https://git.openjdk.org/jfx23u/pull/14
Integrated: 8336940: Update GStreamer to 1.24.6
On Wed, 21 Aug 2024 20:15:00 GMT, Alexander Matveev wrote: > - Updated GStreamer to 1.24.6 and Glib to 2.80.4. > - Testing done on all platforms with all supported formats. > - Removed gstpluginloader.c. This file contains additional plugin loading > mechanism which we do not use. Latest GStreamer modified it heavily and it > did not compile without pulling additional dependency from GLib, but since we > do not use it anyway it was removed. Corresponding calls to gstpluginloader.c > from gstregistry.c were disabled as well. > - Removed DSD support from Alsa audio sink. Latest GStreamer added support > for Alsa DSD (Direct Stream Digital), but we do not use it so it was removed. > Also, such support requires newer libasound2 (ALSA) which is not available on > older Linux distributions. > - No other changes or bug fixes are done. This pull request has now been integrated. Changeset: b88ac049 Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/b88ac0495650bd033ba11e3131e9bffc517872eb Stats: 24630 lines in 329 files changed: 13062 ins; 6878 del; 4690 mod 8336940: Update GStreamer to 1.24.6 8336939: Update Glib to 2.80.4 Reviewed-by: kcr, sykora - PR: https://git.openjdk.org/jfx/pull/1542
[jfx23u] Integrated: 8336938: Update libFFI to 3.4.6
On Wed, 21 Aug 2024 01:15:49 GMT, Alexander Matveev wrote: > - Clean backport of JDK-8336938. This pull request has now been integrated. Changeset: 499fb77b Author: Alexander Matveev URL: https://git.openjdk.org/jfx23u/commit/499fb77b16de1145cd0b8b5adad0995a8ba8f403 Stats: 1056 lines in 26 files changed: 388 ins; 373 del; 295 mod 8336938: Update libFFI to 3.4.6 Backport-of: f1bac5a829e1f2b75b863c4f0822bf418107a1fb - PR: https://git.openjdk.org/jfx23u/pull/11
Re: RFR: 8336940: Update GStreamer to 1.24.6 [v2]
On Wed, 21 Aug 2024 21:36:52 GMT, Alexander Matveev wrote: >> - Updated GStreamer to 1.24.6 and Glib to 2.80.4. >> - Testing done on all platforms with all supported formats. >> - Removed gstpluginloader.c. This file contains additional plugin loading >> mechanism which we do not use. Latest GStreamer modified it heavily and it >> did not compile without pulling additional dependency from GLib, but since >> we do not use it anyway it was removed. Corresponding calls to >> gstpluginloader.c from gstregistry.c were disabled as well. >> - Removed DSD support from Alsa audio sink. Latest GStreamer added support >> for Alsa DSD (Direct Stream Digital), but we do not use it so it was >> removed. Also, such support requires newer libasound2 (ALSA) which is not >> available on older Linux distributions. >> - No other changes or bug fixes are done. > > Alexander Matveev has updated the pull request incrementally with one > additional commit since the last revision: > > 8336940: Update GStreamer to 1.24.6 [v3] 8336940: Update GStreamer to 1.24.6 [v2] - Fixed whitespace. 8336940: Update GStreamer to 1.24.6 [v3] - Added missing header file. - PR Comment: https://git.openjdk.org/jfx/pull/1542#issuecomment-2303064728
Re: RFR: 8336940: Update GStreamer to 1.24.6 [v2]
> - Updated GStreamer to 1.24.6 and Glib to 2.80.4. > - Testing done on all platforms with all supported formats. > - Removed gstpluginloader.c. This file contains additional plugin loading > mechanism which we do not use. Latest GStreamer modified it heavily and it > did not compile without pulling additional dependency from GLib, but since we > do not use it anyway it was removed. Corresponding calls to gstpluginloader.c > from gstregistry.c were disabled as well. > - Removed DSD support from Alsa audio sink. Latest GStreamer added support > for Alsa DSD (Direct Stream Digital), but we do not use it so it was removed. > Also, such support requires newer libasound2 (ALSA) which is not available on > older Linux distributions. > - No other changes or bug fixes are done. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8336940: Update GStreamer to 1.24.6 [v3] - Changes: - all: https://git.openjdk.org/jfx/pull/1542/files - new: https://git.openjdk.org/jfx/pull/1542/files/8d505f81..1e5499a9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1542&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1542&range=00-01 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1542.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1542/head:pull/1542 PR: https://git.openjdk.org/jfx/pull/1542
RFR: 8336940: Update GStreamer to 1.24.6
- Updated GStreamer to 1.24.6 and Glib to 2.80.4. - Testing done on all platforms with all supported formats. - Removed gstpluginloader.c. This file contains additional plugin loading mechanism which we do not use. Latest GStreamer modified it heavily and it did not compile without pulling additional dependency from GLib, but since we do not use it anyway it was removed. Corresponding calls to gstpluginloader.c from gstregistry.c were disabled as well. - Removed DSD support from Alsa audio sink. Latest GStreamer added support for Alsa DSD (Direct Stream Digital), but we do not use it so it was removed. Also, such support requires newer libasound2 (ALSA) which is not available on older Linux distributions. - No other changes or bug fixes are done. - Commit messages: - 8336940: Update GStreamer to 1.24.6 [v2] - 8336940: Update GStreamer to 1.24.6 Changes: https://git.openjdk.org/jfx/pull/1542/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1542&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8336940 Stats: 24626 lines in 329 files changed: 13058 ins; 6878 del; 4690 mod Patch: https://git.openjdk.org/jfx/pull/1542.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1542/head:pull/1542 PR: https://git.openjdk.org/jfx/pull/1542
[jfx23u] RFR: 8336938: Update libFFI to 3.4.6
- Clean backport of JDK-8336938. - Commit messages: - Backport f1bac5a829e1f2b75b863c4f0822bf418107a1fb Changes: https://git.openjdk.org/jfx23u/pull/11/files Webrev: https://webrevs.openjdk.org/?repo=jfx23u&pr=11&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8336938 Stats: 1056 lines in 26 files changed: 388 ins; 373 del; 295 mod Patch: https://git.openjdk.org/jfx23u/pull/11.diff Fetch: git fetch https://git.openjdk.org/jfx23u.git pull/11/head:pull/11 PR: https://git.openjdk.org/jfx23u/pull/11
Integrated: 8336938: Update libFFI to 3.4.6
On Wed, 7 Aug 2024 00:26:13 GMT, Alexander Matveev wrote: > - libFFI updated to 3.4.6. > - No additional changes are done. > - Tested on Windows, macOS and Linux with all supported formats. This pull request has now been integrated. Changeset: f1bac5a8 Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/f1bac5a829e1f2b75b863c4f0822bf418107a1fb Stats: 1056 lines in 26 files changed: 388 ins; 373 del; 295 mod 8336938: Update libFFI to 3.4.6 Reviewed-by: kcr, sykora - PR: https://git.openjdk.org/jfx/pull/1531
Re: RFR: 8336938: Update libFFI to 3.4.6 [v2]
> - libFFI updated to 3.4.6. > - No additional changes are done. > - Tested on Windows, macOS and Linux with all supported formats. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8336938: Update libFFI to 3.4.6 [v2] - Changes: - all: https://git.openjdk.org/jfx/pull/1531/files - new: https://git.openjdk.org/jfx/pull/1531/files/e51e5c71..c4a9d5b2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1531&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1531&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1531.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1531/head:pull/1531 PR: https://git.openjdk.org/jfx/pull/1531
Re: RFR: 8336938: Update libFFI to 3.4.6
On Mon, 12 Aug 2024 23:15:58 GMT, Kevin Rushforth wrote: > I see one minor difference between the upstream and your PR in a commented > out line of code in `sysv_intel.S`. I don't know whether it is significant (I > guess not, as long as it compiles with the current inline comment style). > > ``` > diff -u -w -r > ../../jfx-tmp/jfx/rt/modules/javafx.media/src/main/native/gstreamer/3rd_party/libffi/src/x86/sysv_intel.S > ./src/x86/sysv_intel.S > --- > ../../jfx-tmp/jfx/rt/modules/javafx.media/src/main/native/gstreamer/3rd_party/libffi/src/x86/sysv_intel.S >2024-08-12 15:59:14.597741100 -0700 > +++ ./src/x86/sysv_intel.S 2024-08-12 16:05:42.708534300 -0700 > @@ -102,7 +102,7 @@ > mov ecx, [12+ebp] /* load return type code */ > mov [ebp+8], ebx/* preserve %ebx */ > L(UW2): > -// cfi_rel_offset(%ebx, 8) > + /* cfi_rel_offset(%ebx, 8) */ > > and ecx, X86_RET_TYPE_MASK > lea ebx, [L(store_table) + ecx * 8] > ``` Yes, it compiles fine. It was changed by libFFI itself in 3.4.6. I think to be inline with rest of comments in this file. - PR Comment: https://git.openjdk.org/jfx/pull/1531#issuecomment-2285153581
RFR: 8336938: Update libFFI to 3.4.6
- libFFI updated to 3.4.6. - No additional changes are done. - Tested on Windows, macOS and Linux with all supported formats. - Commit messages: - Merge remote-tracking branch 'upstream/master' into JDK-8336938 - 8336938: Update libFFI to 3.4.6 Changes: https://git.openjdk.org/jfx/pull/1531/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1531&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8336938 Stats: 1055 lines in 26 files changed: 388 ins; 373 del; 294 mod Patch: https://git.openjdk.org/jfx/pull/1531.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1531/head:pull/1531 PR: https://git.openjdk.org/jfx/pull/1531
[jfx23u] Integrated: 8336277: Colors are incorrect when playing H.265/HEVC on Windows 11
On Tue, 6 Aug 2024 20:52:26 GMT, Alexander Matveev wrote: > - Clean backport of JDK-8336277. This pull request has now been integrated. Changeset: d0c5cd42 Author: Alexander Matveev URL: https://git.openjdk.org/jfx23u/commit/d0c5cd42d46be7773e5872dca0ceb442354755dd Stats: 628 lines in 2 files changed: 467 ins; 61 del; 100 mod 8336277: Colors are incorrect when playing H.265/HEVC on Windows 11 Backport-of: 635a09c7855351eac8001b60e240c826db3a6bff - PR: https://git.openjdk.org/jfx23u/pull/7
[jfx23u] RFR: 8336277: Colors are incorrect when playing H.265/HEVC on Windows 11
- Clean backport of JDK-8336277. - Commit messages: - Backport 635a09c7855351eac8001b60e240c826db3a6bff Changes: https://git.openjdk.org/jfx23u/pull/7/files Webrev: https://webrevs.openjdk.org/?repo=jfx23u&pr=7&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8336277 Stats: 628 lines in 2 files changed: 467 ins; 61 del; 100 mod Patch: https://git.openjdk.org/jfx23u/pull/7.diff Fetch: git fetch https://git.openjdk.org/jfx23u.git pull/7/head:pull/7 PR: https://git.openjdk.org/jfx23u/pull/7
Integrated: 8336277: Colors are incorrect when playing H.265/HEVC on Windows 11
On Wed, 31 Jul 2024 03:45:20 GMT, Alexander Matveev wrote: > - For some reason H.265 decoder on Windows accepts proposed media format > without error, but does not actually change output format. > - For 8-bit we proposed IYUV, but decoder outputs NV12. For 10-bit we > proposed NV12, but decoder outputs P010. As result colors where not correct > during rendering. > - To detect such condition we will propose media format and then read it back > to determine actual decoder output format. > - Added color conversion for P010 format, which was missing. P010 conversion > is done in two stages (P010->NV12->IYUV), since color converter does not > support direct conversion from P010 to IYUV. > - Note: Color conversion from P010->NV12->IYUV and NV12->IYUV is temporary > solution and will be disabled/removed once JDK-8337686 is implemented. > JDK-8337686 will add native support for P010 and NV12 to Graphics. > - Added debug trace for formats. Disabled by default. This pull request has now been integrated. Changeset: 635a09c7 Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/635a09c7855351eac8001b60e240c826db3a6bff Stats: 628 lines in 2 files changed: 467 ins; 61 del; 100 mod 8336277: Colors are incorrect when playing H.265/HEVC on Windows 11 Reviewed-by: kcr, arapte, angorya - PR: https://git.openjdk.org/jfx/pull/1525
Re: RFR: 8336277: Colors are incorrect when playing H.265/HEVC on Windows 11
On Thu, 1 Aug 2024 23:56:57 GMT, Andy Goryachev wrote: > > I think this issue/PR should specify it. > > The main reason to mention what needs to be corrected is to help the person > who will be working on JDK-8337686 many years from now ;-) Done. - PR Comment: https://git.openjdk.org/jfx/pull/1525#issuecomment-2266118943
Re: RFR: 8336277: Colors are incorrect when playing H.265/HEVC on Windows 11
On Thu, 1 Aug 2024 21:21:45 GMT, Andy Goryachev wrote: > > JDK-8337686 > > thank you for filing the RFE. Should JDK-8337686 explicitly specify that the > double conversion introduced in this PR be replaced? I think this issue/PR should specify it. I just updated description for this PR that color conversion is temporary solution. - PR Comment: https://git.openjdk.org/jfx/pull/1525#issuecomment-2264196640
Re: RFR: 8336277: Colors are incorrect when playing H.265/HEVC on Windows 11
On Thu, 1 Aug 2024 15:12:15 GMT, Andy Goryachev wrote: > This might sound silly, but these formats seem to differ by the way color > bits are laid out in memory - would it be possible to write a utility that > does the direct conversion instead of calling two conversion methods? NV12 and IYUV are different in memory layout only, but P010 is 10-bit per pixel and NV12 is 8-bit per pixel. I filed enhancement request https://bugs.openjdk.org/browse/JDK-8337686 to support P010 and NV12 directly by Graphics. I think it is better approach, then doing conversion in Media. For now this approach is good enough until JDK-8337686 is implemented. - PR Comment: https://git.openjdk.org/jfx/pull/1525#issuecomment-2263960073
RFR: 8336277: Colors are incorrect when playing H.265/HEVC on Windows 11
- For some reason H.265 decoder on Windows accepts proposed media format without error, but does not actually change output format. - For 8-bit we proposed IYUV, but decoder outputs NV12. For 10-bit we proposed NV12, but decoder outputs P010. As result colors where not correct during rendering. - To detect such condition we will propose media format and then read it back to determine actual decoder output format. - Added color conversion for P010 format, which was missing. P010 conversion is done in two stages (P010->NV12->IYUV), since color converter does not support direct conversion from P010 to IYUV. - Added debug trace for formats. Disabled by default. - Commit messages: - 8336277: Colors are incorrect when playing H.265/HEVC on Windows 11 Changes: https://git.openjdk.org/jfx/pull/1525/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1525&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8336277 Stats: 628 lines in 2 files changed: 467 ins; 61 del; 100 mod Patch: https://git.openjdk.org/jfx/pull/1525.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1525/head:pull/1525 PR: https://git.openjdk.org/jfx/pull/1525
Integrated: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming
On Tue, 2 Apr 2024 00:18:04 GMT, Alexander Matveev wrote: > - Added support for #EXT-X-MEDIA tag to HTTP Live Streaming. > - Following audio renditions via #EXT-X-MEDIA tag will be supported (see CSR > for more details): > - MP2T streams with one H.264/AVC video track and elementary AAC audio > stream via #EXT-X-MEDIA tag. > - fMP4 streams with one H.264/AVC or H.265/HEVC video track and elementary > AAC audio stream via #EXT-X-MEDIA tag. > - fMP4 streams with one H.264/AVC or H.265/HEVC video track and fMP4 > streams with one AAC audio track via #EXT-X-MEDIA tag. > - Separate audio stream will be playback via separate chain of GStreamer > elements inside one pipeline. Which means two "javasource" elements will be > used inside one pipeline and they will be reading data independently of each > other via two separate HLSConnectionHolders. GStreamer will handle audio and > video synchronization based on PTS as for other streams. Other solutions were > considered such as one "javasource" with multiple source pads, but such > implementation will be more complex and does not provide any benefits. > - HLSConnectionHolder which handles video stream will also parse all > information for separate audio stream and then create child > HLSConnectionHolder for separate audio stream which will be responsible for > downloading audio segments and seek of audio streams. > - Parser in HLSConnectionHolder was reworked to make it more readable and > easy to maintain and extend. > - JavaDoc updated to point to latest HLS implementation vs old draft. It also > updated with information on #EXT-X-MEDIA tag. Also, added missing information > on AAC elementary streams and fMP4 from previous fixes. > - Fixed and improved debug output in Linux AV plugins. > - Added new property to "dshowwrapper" to disable PTS reset for each new > segment, since with #EXT-X-MEDIA tag audio and video segments are not align > and they can start at different time. > - Fixed missing PTS on first buffer after seek in MP2T demuxer in > "dshowwrapper". Without it audio and video synchronization breaks with two > separate streams. > - Removed dead code from MediaManager. > - Added handling for GST_MESSAGE_LATENCY. Based on GStreamer doc we need to > call gst_bin_recalculate_latency() when such message received. Not sure if we > really need to do this, but with separate video and audio streams we do > receive this message when seek is done. Most likely due to video and audio is > not align perfectly when we seek. For other streams this message is not > received in most cases. This pull request has now been integrated. Changeset: b86e6771 Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/b86e6771b906bdf7ed517eb2d5ab76a2e92c1282 Stats: 1939 lines in 30 files changed: 1170 ins; 321 del; 448 mod 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming Reviewed-by: kcr, arapte - PR: https://git.openjdk.org/jfx/pull/1435
Integrated: 8328603: HLS video stream fails to render consistently
On Sat, 13 Apr 2024 01:36:01 GMT, Alexander Matveev wrote: > - When video fails to render AVFoundation outputs frame in > `kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange` format which is not > supported. We do force format change after that to > `kCVPixelFormatType_422YpCbCr8`, but AVFoundation does not provides any video > frames after format change. Not sure why it happens. > - When video worked for stream in this issue, then AVFoundation was using > `kCVPixelFormatType_422YpCbCr8` for some reason instead of > `kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange`. > - I tested format fallback from > `kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange` to > `kCVPixelFormatType_422YpCbCr8` manually and many streams I tried works fine, > except one reported in this bug. > - If AVFoundation is initialized with list of formats JavaFX Media rendering > supports, then this issue is no longer reproducible. > - Fixed by providing list of supported formats to AVFoundation. > - Removed unused variable `response`. > - Tested with all streams I have and no issues found. This pull request has now been integrated. Changeset: eb6d55f7 Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/eb6d55f7f067469e89402da2bfa46e1ecf11ea02 Stats: 19 lines in 1 file changed: 16 ins; 1 del; 2 mod 8328603: HLS video stream fails to render consistently Reviewed-by: kcr, arapte - PR: https://git.openjdk.org/jfx/pull/1440
Re: RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v4]
> - Added support for #EXT-X-MEDIA tag to HTTP Live Streaming. > - Following audio renditions via #EXT-X-MEDIA tag will be supported (see CSR > for more details): > - MP2T streams with one H.264/AVC video track and elementary AAC audio > stream via #EXT-X-MEDIA tag. > - fMP4 streams with one H.264/AVC or H.265/HEVC video track and elementary > AAC audio stream via #EXT-X-MEDIA tag. > - fMP4 streams with one H.264/AVC or H.265/HEVC video track and fMP4 > streams with one AAC audio track via #EXT-X-MEDIA tag. > - Separate audio stream will be playback via separate chain of GStreamer > elements inside one pipeline. Which means two "javasource" elements will be > used inside one pipeline and they will be reading data independently of each > other via two separate HLSConnectionHolders. GStreamer will handle audio and > video synchronization based on PTS as for other streams. Other solutions were > considered such as one "javasource" with multiple source pads, but such > implementation will be more complex and does not provide any benefits. > - HLSConnectionHolder which handles video stream will also parse all > information for separate audio stream and then create child > HLSConnectionHolder for separate audio stream which will be responsible for > downloading audio segments and seek of audio streams. > - Parser in HLSConnectionHolder was reworked to make it more readable and > easy to maintain and extend. > - JavaDoc updated to point to latest HLS implementation vs old draft. It also > updated with information on #EXT-X-MEDIA tag. Also, added missing information > on AAC elementary streams and fMP4 from previous fixes. > - Fixed and improved debug output in Linux AV plugins. > - Added new property to "dshowwrapper" to disable PTS reset for each new > segment, since with #EXT-X-MEDIA tag audio and video segments are not align > and they can start at different time. > - Fixed missing PTS on first buffer after seek in MP2T demuxer in > "dshowwrapper". Without it audio and video synchronization breaks with two > separate streams. > - Removed dead code from MediaManager. > - Added handling for GST_MESSAGE_LATENCY. Based on GStreamer doc we need to > call gst_bin_recalculate_latency() when such message received. Not sure if we > really need to do this, but with separate video and audio streams we do > receive this message when seek is done. Most likely due to video and audio is > not align perfectly when we seek. For other streams this message is not > received in most cases. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming [v3] - Changes: - all: https://git.openjdk.org/jfx/pull/1435/files - new: https://git.openjdk.org/jfx/pull/1435/files/84103bd4..4e2e1572 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1435&range=03 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1435&range=02-03 Stats: 1198 lines in 2 files changed: 284 ins; 296 del; 618 mod Patch: https://git.openjdk.org/jfx/pull/1435.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1435/head:pull/1435 PR: https://git.openjdk.org/jfx/pull/1435
Re: RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v3]
On Wed, 8 May 2024 12:26:06 GMT, Kevin Rushforth wrote: >> modules/javafx.media/src/main/native/jfxmedia/Locator/Locator.cpp line 123: >> >>> 121: >>> "(Lcom/sun/media/jfxmedia/locator/ConnectionHolder;)Lcom/sun/media/jfxmedia/locator/ConnectionHolder;"); >>> 122: env->DeleteLocalRef(klass); >>> 123: if (javaEnv.reportException() || >>> (GetAudioStreamConnectionHolder == NULL)) >> >> Observed a build warning (MacOS): >> >> warning: comparison of function 'GetAudioStreamConnectionHolder' equal to a >> null pointer is always false [-Wtautological-pointer-compare] > > Good catch. That looks like a typo to me, and should probably be > `mid_GetAudioStreamConnectionHolder == null` Fixed. - PR Review Comment: https://git.openjdk.org/jfx/pull/1435#discussion_r1594743714
Re: RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v3]
On Tue, 7 May 2024 19:54:52 GMT, Kevin Rushforth wrote: >> Alexander Matveev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming [v2] > > modules/javafx.media/src/main/java/com/sun/media/jfxmedia/locator/HLSConnectionHolder.java > line 58: > >> 56: import static >> com.sun.media.jfxmedia.locator.HLSConnectionHolder.HLS_VALUE_MIMETYPE_MP3; >> 57: import static >> com.sun.media.jfxmedia.locator.HLSConnectionHolder.HLS_VALUE_MIMETYPE_UNKNOWN; >> 58: import static >> com.sun.media.jfxmedia.locator.HLSConnectionHolder.stripParameters; > > If you make the other classes in this file nested static classes, then you > won't need these static imports. Fixed. - PR Review Comment: https://git.openjdk.org/jfx/pull/1435#discussion_r1594747872
Re: RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v3]
On Wed, 8 May 2024 12:39:24 GMT, Kevin Rushforth wrote: >> When these classes were nested classes I got issues with second instance of >> HLSConnectionHolder. If I remember correctly nested classes of second >> instance of HLSConnectionHolder were using fields of first >> HLSConnectionHolder instance. Maybe because I initiated second instance >> incorrectly. To avoid any such potential issues I decided to move away from >> nested classes. I would prefer to keep as is or better to move all nested >> classes under separate package (com.sun.media.jfxmedia.locator.hls). > > Did you declare them as `static` (nested) classes? If not, then yes, they > will have the behavior you mentioned. A non-static "inner" class exists > within an instance of the outer class. A static "nested" class does not. > Other than scoping, a nested class behaves like a top-level class. > > If you do want to keep them as separate top-level classes, then please move > each to its own file. I would not recommend creating a new package, though, > since that will involve more changes to make the classes and elements you > need public. No, I did not declared them as `static`. Fixed by declaring these classes as nested static classes. - PR Review Comment: https://git.openjdk.org/jfx/pull/1435#discussion_r1594747723
Re: RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v3]
On Fri, 26 Apr 2024 23:36:26 GMT, Alexander Matveev wrote: >> - Added support for #EXT-X-MEDIA tag to HTTP Live Streaming. >> - Following audio renditions via #EXT-X-MEDIA tag will be supported (see CSR >> for more details): >> - MP2T streams with one H.264/AVC video track and elementary AAC audio >> stream via #EXT-X-MEDIA tag. >> - fMP4 streams with one H.264/AVC or H.265/HEVC video track and elementary >> AAC audio stream via #EXT-X-MEDIA tag. >> - fMP4 streams with one H.264/AVC or H.265/HEVC video track and fMP4 >> streams with one AAC audio track via #EXT-X-MEDIA tag. >> - Separate audio stream will be playback via separate chain of GStreamer >> elements inside one pipeline. Which means two "javasource" elements will be >> used inside one pipeline and they will be reading data independently of each >> other via two separate HLSConnectionHolders. GStreamer will handle audio and >> video synchronization based on PTS as for other streams. Other solutions >> were considered such as one "javasource" with multiple source pads, but such >> implementation will be more complex and does not provide any benefits. >> - HLSConnectionHolder which handles video stream will also parse all >> information for separate audio stream and then create child >> HLSConnectionHolder for separate audio stream which will be responsible for >> downloading audio segments and seek of audio streams. >> - Parser in HLSConnectionHolder was reworked to make it more readable and >> easy to maintain and extend. >> - JavaDoc updated to point to latest HLS implementation vs old draft. It >> also updated with information on #EXT-X-MEDIA tag. Also, added missing >> information on AAC elementary streams and fMP4 from previous fixes. >> - Fixed and improved debug output in Linux AV plugins. >> - Added new property to "dshowwrapper" to disable PTS reset for each new >> segment, since with #EXT-X-MEDIA tag audio and video segments are not align >> and they can start at different time. >> - Fixed missing PTS on first buffer after seek in MP2T demuxer in >> "dshowwrapper". Without it audio and video synchronization breaks with two >> separate streams. >> - Removed dead code from MediaManager. >> - Added handling for GST_MESSAGE_LATENCY. Based on GStreamer doc we need to >> call gst_bin_recalculate_latency() when such message received. Not sure if >> we really need to do this, but with separate video and audio streams we do >> receive this message when seek is done. Most likely due to video and audio >> is not align perfectly when we seek. For other streams this message is not >> received in most cases. > > Alexander Matveev has updated the pull request incrementally with one > additional commit since the last revision: > > 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming [v2] 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming [v3] - Fixed NULL check in Locator. - Declared HLSConnectionHolder helper classes and static nested classes. - PR Comment: https://git.openjdk.org/jfx/pull/1435#issuecomment-2101574260
Re: RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v3]
On Tue, 7 May 2024 19:53:57 GMT, Kevin Rushforth wrote:
>> Alexander Matveev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming [v2]
>
> modules/javafx.media/src/main/java/com/sun/media/jfxmedia/locator/HLSConnectionHolder.java
> line 480:
>
>> 478: }
>> 479:
>> 480: class PlaylistLoader extends Thread {
>
> Having multiple top-level classes in the same source file is an anti-pattern.
> Is there a good reason that these can't be nested static classes? If not,
> then please make this change. You might be able to then make the nested
> classes private, although there is no harm in leaving them package scope.
When these classes were nested classes I got issues with second instance of
HLSConnectionHolder. If I remember correctly nested classes of second instance
of HLSConnectionHolder were using fields of first HLSConnectionHolder instance.
Maybe because I initiated second instance incorrectly. To avoid any such
potential issues I decided to move away from nested classes. I would prefer to
keep as is or better to move all nested classes under separate package
(com.sun.media.jfxmedia.locator.hls).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1435#discussion_r1593304476
Re: RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v2]
On Fri, 26 Apr 2024 22:58:30 GMT, Alexander Matveev wrote: >> - Added support for #EXT-X-MEDIA tag to HTTP Live Streaming. >> - Following audio renditions via #EXT-X-MEDIA tag will be supported (see CSR >> for more details): >> - MP2T streams with one H.264/AVC video track and elementary AAC audio >> stream via #EXT-X-MEDIA tag. >> - fMP4 streams with one H.264/AVC or H.265/HEVC video track and elementary >> AAC audio stream via #EXT-X-MEDIA tag. >> - fMP4 streams with one H.264/AVC or H.265/HEVC video track and fMP4 >> streams with one AAC audio track via #EXT-X-MEDIA tag. >> - Separate audio stream will be playback via separate chain of GStreamer >> elements inside one pipeline. Which means two "javasource" elements will be >> used inside one pipeline and they will be reading data independently of each >> other via two separate HLSConnectionHolders. GStreamer will handle audio and >> video synchronization based on PTS as for other streams. Other solutions >> were considered such as one "javasource" with multiple source pads, but such >> implementation will be more complex and does not provide any benefits. >> - HLSConnectionHolder which handles video stream will also parse all >> information for separate audio stream and then create child >> HLSConnectionHolder for separate audio stream which will be responsible for >> downloading audio segments and seek of audio streams. >> - Parser in HLSConnectionHolder was reworked to make it more readable and >> easy to maintain and extend. >> - JavaDoc updated to point to latest HLS implementation vs old draft. It >> also updated with information on #EXT-X-MEDIA tag. Also, added missing >> information on AAC elementary streams and fMP4 from previous fixes. >> - Fixed and improved debug output in Linux AV plugins. >> - Added new property to "dshowwrapper" to disable PTS reset for each new >> segment, since with #EXT-X-MEDIA tag audio and video segments are not align >> and they can start at different time. >> - Fixed missing PTS on first buffer after seek in MP2T demuxer in >> "dshowwrapper". Without it audio and video synchronization breaks with two >> separate streams. >> - Removed dead code from MediaManager. >> - Added handling for GST_MESSAGE_LATENCY. Based on GStreamer doc we need to >> call gst_bin_recalculate_latency() when such message received. Not sure if >> we really need to do this, but with separate video and audio streams we do >> receive this message when seek is done. Most likely due to video and audio >> is not align perfectly when we seek. For other streams this message is not >> received in most cases. > > Alexander Matveev has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - Merge remote-tracking branch 'upstream/master' into JDK-8282999 > - 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming [v2] - Added missing header. I do not have Ubuntu 16.04 and this issue does not reproduce on Ubuntu 22.04. - PR Comment: https://git.openjdk.org/jfx/pull/1435#issuecomment-2080234887
Re: RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v3]
> - Added support for #EXT-X-MEDIA tag to HTTP Live Streaming. > - Following audio renditions via #EXT-X-MEDIA tag will be supported (see CSR > for more details): > - MP2T streams with one H.264/AVC video track and elementary AAC audio > stream via #EXT-X-MEDIA tag. > - fMP4 streams with one H.264/AVC or H.265/HEVC video track and elementary > AAC audio stream via #EXT-X-MEDIA tag. > - fMP4 streams with one H.264/AVC or H.265/HEVC video track and fMP4 > streams with one AAC audio track via #EXT-X-MEDIA tag. > - Separate audio stream will be playback via separate chain of GStreamer > elements inside one pipeline. Which means two "javasource" elements will be > used inside one pipeline and they will be reading data independently of each > other via two separate HLSConnectionHolders. GStreamer will handle audio and > video synchronization based on PTS as for other streams. Other solutions were > considered such as one "javasource" with multiple source pads, but such > implementation will be more complex and does not provide any benefits. > - HLSConnectionHolder which handles video stream will also parse all > information for separate audio stream and then create child > HLSConnectionHolder for separate audio stream which will be responsible for > downloading audio segments and seek of audio streams. > - Parser in HLSConnectionHolder was reworked to make it more readable and > easy to maintain and extend. > - JavaDoc updated to point to latest HLS implementation vs old draft. It also > updated with information on #EXT-X-MEDIA tag. Also, added missing information > on AAC elementary streams and fMP4 from previous fixes. > - Fixed and improved debug output in Linux AV plugins. > - Added new property to "dshowwrapper" to disable PTS reset for each new > segment, since with #EXT-X-MEDIA tag audio and video segments are not align > and they can start at different time. > - Fixed missing PTS on first buffer after seek in MP2T demuxer in > "dshowwrapper". Without it audio and video synchronization breaks with two > separate streams. > - Removed dead code from MediaManager. > - Added handling for GST_MESSAGE_LATENCY. Based on GStreamer doc we need to > call gst_bin_recalculate_latency() when such message received. Not sure if we > really need to do this, but with separate video and audio streams we do > receive this message when seek is done. Most likely due to video and audio is > not align perfectly when we seek. For other streams this message is not > received in most cases. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming [v2] - Changes: - all: https://git.openjdk.org/jfx/pull/1435/files - new: https://git.openjdk.org/jfx/pull/1435/files/0187b577..84103bd4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1435&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1435&range=01-02 Stats: 4 lines in 2 files changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1435.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1435/head:pull/1435 PR: https://git.openjdk.org/jfx/pull/1435
Re: RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v2]
> - Added support for #EXT-X-MEDIA tag to HTTP Live Streaming. > - Following audio renditions via #EXT-X-MEDIA tag will be supported (see CSR > for more details): > - MP2T streams with one H.264/AVC video track and elementary AAC audio > stream via #EXT-X-MEDIA tag. > - fMP4 streams with one H.264/AVC or H.265/HEVC video track and elementary > AAC audio stream via #EXT-X-MEDIA tag. > - fMP4 streams with one H.264/AVC or H.265/HEVC video track and fMP4 > streams with one AAC audio track via #EXT-X-MEDIA tag. > - Separate audio stream will be playback via separate chain of GStreamer > elements inside one pipeline. Which means two "javasource" elements will be > used inside one pipeline and they will be reading data independently of each > other via two separate HLSConnectionHolders. GStreamer will handle audio and > video synchronization based on PTS as for other streams. Other solutions were > considered such as one "javasource" with multiple source pads, but such > implementation will be more complex and does not provide any benefits. > - HLSConnectionHolder which handles video stream will also parse all > information for separate audio stream and then create child > HLSConnectionHolder for separate audio stream which will be responsible for > downloading audio segments and seek of audio streams. > - Parser in HLSConnectionHolder was reworked to make it more readable and > easy to maintain and extend. > - JavaDoc updated to point to latest HLS implementation vs old draft. It also > updated with information on #EXT-X-MEDIA tag. Also, added missing information > on AAC elementary streams and fMP4 from previous fixes. > - Fixed and improved debug output in Linux AV plugins. > - Added new property to "dshowwrapper" to disable PTS reset for each new > segment, since with #EXT-X-MEDIA tag audio and video segments are not align > and they can start at different time. > - Fixed missing PTS on first buffer after seek in MP2T demuxer in > "dshowwrapper". Without it audio and video synchronization breaks with two > separate streams. > - Removed dead code from MediaManager. > - Added handling for GST_MESSAGE_LATENCY. Based on GStreamer doc we need to > call gst_bin_recalculate_latency() when such message received. Not sure if we > really need to do this, but with separate video and audio streams we do > receive this message when seek is done. Most likely due to video and audio is > not align perfectly when we seek. For other streams this message is not > received in most cases. Alexander Matveev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into JDK-8282999 - 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming - Changes: - all: https://git.openjdk.org/jfx/pull/1435/files - new: https://git.openjdk.org/jfx/pull/1435/files/c4ac5e9e..0187b577 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1435&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1435&range=00-01 Stats: 5035 lines in 351 files changed: 3411 ins; 991 del; 633 mod Patch: https://git.openjdk.org/jfx/pull/1435.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1435/head:pull/1435 PR: https://git.openjdk.org/jfx/pull/1435
Re: RFR: 8146918: ConcurrentModificationException in MediaPlayer [v3]
On Mon, 22 Apr 2024 15:05:07 GMT, n-gabe wrote: >> There is a ConcurrentModificationException in MediaPlayer when removing a >> MediaView from it. The root cause is that you can't iterate over a HashSet >> with for (WeakReference vref : viewRefs) and removing items from >> the collection by viewRefs.remove(vref); within this loop. > > n-gabe has updated the pull request incrementally with one additional commit > since the last revision: > > Remove needless copyright year Looks good. - Marked as reviewed by almatvee (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1377#pullrequestreview-2015678699
Re: RFR: 8146918: ConcurrentModificationException in MediaPlayer
On Thu, 22 Feb 2024 17:16:42 GMT, n-gabe wrote: > There is a ConcurrentModificationException in MediaPlayer when removing a > MediaView from it. The root cause is that you can't iterate over a HashSet > with for (WeakReference vref : viewRefs) and removing items from > the collection by viewRefs.remove(vref); within this loop. Look good. Can you update copyright year from 2022 to 2024? - Changes requested by almatvee (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1377#pullrequestreview-2012603389
Re: RFR: 8146918: ConcurrentModificationException in MediaPlayer
On Thu, 22 Feb 2024 17:16:42 GMT, n-gabe wrote: > There is a ConcurrentModificationException in MediaPlayer when removing a > MediaView from it. The root cause is that you can't iterate over a HashSet > with for (WeakReference vref : viewRefs) and removing items from > the collection by viewRefs.remove(vref); within this loop. Do we have a test application to reproduce this issue? I tired to create one based on bug description, but was not able to reproduce issue. - PR Comment: https://git.openjdk.org/jfx/pull/1377#issuecomment-2065544045
RFR: 8328603: HLS video stream fails to render consistently
- When video fails to render AVFoundation outputs frame in `kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange` format which is not supported. We do force format change after that to `kCVPixelFormatType_422YpCbCr8`, but AVFoundation does not provides any video frames after format change. Not sure why it happens. - When video worked for stream in this issue, then AVFoundation was using `kCVPixelFormatType_422YpCbCr8` for some reason instead of `kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange`. - I tested format fallback from `kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange` to `kCVPixelFormatType_422YpCbCr8` manually and many streams I tried works fine, except one reported in this bug. - If AVFoundation is initialized with list of formats JavaFX Media rendering supports, then this issue is no longer reproducible. - Fixed by providing list of supported formats to AVFoundation. - Removed unused variable `response`. - Tested with all streams I have and no issues found. - Commit messages: - 8328603: HLS video stream fails to render consistently Changes: https://git.openjdk.org/jfx/pull/1440/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1440&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8328603 Stats: 19 lines in 1 file changed: 16 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1440.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1440/head:pull/1440 PR: https://git.openjdk.org/jfx/pull/1440
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v7]
On Wed, 10 Apr 2024 23:45:56 GMT, Andy Goryachev wrote:
>> ## ManualTestWindow
>>
>> This facility provides the base class for manual tests which displays the
>> test instructions,
>> the UI under test, and the Pass/Fail buttons.
>>
>> Example:
>>
>>
>> public class ManualTestExample extends ManualTestWindow {
>> public ManualTestExample() {
>> super(
>> "Manual Test Example",
>> """
>> Instructions:
>> 1. you will see a button named "Test"
>> 2. press the button
>> 3. verify that the button can be pressed""",
>> 400, 250
>> );
>> }
>>
>> public static void main(String[] args) throws Exception {
>> launch(args);
>> }
>>
>> @Override
>> protected Node createContent() {
>> return new Button("Test");
>> }
>> }
>>
>>
>> Resulting application window:
>>
>> 
>>
>> Readme:
>>
>> https://github.com/andy-goryachev-oracle/jfx/blob/8319555.manual/tests/manual/util/README.md
>>
>> @prrace 's test EmojiTest has been converted to use the new test window as a
>> demonstration (also fixed the Eclipse project so it works now).
>>
>> Q: What other features can be added to the test window?
>>
>> Edit: the sources are left in their original place at the root of the
>> project.
>
> Andy Goryachev has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'origin/8319555.manual' into 8319555.manual
> - cleanup
Marked as reviewed by almatvee (Reviewer).
-
PR Review: https://git.openjdk.org/jfx/pull/1413#pullrequestreview-1992862962
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v6]
On Wed, 10 Apr 2024 23:16:02 GMT, Andy Goryachev wrote:
>> ## ManualTestWindow
>>
>> This facility provides the base class for manual tests which displays the
>> test instructions,
>> the UI under test, and the Pass/Fail buttons.
>>
>> Example:
>>
>>
>> public class ManualTestExample extends ManualTestWindow {
>> public ManualTestExample() {
>> super(
>> "Manual Test Example",
>> """
>> Instructions:
>> 1. you will see a button named "Test"
>> 2. press the button
>> 3. verify that the button can be pressed""",
>> 400, 250
>> );
>> }
>>
>> public static void main(String[] args) throws Exception {
>> launch(args);
>> }
>>
>> @Override
>> protected Node createContent() {
>> return new Button("Test");
>> }
>> }
>>
>>
>> Resulting application window:
>>
>> 
>>
>> Readme:
>>
>> https://github.com/andy-goryachev-oracle/jfx/blob/8319555.manual/tests/manual/util/README.md
>>
>> @prrace 's test EmojiTest has been converted to use the new test window as a
>> demonstration (also fixed the Eclipse project so it works now).
>>
>> Q: What other features can be added to the test window?
>>
>> Edit: the sources are left in their original place at the root of the
>> project.
>
> Andy Goryachev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> removed example
Only minor comment about unused code. Otherwise looks fine to me.
tests/manual/util/src/com/oracle/util/testing/ManualTestWindow.java line 130:
> 128: });
> 129: screenshotButton.setDisable(true);
> 130: */
I will suggest to delete unused code.
tests/manual/util/src/com/oracle/util/testing/ManualTestWindow.java line 148:
> 146: HBox buttons = new HBox(
> 147: 10,
> 148: //screenshotButton,
Same here.
-
PR Review: https://git.openjdk.org/jfx/pull/1413#pullrequestreview-1992844835
PR Review Comment: https://git.openjdk.org/jfx/pull/1413#discussion_r1560162005
PR Review Comment: https://git.openjdk.org/jfx/pull/1413#discussion_r1560162155
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v4]
On Mon, 8 Apr 2024 22:53:26 GMT, Andy Goryachev wrote: >> tests/manual/util/README.md line 30: >> >>> 28: ``` >>> 29: >>> 30: Resulting application window: >> >> Resulting application window does not look like it is from example above. >> Not sure if it is important. > > Not really important. > > Apart from being resized to decrease the size of the PNG image, it looks > exactly the same. Are you running it on a different platform? How does it > look, anything appears missing (can you post a screenshot)? If you run `EmojiTest`, then yes it will look correct, but if you run `SampleManualTest` it will not. Since you added this image to `SampleManualTest` as resulting image of `SampleManualTest`. Look at [ReadMe.md](https://github.com/openjdk/jfx/blob/a106587a69859966e668233a9d89539c3438fed7/tests/manual/util/README.md). - PR Review Comment: https://git.openjdk.org/jfx/pull/1413#discussion_r1556551412
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v4]
On Mon, 8 Apr 2024 22:25:53 GMT, Andy Goryachev wrote:
>> tests/manual/text/EmojiTest.java line 34:
>>
>>> 32: import com.oracle.util.testing.ManualTestWindow;
>>> 33:
>>> 34: public class EmojiTest {
>>
>> Our existing manual tests all extend `Application`. Is there a reason this
>> has to change? Absent a compelling reason, I'd prefer to not require test
>> apps to stop extending Application in order to use this utility.
>
> Everything, including creating an instance of Application, is taken care of
> by the `ManualTestWindow`. This simplifies the test code greatly.
>
> What is the reason that the test must extend `Application`?
For example if test needs to access `getParameters()` or `getHostServices ()`
from `Application` how it can be done? I think it might be better to extend
`ManualTestWindow` from `Application` and require test itself to extend
`ManualTestWindow`. In this case tests will extend `Application`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1413#discussion_r1556506989
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v4]
On Mon, 8 Apr 2024 15:04:24 GMT, Andy Goryachev wrote:
>> ## ManualTestWindow
>>
>> This facility provides a framework for manual tests to display test
>> instructions, test pane, and Pass/Fail buttons.
>>
>> A simple test would look like this:
>>
>>
>> public class SampleManualTest {
>> public static void main(String[] args) throws Exception {
>> ManualTestWindow.builder().
>> title("Sample Manual Test").
>> instructions(
>> """
>> Provide
>> multi-line instructions here.
>> """
>> ).
>> ui(() -> createTestUI()).
>> buildAndRun();
>> }
>>
>> private static Node createTestUI() {
>> return new Label("Test UI");
>> }
>> }
>>
>>
>> Resulting application window:
>>
>> 
>>
>> Readme:
>>
>> https://github.com/openjdk/jfx/blob/1cc095049be3773e1211ad570eb2285f08f25cec/tests/manual/util/README.md
>>
>> @prrace 's test EmojiTest has been converted to use the new test window as a
>> demonstration (also fixed the Eclipse project so it works now).
>>
>> Q: What other features can be added to the test window?
>>
>> Edit: the sources are left in their original place at the root of the
>> project.
>
> Andy Goryachev has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - works
> - .
tests/manual/util/README.md line 19:
> 17: multi-line instructions here.
> 18: """
> 19: ).
Add `size(1200, 800).` to example. I think adjusting size will be common enough
and no need to look at `ManualTestWindow` to figure it out.
tests/manual/util/README.md line 30:
> 28: ```
> 29:
> 30: Resulting application window:
Resulting application window does not look like it is from example above. Not
sure if it is important.
tests/manual/util/src/com/oracle/util/testing/ManualTestWindow.java line 89:
> 87: // TODO log area
> 88: // TODO screenshots on failure?
> 89: // TODO initial position?
In general I do not like TODO in comments. You can file follow up issue to
improve/extend `ManualTestWindow` if needed.
tests/manual/util/src/com/oracle/util/testing/ManualTestWindow.java line 250:
> 248: Scene scene = new Scene(vb);
> 249: stage.setWidth(width == 0 ? 800 : width);
> 250: stage.setHeight(height == 0 ? 600 : height);
I think `width` and `height` is `double` and not `int`. Also, lets not change
it. If test wants 0, then pass 0.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1413#discussion_r1556487869
PR Review Comment: https://git.openjdk.org/jfx/pull/1413#discussion_r1556488908
PR Review Comment: https://git.openjdk.org/jfx/pull/1413#discussion_r1556491820
PR Review Comment: https://git.openjdk.org/jfx/pull/1413#discussion_r1556500553
RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming
- Added support for #EXT-X-MEDIA tag to HTTP Live Streaming. - Following audio renditions via #EXT-X-MEDIA tag will be supported (see CSR for more details): - MP2T streams with one H.264/AVC video track and elementary AAC audio stream via #EXT-X-MEDIA tag. - fMP4 streams with one H.264/AVC or H.265/HEVC video track and elementary AAC audio stream via #EXT-X-MEDIA tag. - fMP4 streams with one H.264/AVC or H.265/HEVC video track and fMP4 streams with one AAC audio track via #EXT-X-MEDIA tag. - Separate audio stream will be playback via separate chain of GStreamer elements inside one pipeline. Which means two "javasource" elements will be used inside one pipeline and they will be reading data independently of each other via two separate HLSConnectionHolders. GStreamer will handle audio and video synchronization based on PTS as for other streams. Other solutions were considered such as one "javasource" with multiple source pads, but such implementation will be more complex and does not provide any benefits. - HLSConnectionHolder which handles video stream will also parse all information for separate audio stream and then create child HLSConnectionHolder for separate audio stream which will be responsible for downloading audio segments and seek of audio streams. - Parser in HLSConnectionHolder was reworked to make it more readable and easy to maintain and extend. - JavaDoc updated to point to latest HLS implementation vs old draft. It also updated with information on #EXT-X-MEDIA tag. Also, added missing information on AAC elementary streams and fMP4 from previous fixes. - Fixed and improved debug output in Linux AV plugins. - Added new property to "dshowwrapper" to disable PTS reset for each new segment, since with #EXT-X-MEDIA tag audio and video segments are not align and they can start at different time. - Fixed missing PTS on first buffer after seek in MP2T demuxer in "dshowwrapper". Without it audio and video synchronization breaks with two separate streams. - Removed dead code from MediaManager. - Added handling for GST_MESSAGE_LATENCY. Based on GStreamer doc we need to call gst_bin_recalculate_latency() when such message received. Not sure if we really need to do this, but with separate video and audio streams we do receive this message when seek is done. Most likely due to video and audio is not align perfectly when we seek. For other streams this message is not received in most cases. - Commit messages: - 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming Changes: https://git.openjdk.org/jfx/pull/1435/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1435&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8282999 Stats: 2344 lines in 30 files changed: 1265 ins; 408 del; 671 mod Patch: https://git.openjdk.org/jfx/pull/1435.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1435/head:pull/1435 PR: https://git.openjdk.org/jfx/pull/1435
Re: [jfx17u] RFR: 8318387: Update GStreamer to 1.22.6 [v2]
On Thu, 7 Mar 2024 14:13:18 GMT, Johan Vos wrote: >> Almost clean backport of JDK-8318386. >> The original patch contains a fix to gstaacparse.c which is not in jfx17u >> because it does not have JDK-8277309 (JDK-8277309) > > Johan Vos has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains two additional commits since > the last revision: > > - Merge branch 'openjdk:master' into backport-johanvos-606878af > - Backport 606878af275dbad551a10653c92a2deef61c10cd Looks good. - Marked as reviewed by almatvee (Reviewer). PR Review: https://git.openjdk.org/jfx17u/pull/181#pullrequestreview-1923519946
Integrated: 8308955: MediaPlayer/AudioClip skip data on seek/loop
On Tue, 23 Jan 2024 03:13:11 GMT, Alexander Matveev wrote: > This is regression from JDK-8262365. JDK-8262365 introduced support for > hardware pause for audio device. For some reason we will skip ~500 ms of > audio data after such pause. It is not noticeable for large audio files, but > for anything small like sound effects 1-3 seconds long it is noticeable and > makes short audio effects unusable without re-creating MediaPlayer after each > playback. Fix/workaround is to disable hardware pause. I did not figure out > why hardware pause skips audio data. This pull request has now been integrated. Changeset: 359ffb97 Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/359ffb97931e19c6e2a8b9a35f6fee3b44639bdc Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod 8308955: MediaPlayer/AudioClip skip data on seek/loop Reviewed-by: kcr, arapte - PR: https://git.openjdk.org/jfx/pull/1346
RFR: 8308955: MediaPlayer/AudioClip skip data on seek/loop
This is regression from JDK-8262365. JDK-8262365 introduced support for hardware pause for audio device. For some reason we will skip ~500 ms of audio data after such pause. It is not noticeable for large audio files, but for anything small like sound effects 1-3 seconds long it is noticeable and makes short audio effects unusable without re-creating MediaPlayer after each playback. Fix/workaround is to disable hardware pause. I did not figure out why hardware pause skips audio data. - Commit messages: - 8308955: MediaPlayer/AudioClip skip data on seek/loop Changes: https://git.openjdk.org/jfx/pull/1346/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1346&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8308955 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1346.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1346/head:pull/1346 PR: https://git.openjdk.org/jfx/pull/1346
[jfx21u] Integrated: 8318387: Update GStreamer to 1.22.6
On Thu, 23 Nov 2023 01:06:16 GMT, Alexander Matveev wrote: > Clean backport of JDK-8318387. This pull request has now been integrated. Changeset: a06b5ded Author: Alexander Matveev URL: https://git.openjdk.org/jfx21u/commit/a06b5dedf6fdb1c97afc0cfce3afb9600d9155bd Stats: 45668 lines in 438 files changed: 19431 ins; 18936 del; 7301 mod 8318387: Update GStreamer to 1.22.6 8318386: Update Glib to 2.78.1 Backport-of: 606878af275dbad551a10653c92a2deef61c10cd - PR: https://git.openjdk.org/jfx21u/pull/33
[jfx21u] RFR: 8318387: Update GStreamer to 1.22.6
Clean backport of JDK-8318387. - Commit messages: - Backport 606878af275dbad551a10653c92a2deef61c10cd Changes: https://git.openjdk.org/jfx21u/pull/33/files Webrev: https://webrevs.openjdk.org/?repo=jfx21u&pr=33&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8318387 Stats: 45668 lines in 438 files changed: 19431 ins; 18936 del; 7301 mod Patch: https://git.openjdk.org/jfx21u/pull/33.diff Fetch: git fetch https://git.openjdk.org/jfx21u.git pull/33/head:pull/33 PR: https://git.openjdk.org/jfx21u/pull/33
Integrated: 8318387: Update GStreamer to 1.22.6
On Fri, 17 Nov 2023 21:27:04 GMT, Alexander Matveev wrote: > - GStreamer updated to 1.22.6 and GLib updated to 2.78.1. > - Tested on all platforms with all supported media streams. > - GStreamer 1.22.6 requires GLib 2.62.0, but since we need to support older > GLib versions on Linux following APIs were changed from new to old one > (restored GStreamer 1.20.1 code) (Linux only, other platforms using latest > code): > - g_queue_clear_full() -> g_queue_foreach(), g_queue_clear(). > - g_atomic_rc_box_*() -> g_weak_ref_init(), g_weak_ref_clear(). This pull request has now been integrated. Changeset: 606878af Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/606878af275dbad551a10653c92a2deef61c10cd Stats: 45668 lines in 438 files changed: 19431 ins; 18936 del; 7301 mod 8318387: Update GStreamer to 1.22.6 8318386: Update Glib to 2.78.1 Reviewed-by: sykora, kcr - PR: https://git.openjdk.org/jfx/pull/1290
Re: RFR: 8318387: Update GStreamer to 1.22.6
On Fri, 17 Nov 2023 21:27:04 GMT, Alexander Matveev wrote: > - GStreamer updated to 1.22.6 and GLib updated to 2.78.1. > - Tested on all platforms with all supported media streams. > - GStreamer 1.22.6 requires GLib 2.62.0, but since we need to support older > GLib versions on Linux following APIs were changed from new to old one > (restored GStreamer 1.20.1 code) (Linux only, other platforms using latest > code): > - g_queue_clear_full() -> g_queue_foreach(), g_queue_clear(). > - g_atomic_rc_box_*() -> g_weak_ref_init(), g_weak_ref_clear(). 8318387: Update GStreamer to 1.22.6 [v3] - Fixed GLib version. It should be 2.78.1 instead of 2.78.0. - PR Comment: https://git.openjdk.org/jfx/pull/1290#issuecomment-1820046932
Re: RFR: 8318387: Update GStreamer to 1.22.6 [v2]
> - GStreamer updated to 1.22.6 and GLib updated to 2.78.1. > - Tested on all platforms with all supported media streams. > - GStreamer 1.22.6 requires GLib 2.62.0, but since we need to support older > GLib versions on Linux following APIs were changed from new to old one > (restored GStreamer 1.20.1 code) (Linux only, other platforms using latest > code): > - g_queue_clear_full() -> g_queue_foreach(), g_queue_clear(). > - g_atomic_rc_box_*() -> g_weak_ref_init(), g_weak_ref_clear(). Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8318387: Update GStreamer to 1.22.6 [v3] - Changes: - all: https://git.openjdk.org/jfx/pull/1290/files - new: https://git.openjdk.org/jfx/pull/1290/files/70bd5aa6..3ac4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1290&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1290&range=00-01 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1290.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1290/head:pull/1290 PR: https://git.openjdk.org/jfx/pull/1290
RFR: 8318387: Update GStreamer to 1.22.6
- GStreamer updated to 1.22.6 and GLib updated to 2.78.1. - Tested on all platforms with all supported media streams. - GStreamer 1.22.6 requires GLib 2.62.0, but since we need to support older GLib versions on Linux following APIs were changed from new to old one (restored GStreamer 1.20.1 code) (Linux only, other platforms using latest code): - g_queue_clear_full() -> g_queue_foreach(), g_queue_clear(). - g_atomic_rc_box_*() -> g_weak_ref_init(), g_weak_ref_clear(). - Commit messages: - 8318387: Update GStreamer to 1.22.6 [v2] - 8318387: Update GStreamer to 1.22.6 Changes: https://git.openjdk.org/jfx/pull/1290/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1290&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8318387 Stats: 45668 lines in 438 files changed: 19431 ins; 18936 del; 7301 mod Patch: https://git.openjdk.org/jfx/pull/1290.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1290/head:pull/1290 PR: https://git.openjdk.org/jfx/pull/1290
[jfx21u] Integrated: 8317508: Provide media support for libavcodec version 60
On Fri, 20 Oct 2023 21:29:03 GMT, Alexander Matveev wrote: > Clean backport of JDK-8317508. This pull request has now been integrated. Changeset: 5958b044 Author: Alexander Matveev URL: https://git.openjdk.org/jfx21u/commit/5958b044064127c92e0776a741f4341931e3ac23 Stats: 12 lines in 4 files changed: 7 ins; 0 del; 5 mod 8317508: Provide media support for libavcodec version 60 Backport-of: de456dad193f295a8bd93b72bb9b71960e6afd18 - PR: https://git.openjdk.org/jfx21u/pull/21
[jfx21u] RFR: 8317508: Provide media support for libavcodec version 60
Clean backport of JDK-8317508. - Commit messages: - Backport de456dad193f295a8bd93b72bb9b71960e6afd18 Changes: https://git.openjdk.org/jfx21u/pull/21/files Webrev: https://webrevs.openjdk.org/?repo=jfx21u&pr=21&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8317508 Stats: 12 lines in 4 files changed: 7 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jfx21u/pull/21.diff Fetch: git fetch https://git.openjdk.org/jfx21u.git pull/21/head:pull/21 PR: https://git.openjdk.org/jfx21u/pull/21
Integrated: 8317508: Provide media support for libavcodec version 60
On Fri, 13 Oct 2023 01:24:36 GMT, Alexander Matveev wrote: > - Added support for libavcodec version 60. > - Tested on Ubuntu 23.10 with all possible media formats. > - Changed "--disable-yasm" to "--disable-asm". "--disable-asm" disables all > assembler optimization and "--disable-yasm" only x86. Without "--disable-asm" > I had issue building ffmpeg 6.0 on Ubuntu 23.10 most likely due to compiler > version. Disabling assembler optimization fixed issue and we do not need this > anyway for our stubs. This pull request has now been integrated. Changeset: de456dad Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/de456dad193f295a8bd93b72bb9b71960e6afd18 Stats: 12 lines in 4 files changed: 7 ins; 0 del; 5 mod 8317508: Provide media support for libavcodec version 60 Reviewed-by: sykora, arapte - PR: https://git.openjdk.org/jfx/pull/1259
RFR: 8317508: Provide media support for libavcodec version 60
- Added support for libavcodec version 60. - Tested on Ubuntu 23.10 with all possible media formats. - Changed "--disable-yasm" to "--disable-asm". "--disable-asm" disables all assembler optimization and "--disable-yasm" only x86. Without "--disable-asm" I had issue building ffmpeg 6.0 on Ubuntu 23.10 most likely due to compiler version. Disabling assembler optimization fixed issue and we do not need this anyway for our stubs. - Commit messages: - 8317508: Provide media support for libavcodec version 60 Changes: https://git.openjdk.org/jfx/pull/1259/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1259&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8317508 Stats: 12 lines in 4 files changed: 7 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jfx/pull/1259.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1259/head:pull/1259 PR: https://git.openjdk.org/jfx/pull/1259
[jfx21u] Integrated: 8313900: Possible NULL pointer access in NativeAudioSpectrum and NativeVideoBuffer
On Tue, 8 Aug 2023 22:04:08 GMT, Alexander Matveev wrote: > This is clean backport of JDK-8313900. This pull request has now been integrated. Changeset: 27b179a3 Author: Alexander Matveev URL: https://git.openjdk.org/jfx21u/commit/27b179a38df9eff614155e5e1b8c298910622f53 Stats: 13 lines in 2 files changed: 11 ins; 0 del; 2 mod 8313900: Possible NULL pointer access in NativeAudioSpectrum and NativeVideoBuffer Backport-of: 9f180e20adc5bc8e6892d9672a414e8b7f614a20 - PR: https://git.openjdk.org/jfx21u/pull/7
[jfx21u] RFR: 8313900: Possible NULL pointer access in NativeAudioSpectrum and NativeVideoBuffer
This is clean backport of JDK-8313900. - Commit messages: - Backport 9f180e20adc5bc8e6892d9672a414e8b7f614a20 Changes: https://git.openjdk.org/jfx21u/pull/7/files Webrev: https://webrevs.openjdk.org/?repo=jfx21u&pr=7&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8313900 Stats: 13 lines in 2 files changed: 11 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx21u/pull/7.diff Fetch: git fetch https://git.openjdk.org/jfx21u.git pull/7/head:pull/7 PR: https://git.openjdk.org/jfx21u/pull/7
Integrated: 8313900: Possible NULL pointer access in NativeAudioSpectrum and NativeVideoBuffer
On Mon, 7 Aug 2023 23:33:37 GMT, Alexander Matveev wrote: > - Fixed by checking for `NULL` pointer after memory allocation. > - In `NativeVideoBuffer` `std::nothrow` was added when allocating `jint` > array, so `new` will return `NULL` instead of throwing exception. This done > for consistency and also it is not clear how well JNI handles C++ exceptions > in this case and what value will Java code get if exception is thrown. This pull request has now been integrated. Changeset: 9f180e20 Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/9f180e20adc5bc8e6892d9672a414e8b7f614a20 Stats: 13 lines in 2 files changed: 11 ins; 0 del; 2 mod 8313900: Possible NULL pointer access in NativeAudioSpectrum and NativeVideoBuffer Reviewed-by: kcr, arapte - PR: https://git.openjdk.org/jfx/pull/1204
RFR: 8313900: Possible NULL pointer access in NativeAudioSpectrum and NativeVideoBuffer
- Fixed by checking for `NULL` pointer after memory allocation. - In `NativeVideoBuffer` `std::nothrow` was added when allocating `jint` array, so `new` will return `NULL` instead of throwing exception. This done for consistency and also it is not clear how well JNI handles C++ exceptions in this case and what value will Java code get if exception is thrown. - Commit messages: - 8313900: Possible NULL pointer access in NativeAudioSpectrum and NativeVideoBuffer Changes: https://git.openjdk.org/jfx/pull/1204/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1204&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8313900 Stats: 13 lines in 2 files changed: 11 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1204.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1204/head:pull/1204 PR: https://git.openjdk.org/jfx/pull/1204
Integrated: 8304290: Some JNI calls made without checking exceptions in media
On Tue, 18 Apr 2023 02:08:03 GMT, Alexander Matveev wrote: > - Added missing exception checks for JNI calls. > - Improved JNI error checking by checking for both exception and return > value. > - Minor code clean up. This pull request has now been integrated. Changeset: 77c43e0d Author: Alexander Matveev URL: https://git.openjdk.org/jfx/commit/77c43e0d7209da756a6b10bb29c88bd22f199f26 Stats: 291 lines in 15 files changed: 77 ins; 96 del; 118 mod 8304290: Some JNI calls made without checking exceptions in media Reviewed-by: arapte, kcr - PR: https://git.openjdk.org/jfx/pull/1094
Re: RFR: 8304290: Some JNI calls made without checking exceptions in media [v2]
On Wed, 31 May 2023 22:36:54 GMT, Alexander Matveev wrote: >> - Added missing exception checks for JNI calls. >> - Improved JNI error checking by checking for both exception and return >> value. >> - Minor code clean up. > > Alexander Matveev has updated the pull request incrementally with one > additional commit since the last revision: > > 8304290: Some JNI calls made without checking exceptions in media [v2] 8304290: Some JNI calls made without checking exceptions in media [v3] - Fixed latest comments. - PR Comment: https://git.openjdk.org/jfx/pull/1094#issuecomment-1579589967
Re: RFR: 8304290: Some JNI calls made without checking exceptions in media [v3]
> - Added missing exception checks for JNI calls. > - Improved JNI error checking by checking for both exception and return > value. > - Minor code clean up. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8304290: Some JNI calls made without checking exceptions in media [v3] - Changes: - all: https://git.openjdk.org/jfx/pull/1094/files - new: https://git.openjdk.org/jfx/pull/1094/files/582daf7e..aad51ea7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1094&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1094&range=01-02 Stats: 10 lines in 2 files changed: 1 ins; 8 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1094.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1094/head:pull/1094 PR: https://git.openjdk.org/jfx/pull/1094
Re: RFR: 8304290: Some JNI calls made without checking exceptions in media [v2]
On Mon, 5 Jun 2023 21:04:15 GMT, Kevin Rushforth wrote: >> Alexander Matveev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8304290: Some JNI calls made without checking exceptions in media [v2] > > modules/javafx.media/src/main/native/jfxmedia/jni/JniUtils.cpp line 59: > >> 57: } >> 58: // This shouldn't happen... >> 59: return; > > Don't you still need this `return`? I moved it under `if (env->ExceptionCheck() || klass != NULL)`, but this check is incorrect and it should be `if (env->ExceptionCheck() || klass == NULL)`. I fixed it. - PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1220523650
Re: RFR: 8304290: Some JNI calls made without checking exceptions in media [v2]
On Mon, 5 Jun 2023 20:55:30 GMT, Kevin Rushforth wrote: >> Alexander Matveev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8304290: Some JNI calls made without checking exceptions in media [v2] > > modules/javafx.media/src/main/native/jfxmedia/jni/JavaPlayerEventDispatcher.cpp > line 199: > >> 197: } >> 198: >> 199: pEnv->DeleteLocalRef(jmessage); > > This needs to be moved inside the previous `if` block. Fixed. - PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1220513572
Re: RFR: 8304290: Some JNI calls made without checking exceptions in media [v2]
> - Added missing exception checks for JNI calls. > - Improved JNI error checking by checking for both exception and return > value. > - Minor code clean up. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8304290: Some JNI calls made without checking exceptions in media [v2] - Changes: - all: https://git.openjdk.org/jfx/pull/1094/files - new: https://git.openjdk.org/jfx/pull/1094/files/0f8bd616..582daf7e Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1094&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1094&range=00-01 Stats: 14 lines in 3 files changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jfx/pull/1094.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1094/head:pull/1094 PR: https://git.openjdk.org/jfx/pull/1094
Re: RFR: 8304290: Some JNI calls made without checking exceptions in media
On Tue, 18 Apr 2023 02:08:03 GMT, Alexander Matveev wrote: > - Added missing exception checks for JNI calls. > - Improved JNI error checking by checking for both exception and return > value. > - Minor code clean up. 8304290: Some JNI calls made without checking exceptions in media [v2] - Fixed latest comments. - PR Comment: https://git.openjdk.org/jfx/pull/1094#issuecomment-1571048630
Re: RFR: 8304290: Some JNI calls made without checking exceptions in media
On Tue, 9 May 2023 09:05:57 GMT, Ambarish Rapte wrote:
>> - Added missing exception checks for JNI calls.
>> - Improved JNI error checking by checking for both exception and return
>> value.
>> - Minor code clean up.
>
> modules/javafx.media/src/main/native/jfxmedia/Locator/Locator.cpp line 66:
>
>> 64: mid_toString = env->GetMethodID(klass, "getStringLocation",
>> "()Ljava/lang/String;");
>> 65: env->DeleteLocalRef(klass);
>> 66: if (javaEnv.clearException() || mid_toString == NULL)
>
> The variable name can be changed. It holds id to
> `Locator.getStringLocation()`, so the variable name does not reflect it
> correctly. As the PR is touching this code, it seems ok to change.
Fixed.
> modules/javafx.media/src/main/native/jfxmedia/jni/JavaInputStreamCallbacks.cpp
> line 68:
>
>> 66: createConnectionHolder = env->GetMethodID(klass,
>> "createConnectionHolder",
>> "()Lcom/sun/media/jfxmedia/locator/ConnectionHolder;");
>> 67: env->DeleteLocalRef(klass);
>> 68: if (javaEnv.reportException() || createConnectionHolder == 0)
>
> I would recommend to add as `(createConnectionHolder == NULL)`, similar to
> the below changes.
> And may be change previous instance and declaration of
> `createConnectionHolder`.
Fixed.
> modules/javafx.media/src/main/native/jfxmedia/jni/JavaInputStreamCallbacks.cpp
> line 73:
>
>> 71:
>> 72: jobject connectionHolder = env->CallObjectMethod(jLocator,
>> createConnectionHolder);
>> 73: if (javaEnv.reportException() || NULL == connectionHolder)
>
> May be enclose in parenthesis, like the below changes : `(NULL ==
> connectionHolder)`
Fixed.
> modules/javafx.media/src/main/native/jfxmedia/jni/JavaInputStreamCallbacks.cpp
> line 244:
>
>> 242: }
>> 243:
>> 244: javaEnv.reportException();
>
> modules/javafx.media/src/main/native/jfxmedia/jni/JavaInputStreamCallbacks.cpp
> : 240
>
> The `reportException()` method clears the exception and **prints** the
> exception message. So this change would result in behavior change. Is this
> change safe ?
> Similar change of removing `reportException()` can be done in `NeedBuffer()`
> method also.
Yes, it should be safe. I do not see any need to print exception, since it is
unlikely will happen. Also, these methods called many times to read data and
`clearException()` is more light weight. I updated `NeedBuffer()` to use
`clearException()`.
> modules/javafx.media/src/main/native/jfxmedia/jni/JavaMediaWarningListener.cpp
> line 49:
>
>> 47: jclass mediaUtilsClass =
>> pEnv->FindClass("com/sun/media/jfxmediaimpl/MediaUtils");
>> 48: if (!javaEnv.clearException() && mediaUtilsClass != NULL) {
>> 49: jmethodID errorMethodID =
>> pEnv->GetStaticMethodID(mediaUtilsClass,
>
> `errorMethodID` -> `nativeWarningMethodID`
Fixed.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212352448
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212354698
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212354937
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212365506
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212367015
Integrated: 8306328: Update libFFI to 3.4.4
On Wed, 10 May 2023 02:11:19 GMT, Alexander Matveev wrote: > - libFFI updated to 3.4.4. > - No additional changes are done to the code. > - Tested on Linux, Windows x64 and macOS x64/aarch64. > - Windows x86 config files are updated, but no build/testing was done for > 32-bit. This pull request has now been integrated. Changeset: 8110f548 Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/8110f548fc561dc39e15deb7ac0cececa8b6 Stats: 520 lines in 21 files changed: 267 ins; 41 del; 212 mod 8306328: Update libFFI to 3.4.4 Reviewed-by: jvos, arapte - PR: https://git.openjdk.org/jfx/pull/1131
Re: RFR: 8306328: Update libFFI to 3.4.4 [v2]
On Thu, 11 May 2023 01:09:53 GMT, Alexander Matveev wrote: >> - libFFI updated to 3.4.4. >> - No additional changes are done to the code. >> - Tested on Linux, Windows x64 and macOS x64/aarch64. >> - Windows x86 config files are updated, but no build/testing was done for >> 32-bit. > > Alexander Matveev has updated the pull request incrementally with one > additional commit since the last revision: > > 8306328: Update libFFI to 3.4.4 [v3] No, this code is used on macOS and Windows. - PR Comment: https://git.openjdk.org/jfx/pull/1131#issuecomment-1544722133
Re: RFR: 8306328: Update libFFI to 3.4.4
On Wed, 10 May 2023 02:11:19 GMT, Alexander Matveev wrote: > - libFFI updated to 3.4.4. > - No additional changes are done to the code. > - Tested on Linux, Windows x64 and macOS x64/aarch64. > - Windows x86 config files are updated, but no build/testing was done for > 32-bit. 8306328: Update libFFI to 3.4.4 [v3] - Updated libffi.md. - PR Comment: https://git.openjdk.org/jfx/pull/1131#issuecomment-1543012858
Re: RFR: 8306328: Update libFFI to 3.4.4 [v2]
> - libFFI updated to 3.4.4. > - No additional changes are done to the code. > - Tested on Linux, Windows x64 and macOS x64/aarch64. > - Windows x86 config files are updated, but no build/testing was done for > 32-bit. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8306328: Update libFFI to 3.4.4 [v3] - Changes: - all: https://git.openjdk.org/jfx/pull/1131/files - new: https://git.openjdk.org/jfx/pull/1131/files/34bc8000..fed6d204 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1131&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1131&range=00-01 Stats: 45 lines in 1 file changed: 45 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1131.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1131/head:pull/1131 PR: https://git.openjdk.org/jfx/pull/1131
RFR: 8306328: Update libFFI to 3.4.4
- libFFI updated to 3.4.4. - No additional changes are done to the code. - Tested on Linux, Windows x64 and macOS x64/aarch64. - Windows x86 config files are updated, but no build/testing was done for 32-bit. - Commit messages: - 8306328: Update libFFI to 3.4.4 [v2] - 8306328: Update libFFI to 3.4.4 Changes: https://git.openjdk.org/jfx/pull/1131/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1131&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8306328 Stats: 475 lines in 21 files changed: 222 ins; 41 del; 212 mod Patch: https://git.openjdk.org/jfx/pull/1131.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1131/head:pull/1131 PR: https://git.openjdk.org/jfx/pull/1131
Re: RFR: 8306328: Update libFFI to 3.4.4
On Wed, 10 May 2023 02:11:19 GMT, Alexander Matveev wrote: > - libFFI updated to 3.4.4. > - No additional changes are done to the code. > - Tested on Linux, Windows x64 and macOS x64/aarch64. > - Windows x86 config files are updated, but no build/testing was done for > 32-bit. 8306328: Update libFFI to 3.4.4 [v2] - Fixed whitespace issues. - PR Comment: https://git.openjdk.org/jfx/pull/1131#issuecomment-1542885053
Re: RFR: 8304290: Some JNI calls made without checking exceptions in media
On Mon, 24 Apr 2023 05:53:34 GMT, Michael Strauß wrote: >> - Added missing exception checks for JNI calls. >> - Improved JNI error checking by checking for both exception and return >> value. >> - Minor code clean up. > > modules/javafx.media/src/main/native/jfxmedia/jni/Logger.cpp line 130: > >> 128: // Get global reference >> 129: m_cls = (jclass)pEnv->NewWeakGlobalRef(local_cls); >> 130: pEnv->DeleteLocalRef(local_cls); > > What is this code attempting to do? After `local_cls` is deleted, `m_cls` > refers to a potentially freed object and is thus not safe to use. In order to > do anything meaningful with `m_cls`, a strong reference needs to be acquired > with `NewLocalRef(m_cls)`. I think it should be fine to use `NewWeakGlobalRef`. `NewLocalRef` will make reference which is valid only for duration of `CLogger::init` and we using `m_cls` in `CLogger::logMsg` as well. I think you mean `NewGlobalRef`, but in this case there is no way to call `DeleteGlobalRef`, since CLogger instance is static global variable which gets deleted only when native media library gets unloaded and by this time JVM will be shutting down. Java Logger class referenced by m_cls should not be freed, since it contains only static methods and fields, until our native code unloads. So, using weak reference should be fine in this case. - PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1184395328
RFR: 8304290: Some JNI calls made without checking exceptions in media
- Added missing exception checks for JNI calls. - Improved JNI error checking by checking for both exception and return value. - Minor code clean up. - Commit messages: - 8304290: Some JNI calls made without checking exceptions in media Changes: https://git.openjdk.org/jfx/pull/1094/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1094&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8304290 Stats: 278 lines in 15 files changed: 80 ins; 92 del; 106 mod Patch: https://git.openjdk.org/jfx/pull/1094.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1094/head:pull/1094 PR: https://git.openjdk.org/jfx/pull/1094
Re: RFR: 8304924: [testbug] Skip failing tests on Linux
On Sat, 25 Mar 2023 15:50:21 GMT, Kevin Rushforth wrote: > This PR skips three headful tests on Linux until the issues with the tests > can be fixed. These tests consistently fail on our physical Ubuntu 22.04 test > machine. This is a step towards getting clean headful test runs. Each of the > skipped tests has an associated bug that needs to be fixed. Marked as reviewed by almatvee (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1068#pullrequestreview-1359530257
Re: RFR: 8282386: JavaFX media stubs rely on libav.org [v2]
On Thu, 5 Jan 2023 20:24:41 GMT, Johan Vos wrote: >> Retrieve libav sources from github. >> Fix JDK-8282386 > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > Use new naming convention when working with libav files downloaded via > github Looks good. - Marked as reviewed by almatvee (Reviewer). PR: https://git.openjdk.org/jfx/pull/989
Integrated: 8287822: [macos] Remove support of duplicated formats from macOS
On Tue, 11 Oct 2022 00:01:18 GMT, Alexander Matveev wrote: > - Added support for JAR and JRT protocol to AVFoundation platform. > - Removed H.264/MP3 and AAC support from GStreamer platform, which was > primary used to playback these formats for JAR and JRT protocols. > - Added ability to FXMediaPlayer sample to generate playlist for JAR and JRT > protocols for testing. See FXMedia.java for how to use it. > - H.265/HEVC via JAR/JRT protocols should work on macOS after this change. > Before it did not work because GStreamer platform did not support H.265/HEVC > and AVFoundation did not support JAR/JRT protocols. > - Minor code clean up. > > After this changes: > GSTPlatform: AIFF and WAV for all protocols. > AVFoundation: MP3, AAC, HLS, H.264 and H.265 for all protocols. > > This change is transparent for end user and does not affect list of supported > formats by JavaFX Media. This pull request has now been integrated. Changeset: fe81a27f Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/fe81a27f0dc9e90a51d42e525b074be32ec5c82b Stats: 3283 lines in 26 files changed: 455 ins; 2784 del; 44 mod 8287822: [macos] Remove support of duplicated formats from macOS Reviewed-by: angorya, kcr - PR: https://git.openjdk.org/jfx/pull/909
Integrated: 8297362: EOS might not be delivered by progressbuffer in some cases
On Wed, 23 Nov 2022 00:57:11 GMT, Alexander Matveev wrote: > This is regression from > [JDK-8043352](https://bugs.openjdk.org/browse/JDK-8043352). > [JDK-8043352](https://bugs.openjdk.org/browse/JDK-8043352) moved clearing > pending events in progress buffer when upstream sends EOS. We need to do this > for any other events except EOS. If we clear pending EOS, then it will not be > delivered and we will hang due to downstream waiting for data or EOS. We > still need to clear any other events such as new segment and not deliver it > once we receive EOS. > > Added "plugins" folder to macOS project. For some reason it was missing and > debugging plugins with xcode was not possible. > > Tested on all platforms with all formats over FILE and HTTP protocol. EOS was > delivered correctly in all cases. This pull request has now been integrated. Changeset: 9416874f Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/9416874f632d8d036a324e195fae58e6a831bfc5 Stats: 20 lines in 2 files changed: 16 ins; 0 del; 4 mod 8297362: EOS might not be delivered by progressbuffer in some cases Reviewed-by: kcr, arapte - PR: https://git.openjdk.org/jfx/pull/961
RFR: 8297362: EOS might not be delivered by progressbuffer in some cases
This is regression from [JDK-8043352](https://bugs.openjdk.org/browse/JDK-8043352). [JDK-8043352](https://bugs.openjdk.org/browse/JDK-8043352) moved clearing pending events in progress buffer when upstream sends EOS. We need to do this for any other events except EOS. If we clear pending EOS, then it will not be delivered and we will hang due to downstream waiting for data or EOS. We still need to clear any other events such as new segment and not deliver it once we receive EOS. Added "plugins" folder to macOS project. For some reason it was missing and debugging plugins with xcode was not possible. Tested on all platforms with all formats over FILE and HTTP protocol. EOS was delivered correctly in all cases. - Commit messages: - 8297362: EOS might not be delivered by progressbuffer in some cases Changes: https://git.openjdk.org/jfx/pull/961/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=961&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8297362 Stats: 20 lines in 2 files changed: 16 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/961.diff Fetch: git fetch https://git.openjdk.org/jfx pull/961/head:pull/961 PR: https://git.openjdk.org/jfx/pull/961
Integrated: 8294400: Provide media support for libavcodec version 59
On Tue, 25 Oct 2022 05:07:05 GMT, Alexander Matveev wrote: > - Added support for libavcodec 59. > - libavcodec 59 removed several deprecated APIs and avplugin code was > updated to use replacement APIs instead. > - No changes to avplugin when used with 58 or below. > - Note: Support for libavcodec 59 was added based on ffmpeg only. libav does > not have version 59. This pull request has now been integrated. Changeset: ac8382bd Author: Alexander Matveev URL: https://git.openjdk.org/jfx/commit/ac8382bd378c7b51ccd625af46bdf10d24176692 Stats: 140 lines in 10 files changed: 108 ins; 0 del; 32 mod 8294400: Provide media support for libavcodec version 59 Reviewed-by: kcr, jvos - PR: https://git.openjdk.org/jfx/pull/932
Re: RFR: 8294400: Provide media support for libavcodec version 59 [v2]
> - Added support for libavcodec 59. > - libavcodec 59 removed several deprecated APIs and avplugin code was > updated to use replacement APIs instead. > - No changes to avplugin when used with 58 or below. > - Note: Support for libavcodec 59 was added based on ffmpeg only. libav does > not have version 59. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8294400: Provide media support for libavcodec version 59 [v2] - Changes: - all: https://git.openjdk.org/jfx/pull/932/files - new: https://git.openjdk.org/jfx/pull/932/files/bb4cead0..86fd01fc Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=932&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=932&range=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/932.diff Fetch: git fetch https://git.openjdk.org/jfx pull/932/head:pull/932 PR: https://git.openjdk.org/jfx/pull/932
Re: RFR: 8294400: Provide media support for libavcodec version 59
On Tue, 25 Oct 2022 05:07:05 GMT, Alexander Matveev wrote: > - Added support for libavcodec 59. > - libavcodec 59 removed several deprecated APIs and avplugin code was > updated to use replacement APIs instead. > - No changes to avplugin when used with 58 or below. > - Note: Support for libavcodec 59 was added based on ffmpeg only. libav does > not have version 59. 8294400: Provide media support for libavcodec version 59 [v2] - Added `avplugin-ffmpeg-59` in the NativeMediaManager constructor. - PR: https://git.openjdk.org/jfx/pull/932
RFR: 8294400: Provide media support for libavcodec version 59
- Added support for libavcodec 59. - libavcodec 59 removed several deprecated APIs and avplugin code was updated to use replacement APIs instead. - No changes to avplugin when used with 58 or below. - Note: Support for libavcodec 59 was added based on ffmpeg only. libav does not have version 59. - Commit messages: - 8294400: Provide media support for libavcodec version 59 Changes: https://git.openjdk.org/jfx/pull/932/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=932&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8294400 Stats: 138 lines in 9 files changed: 107 ins; 0 del; 31 mod Patch: https://git.openjdk.org/jfx/pull/932.diff Fetch: git fetch https://git.openjdk.org/jfx pull/932/head:pull/932 PR: https://git.openjdk.org/jfx/pull/932
Re: RFR: 8287822: [macos] Remove support of duplicated formats from macOS [v3]
On Thu, 13 Oct 2022 22:24:06 GMT, Andy Goryachev wrote:
>> Alexander Matveev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8287822: [macos] Remove support of duplicated formats from macOS [v3]
>
> tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java line
> 118:
>
>> 116: path = fileSystem.getPath("fxmediaplayer", "media");
>> 117: } else {
>> 118: path = Path.of(uri);
>
> should we explicitly check if it is a file?
> if("file".equals(uri.getScheme()) { ...
I do not see a point of doing it, since it should be a file.
-
PR: https://git.openjdk.org/jfx/pull/909
Re: RFR: 8287822: [macos] Remove support of duplicated formats from macOS [v3]
On Thu, 13 Oct 2022 22:23:08 GMT, Andy Goryachev wrote: >> Related question: Can `uri` itself be null? > > so the logic seems to be either a "jar", or else it's a file, right? > is it possible to be something else (i.e. the resource is missing > altogether)? or some other condition? Yes, when we run jar file it will be jar and if we run unpacked it will be file. If resource is missing we will not enumerate files and it is acceptable condition. We should fallback to default playlist if no files present. - PR: https://git.openjdk.org/jfx/pull/909
Re: RFR: 8287822: [macos] Remove support of duplicated formats from macOS [v3]
> - Added support for JAR and JRT protocol to AVFoundation platform. > - Removed H.264/MP3 and AAC support from GStreamer platform, which was > primary used to playback these formats for JAR and JRT protocols. > - Added ability to FXMediaPlayer sample to generate playlist for JAR and JRT > protocols for testing. See FXMedia.java for how to use it. > - H.265/HEVC via JAR/JRT protocols should work on macOS after this change. > Before it did not work because GStreamer platform did not support H.265/HEVC > and AVFoundation did not support JAR/JRT protocols. > - Minor code clean up. > > After this changes: > GSTPlatform: AIFF and WAV for all protocols. > AVFoundation: MP3, AAC, HLS, H.264 and H.265 for all protocols. > > This change is transparent for end user and does not affect list of supported > formats by JavaFX Media. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8287822: [macos] Remove support of duplicated formats from macOS [v3] - Changes: - all: https://git.openjdk.org/jfx/pull/909/files - new: https://git.openjdk.org/jfx/pull/909/files/9b76b9e9..d4eaaa89 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=909&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=909&range=01-02 Stats: 22 lines in 1 file changed: 16 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/909.diff Fetch: git fetch https://git.openjdk.org/jfx pull/909/head:pull/909 PR: https://git.openjdk.org/jfx/pull/909
Re: RFR: 8287822: [macos] Remove support of duplicated formats from macOS [v3]
On Wed, 12 Oct 2022 22:57:51 GMT, Andy Goryachev wrote: >> Spelling fixed and javafx.base and javafx.graphics is removed. I do not >> think that we need to worry about javadoc, since we do not generate it for >> this app. > > This is just a suggestion, really, especially if it is in test code. > However, Eclipse IDE shows javadoc (something I routinely use, the javadoc > tab is always open): > > src="https://user-images.githubusercontent.com/107069028/195462859-1174e2ca-53cd-4f02-9ff3-bd989ccd2cdd.png";> Fixed. - PR: https://git.openjdk.org/jfx/pull/909
Re: RFR: 8287822: [macos] Remove support of duplicated formats from macOS [v2]
> - Added support for JAR and JRT protocol to AVFoundation platform. > - Removed H.264/MP3 and AAC support from GStreamer platform, which was > primary used to playback these formats for JAR and JRT protocols. > - Added ability to FXMediaPlayer sample to generate playlist for JAR and JRT > protocols for testing. See FXMedia.java for how to use it. > - H.265/HEVC via JAR/JRT protocols should work on macOS after this change. > Before it did not work because GStreamer platform did not support H.265/HEVC > and AVFoundation did not support JAR/JRT protocols. > - Minor code clean up. > > After this changes: > GSTPlatform: AIFF and WAV for all protocols. > AVFoundation: MP3, AAC, HLS, H.264 and H.265 for all protocols. > > This change is transparent for end user and does not affect list of supported > formats by JavaFX Media. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8287822: [macos] Remove support of duplicated formats from macOS [v2] - Changes: - all: https://git.openjdk.org/jfx/pull/909/files - new: https://git.openjdk.org/jfx/pull/909/files/3a0cc087..9b76b9e9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=909&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=909&range=00-01 Stats: 15 lines in 4 files changed: 4 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/909.diff Fetch: git fetch https://git.openjdk.org/jfx pull/909/head:pull/909 PR: https://git.openjdk.org/jfx/pull/909
Re: RFR: 8287822: [macos] Remove support of duplicated formats from macOS [v2]
On Tue, 11 Oct 2022 21:19:05 GMT, Kevin Rushforth wrote:
>> modules/javafx.media/src/main/native/jfxmedia/Locator/Locator.cpp line 73:
>>
>>> 71: if (javaEnv.clearException())
>>> 72: return NULL;
>>> 73:
>>
>> minor: would it make sense to wrap return in { }, line 67 and 72?
>
> A lot of our third-party native code doesn't (I would insist on the `{}` if
> this were Java code), so if this were a gstreamer file it would be better to
> follow their convention, but since this is entirely our code, it makes sense
> to enclose in `{}`.
Fixed.
>> tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java line
>> 66:
>>
>>> 64: * [Windows] jlink --output dist/FXMediaPlayer -p
>>> ../../../../build/jmods;dist --add-modules
>>> javafx.base,javafx.controls,javafx.media,javafx.graphics,FXMediaPlayer
>>> --launcher FXMediaPlayer=FXMediaPlayer/fxmediaplayer.FXMediaPlayer
>>> 65: * [Windows] dist\FXMediaPlayer\bin\FXMediaPlayer.bat
>>> 66: */
>>
>> minor: this javadoc will likely not render as expected. perhaps use
>> from line 50 to 61 or 65.
>> also, spelling "protcol" on line 50
>
> Also minor: you can shorten the command lines by removing `javafx.base` and
> `javafx.graphics` from the list of added modules, since `javafx.controls`
> transiently requires them.
Spelling fixed and javafx.base and javafx.graphics is removed. I do not think
that we need to worry about javadoc, since we do not generate it for this app.
>> tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java line
>> 131:
>>
>>> 129: });
>>> 130: } catch (URISyntaxException | IOException ex) {
>>> 131: System.out.println("Exception: " + ex);
>>
>> should System.out be removed?
>>
>> general question: do we want to use logging in places like this?
>
> Since this is a test app, I wouldn't bother with logging. We generally would
> use `System.err` rather than `System.out` for printing exceptions, but that's
> a minor point.
I change it to System.err.
-
PR: https://git.openjdk.org/jfx/pull/909
Re: RFR: 8287822: [macos] Remove support of duplicated formats from macOS [v2]
On Tue, 11 Oct 2022 15:37:15 GMT, Andy Goryachev wrote:
>> Alexander Matveev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8287822: [macos] Remove support of duplicated formats from macOS [v2]
>
> tests/manual/media/FXMediaPlayer/src/fxmediaplayer/FXMediaPlayer.java line
> 165:
>
>> 163:
>> 164: // Create Media, MediaPlayer, MediaView and ImageView
>> 165: System.out.println("FXMediaPlayer source: " + source);
>
> probably needs to be removed?
No, I done it on purpose to see which files we loading. Makes it easy to test a
lot of files via playlist.
-
PR: https://git.openjdk.org/jfx/pull/909
Re: RFR: 8287822: [macos] Remove support of duplicated formats from macOS [v2]
On Tue, 11 Oct 2022 21:07:34 GMT, Kevin Rushforth wrote: >> Alexander Matveev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8287822: [macos] Remove support of duplicated formats from macOS [v2] > > modules/javafx.media/src/main/java/com/sun/media/jfxmediaimpl/platform/gstreamer/GSTMedia.java > line 95: > >> 93: String contentType, >> 94: long sizeHint, >> 95: long[] nativeMediaHandle); > > This file contains whitespace-only changes. Maybe revert the file since it is > otherwise unchanged? Reverted. > modules/javafx.media/src/main/native/jfxmedia/platform/osx/avf/AVFMediaPlayer.mm > line 89: > >> 87: >> 88: // Max number of bytes we will provide per request >> 89: #define MAX_READ_SIZE 1048576 > > Optional suggestion: define this as `(1024 * 1024)` ? It's OK to leave it as > is if you prefer. Maybe add a comment in that case? Fixed. > modules/javafx.media/src/main/native/jfxmedia/platform/osx/avf/AVFMediaPlayer.mm > line 779: > >> 777: >> 778: // Do not provide more then MAX_READ_SIZE at one call, otherwise >> 779: // AVFoundation might fail if we providing too much data. > > Minor: "providing" --> "provide" Fixed. > tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java line > 141: > >> 139: } >> 140: >> 141: } > > Minor: missing newline at end of file. Fixed. - PR: https://git.openjdk.org/jfx/pull/909
Re: RFR: 8287822: [macos] Remove support of duplicated formats from macOS
On Tue, 11 Oct 2022 00:01:18 GMT, Alexander Matveev wrote: > - Added support for JAR and JRT protocol to AVFoundation platform. > - Removed H.264/MP3 and AAC support from GStreamer platform, which was > primary used to playback these formats for JAR and JRT protocols. > - Added ability to FXMediaPlayer sample to generate playlist for JAR and JRT > protocols for testing. See FXMedia.java for how to use it. > - H.265/HEVC via JAR/JRT protocols should work on macOS after this change. > Before it did not work because GStreamer platform did not support H.265/HEVC > and AVFoundation did not support JAR/JRT protocols. > - Minor code clean up. > > After this changes: > GSTPlatform: AIFF and WAV for all protocols. > AVFoundation: MP3, AAC, HLS, H.264 and H.265 for all protocols. > > This change is transparent for end user and does not affect list of supported > formats by JavaFX Media. I did full testing on all three platforms and tested all formats via all protocols. Mostly to make sure that changes for FXMediaPlayer works on all platforms. - PR: https://git.openjdk.org/jfx/pull/909
RFR: 8287822: [macos] Remove support of duplicated formats from macOS
- Added support for JAR and JRT protocol to AVFoundation platform. - Removed H.264/MP3 and AAC support from GStreamer platform, which was primary used to playback these formats for JAR and JRT protocols. - Added ability to FXMediaPlayer sample to generate playlist for JAR and JRT protocols for testing. See FXMedia.java for how to use it. - H.265/HEVC via JAR/JRT protocols should work on macOS after this change. Before it did not work because GStreamer platform did not support H.265/HEVC and AVFoundation did not support JAR/JRT protocols. - Minor code clean up. After this changes: GSTPlatform: AIFF and WAV for all protocols. AVFoundation: MP3, AAC, HLS, H.264 and H.265 for all protocols. This change is transparent for end user and does not affect list of supported formats by JavaFX Media. - Commit messages: - Merge remote-tracking branch 'upstream/master' into JDK-8287822 - 8287822: [macos] Remove support of duplicated formats from macOS Changes: https://git.openjdk.org/jfx/pull/909/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=909&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8287822 Stats: 3269 lines in 27 files changed: 436 ins; 2784 del; 49 mod Patch: https://git.openjdk.org/jfx/pull/909.diff Fetch: git fetch https://git.openjdk.org/jfx pull/909/head:pull/909 PR: https://git.openjdk.org/jfx/pull/909
Integrated: 8293971: Loading new Media from resources can sometimes fail when loading from FXML
On Sat, 24 Sep 2022 03:07:00 GMT, Alexander Matveev wrote: > There was two problems: > - uri.getPath() was returning null for jar URI, since it is not a standard > URI and thus we did not detect file type based on extension. > - Our signature detection for MP3 had a bug and did not detect MP3 > correctly. See comments in code. > > Fixed by adding function to extract file name from jar URI and also signature > detection was fixed for MP3. This pull request has now been integrated. Changeset: 35675c8d Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/35675c8d27d54a26059b182614e18152794dbcec Stats: 53 lines in 2 files changed: 42 ins; 0 del; 11 mod 8293971: Loading new Media from resources can sometimes fail when loading from FXML Reviewed-by: angorya, kcr - PR: https://git.openjdk.org/jfx/pull/902
Re: RFR: 8293971: Loading new Media from resources can sometimes fail when loading from FXML [v2]
On Mon, 26 Sep 2022 15:16:34 GMT, Andy Goryachev wrote:
>> Alexander Matveev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8293971: Loading new Media from resources can sometimes fail when loading
>> from FXML [v2]
>
> modules/javafx.media/src/main/java/com/sun/media/jfxmedia/locator/Locator.java
> line 425:
>
>> 423: stream.close();
>> 424: isConnected = true;
>> 425: contentType =
>> MediaUtils.filenameToContentType(uri); // We need to provide at least
>> something
>
> would it be possible to explain what ie being done instead? i.e. "try to
> determine the content type based on extension" or something like that?
Fixed.
> modules/javafx.media/src/main/java/com/sun/media/jfxmediaimpl/MediaUtils.java
> line 187:
>
>> 185: * Returns the content type given the file name.
>> 186: *
>> 187: * @param uri
>
> should the comment be changed since the argument is uri and not a file name?
Fixed.
> modules/javafx.media/src/main/java/com/sun/media/jfxmediaimpl/MediaUtils.java
> line 241:
>
>> 239:
>> 240: String scheme = uri.getScheme().toLowerCase();
>> 241: if (scheme.equals("jar")) {
>
> is it possible for the 'scheme' to be null?
>
> (may be "jar".equals(scheme) would work better here?)
No, it should not be null, since we have check for null before it. I did change
it, so it is same as rest of our code.
-
PR: https://git.openjdk.org/jfx/pull/902
Re: RFR: 8293971: Loading new Media from resources can sometimes fail when loading from FXML
On Sat, 24 Sep 2022 03:07:00 GMT, Alexander Matveev wrote: > There was two problems: > - uri.getPath() was returning null for jar URI, since it is not a standard > URI and thus we did not detect file type based on extension. > - Our signature detection for MP3 had a bug and did not detect MP3 > correctly. See comments in code. > > Fixed by adding function to extract file name from jar URI and also signature > detection was fixed for MP3. 8293971: Loading new Media from resources can sometimes fail when loading from FXML [v2] - Fixed comments from Andy. - PR: https://git.openjdk.org/jfx/pull/902
