8287822: [macos] Remove support of duplicated formats from macOS #909

sashamatveev · 2022-10-11T00:01:18Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8287822: [macos] Remove support of duplicated formats from macOS

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Committer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/909/head:pull/909
$ git checkout pull/909

Update a local copy of the PR:
$ git checkout pull/909
$ git pull https://git.openjdk.org/jfx pull/909/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 909

View PR using the GUI difftool:
$ git pr show -t 909

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/909.diff

bridgekeeper · 2022-10-11T00:02:03Z

👋 Welcome back almatvee! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2022-10-11T00:08:25Z

Webrevs

kevinrushforth · 2022-10-11T14:26:41Z

/reviewers 2

openjdk · 2022-10-11T14:26:59Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

tests/manual/media/FXMediaPlayer/src/fxmediaplayer/FXMediaPlayer.java

tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java

andy-goryachev-oracle · 2022-10-11T15:41:36Z

tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java

+                isJRT = false;
+                uri = FXMedia.class.getResource("/fxmediaplayer/media").toURI();
+
+                if (uri.getScheme().equals("jar")) {


is it possible for uri scheme to be null?
may be "jar".equals(uri.getScheme()) ?

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.

tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java

modules/javafx.media/src/main/native/jfxmedia/Locator/Locator.cpp

kevinrushforth

I left a few comments. I'll want to do some quick testing, but don't have time today.

I presume you have carefully tested this on macOS. Have you done some minimal testing on Windows and Linux, too (since there are platform-independent changes)?

modules/javafx.media/src/main/java/com/sun/media/jfxmediaimpl/platform/gstreamer/GSTMedia.java

modules/javafx.media/src/main/native/jfxmedia/Locator/Locator.cpp

modules/javafx.media/src/main/native/jfxmedia/platform/osx/avf/AVFMediaPlayer.mm

tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java

kevinrushforth · 2022-10-11T21:41:55Z

tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java

+                isJRT = false;
+                uri = FXMedia.class.getResource("/fxmediaplayer/media").toURI();
+
+                if (uri.getScheme().equals("jar")) {


Related question: Can uri itself be null?

tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java

sashamatveev · 2022-10-12T22:39:26Z

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.

andy-goryachev-oracle · 2022-10-13T22:23:08Z

tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java

+                isJRT = false;
+                uri = FXMedia.class.getResource("/fxmediaplayer/media").toURI();
+
+                if (uri.getScheme().equals("jar")) {


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?

andy-goryachev-oracle · 2022-10-13T22:24:06Z

tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java

+                            Collections.<String, Object>emptyMap());
+                    path = fileSystem.getPath("fxmediaplayer", "media");
+                } else {
+                    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.

kevinrushforth

Finally getting back to this. The current version looks good to me.

andy-goryachev-oracle

all my comments got explained and/or resolved. thank you!

openjdk · 2022-11-30T18:58:13Z

@sashamatveev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8287822: [macos] Remove support of duplicated formats from macOS

Reviewed-by: angorya, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 35 new commits pushed to the master branch:

0a785ae: 8295754: PaginationSkin: memory leak when changing skin
2fa9f4b: 8297414: Remove easy warnings in javafx.controls
7cb408b: 8297412: Remove easy warnings in javafx.fxml, javafx.media, javafx.swing, javafx.swt and javafx.web
fbad15d: 8295796: ScrollPaneSkin: memory leak when changing skin
4ad8582: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
d040c1f: 8297680: JavaDoc example for PseudoClass has minor typo
3376228: 8294809: ListenerHelper for managing and disconnecting listeners
67088be: 8256397: MultipleSelectionModel throws IndexOutOfBoundException
4a697f1: 8297130: ComboBox popup doesn't close after selecting value that was added with 'runLater'
cce8580: 8254676: Alert disables Tab selection when TabDragPolicy REORDER is used
... and 25 more: https://git.openjdk.org/jfx/compare/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

sashamatveev · 2022-12-03T00:15:47Z

/integrate

openjdk · 2022-12-03T00:17:50Z

Going to push as commit fe81a27.
Since your change was applied there have been 39 commits pushed to the master branch:

9416874: 8297362: EOS might not be delivered by progressbuffer in some cases
e64e129: 8295500: AccordionSkin: memory leak when changing skin
6ab65a9: 8295506: ButtonBarSkin: memory leak when changing skin
4a19fe7: 8295426: MenuButtonSkin: memory leak when changing skin
0a785ae: 8295754: PaginationSkin: memory leak when changing skin
2fa9f4b: 8297414: Remove easy warnings in javafx.controls
7cb408b: 8297412: Remove easy warnings in javafx.fxml, javafx.media, javafx.swing, javafx.swt and javafx.web
fbad15d: 8295796: ScrollPaneSkin: memory leak when changing skin
4ad8582: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
d040c1f: 8297680: JavaDoc example for PseudoClass has minor typo
... and 29 more: https://git.openjdk.org/jfx/compare/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-03T00:18:08Z

@sashamatveev Pushed as commit fe81a27.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

sashamatveev added 2 commits October 10, 2022 16:24

8287822: [macos] Remove support of duplicated formats from macOS

4afdd98

Merge remote-tracking branch 'upstream/master' into JDK-8287822

3a0cc08

openjdk bot added the rfr Ready for review label Oct 11, 2022

kevinrushforth self-requested a review October 11, 2022 14:26

andy-goryachev-oracle suggested changes Oct 11, 2022

View reviewed changes

kevinrushforth reviewed Oct 11, 2022

View reviewed changes

8287822: [macos] Remove support of duplicated formats from macOS [v2]

9b76b9e

8287822: [macos] Remove support of duplicated formats from macOS [v3]

d4eaaa8

andy-goryachev-oracle suggested changes Oct 13, 2022

View reviewed changes

kevinrushforth self-requested a review October 26, 2022 18:18

kevinrushforth approved these changes Nov 29, 2022

View reviewed changes

andy-goryachev-oracle approved these changes Nov 30, 2022

View reviewed changes

openjdk bot added the ready Ready to be integrated label Nov 30, 2022

openjdk bot added the integrated Pull request has been integrated label Dec 3, 2022

openjdk bot closed this Dec 3, 2022

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Dec 3, 2022

8287822: [macos] Remove support of duplicated formats from macOS #909

8287822: [macos] Remove support of duplicated formats from macOS #909

sashamatveev commented Oct 11, 2022 •

edited by openjdk bot

bridgekeeper bot commented Oct 11, 2022

mlbridge bot commented Oct 11, 2022 •

edited

kevinrushforth commented Oct 11, 2022

openjdk bot commented Oct 11, 2022

andy-goryachev-oracle Oct 11, 2022

kevinrushforth Oct 11, 2022

andy-goryachev-oracle Oct 13, 2022

sashamatveev Oct 13, 2022

kevinrushforth left a comment

kevinrushforth Oct 11, 2022

sashamatveev commented Oct 12, 2022

andy-goryachev-oracle Oct 13, 2022

andy-goryachev-oracle Oct 13, 2022

sashamatveev Oct 13, 2022

kevinrushforth left a comment

andy-goryachev-oracle left a comment

openjdk bot commented Nov 30, 2022

sashamatveev commented Dec 3, 2022

openjdk bot commented Dec 3, 2022

openjdk bot commented Dec 3, 2022

8287822: [macos] Remove support of duplicated formats from macOS #909

8287822: [macos] Remove support of duplicated formats from macOS #909

Conversation

sashamatveev commented Oct 11, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 11, 2022

mlbridge bot commented Oct 11, 2022 • edited

Webrevs

kevinrushforth commented Oct 11, 2022

openjdk bot commented Oct 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sashamatveev commented Oct 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 30, 2022

sashamatveev commented Dec 3, 2022

openjdk bot commented Dec 3, 2022

openjdk bot commented Dec 3, 2022

sashamatveev commented Oct 11, 2022 •

edited by openjdk bot

mlbridge bot commented Oct 11, 2022 •

edited