Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8238755: allow to create static lib for javafx.media on linux #109

Closed
wants to merge 7 commits into from

Conversation

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Feb 10, 2020

  • add support for the "resource" protocol (which is used in the GraalVM URLs pointing to statically bundled resources)
  • avoid duplicate symbols with different gst plugins
  • statically register gst plugins

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8238755: allow to create static lib for javafx.media on linux

Reviewers

  • Kevin Rushforth (kcr - Reviewer)
  • Alexander Matveev (almatvee - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/109/head:pull/109
$ git checkout pull/109

* add support for the "resource" protocol (which is used in the GraalVM URLs pointing to statically bundled resources)
* avoid duplicate symbols with different gst plugins
* statically register gst plugins
@johanvos johanvos requested a review from kevinrushforth Feb 10, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 10, 2020

👋 Welcome back jvos! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Feb 10, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 10, 2020

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 10, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Feb 10, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 10, 2020

I'll review this and also ask @sashamatveev to review.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

I added a comment about using an ifdef for GSTREAMER_LITE in one of the modified files (the others are fine, since those files are entirely FX_specific).

One other question: do you need any changes in ConnectionHolder.java? That class checks for URL scheme to determine whether a URL is seekable.

@@ -62,7 +62,8 @@
"file",
"http",
"https",
"jrt"
"jrt",
"resource"

This comment has been minimized.

@sashamatveev

sashamatveev Feb 10, 2020
Member

We do not support "resource" protocol in non-static build, so it is better to only enable it for static build. Not sure how to do this.

This comment has been minimized.

@johanvos

johanvos Feb 11, 2020
Author Collaborator

As an alternative, the "iPod" strategy can be used.
For some reasons, the "ipod-library" protocol is supported in the code (Locator), and if that is found, most connection code is bypassed.
That strategy seems to be much more difficult to maintain though.

This comment has been minimized.

@sashamatveev

sashamatveev Feb 11, 2020
Member

"ipod-library" is only supported if IOSPlatform is loaded, but it will not be loaded on all platform due to check for HostUtils.isIOS(). Do you know what happens if user tries "resource" protocol on not supported platform? and same for some "unknown" protocol. If error message makes sense we can probably keep it as is.

This comment has been minimized.

@johanvos

johanvos Feb 12, 2020
Author Collaborator

That is not 100% correct. ipod-library is supported in case System.getProperty("os.name") starts with "ios" (which we do in GraalVM and OpenJDK/mobile) (which does not guarantee at all that we're on ios).

We can encapsulate the "resource" protocol using the same approach (with a System.getProperty check). In that case, it might make sense to use a wide property that indicates we're running on a statically linked image. That would then be useful for all Java code, not just javafx.media. But that is a bigger change, so I'd like @kevinrushforth opinion on this.

This comment has been minimized.

@kevinrushforth

kevinrushforth Feb 13, 2020
Member

It might make sense to add an isStaticallyLinked method to PlatformUtil, which is where similar platform methods are kept.

@johanvos
Copy link
Collaborator Author

@johanvos johanvos commented Feb 11, 2020

As for the ConnectionHolder: the URIConnectionHolder will be used. This indeed will by default return false for isSeekable hence that functionality will now not be possible using the "resource" protocol.

EDIT: with 1c7a4eb this is now working as intended (support for seek has been added, since the lower layers support it)

@sashamatveev
Copy link
Member

@sashamatveev sashamatveev commented Feb 11, 2020

Does "resource" protocol allows random access? If yes, we should update isSeekable() to return true for resource protocol. seek() should also be updated. I think same should work as for "if ((urlConnection instanceof JarURLConnection) || isJRT())"

@johanvos
Copy link
Collaborator Author

@johanvos johanvos commented Feb 12, 2020

Does "resource" protocol allows random access? If yes, we should update isSeekable() to return true for resource protocol. seek() should also be updated. I think same should work as for "if ((urlConnection instanceof JarURLConnection) || isJRT())"

Good suggestion, committed.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good to me. I ran a full test on Linux with the default dynamic libraries and all looks good. I also tested that the resource protocol throws an informative exception:

MediaException: MEDIA_INACCESSIBLE : unknown protocol: resource

I asked one question, but whatever you decide is fine with me.

@@ -71,6 +71,7 @@
private static final boolean LINUX = os.startsWith("Linux") && !ANDROID;
private static final boolean SOLARIS = os.startsWith("SunOS");
private static final boolean IOS = os.startsWith("iOS");
private static final boolean STATIC_BUILD = "Substrate VM".equals(System.getProperty("java.vm.name"));

This comment has been minimized.

@kevinrushforth

kevinrushforth Feb 20, 2020
Member

Is it always going to be the case that STATIC_BUILD == is SubStrate VM?

This comment has been minimized.

@johanvos

johanvos Feb 21, 2020
Author Collaborator

That's a good question. At this moment, there is a 1-1 link between statically linked images and Substrate VM.
In case there will be more VM's that are statically linked with the applications and resources, I assume it will be beneficial to add a property in the "java." space to reflect this. There are more parts in OpenJDK that can benefit from different treatment in case of a statically linked image.

Copy link
Member

@sashamatveev sashamatveev left a comment

Looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 21, 2020

@johanvos This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8238755: allow to create static lib for javafx.media on linux

Reviewed-by: kcr, almatvee
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 9 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate ef2f9ce96e21ff5ec4f528fb8e5f191632506791.

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

@openjdk openjdk bot added the ready label Feb 21, 2020
@johanvos
Copy link
Collaborator Author

@johanvos johanvos commented Feb 25, 2020

/integrate

@openjdk openjdk bot closed this Feb 25, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 25, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Feb 25, 2020

@johanvos The following commits have been pushed to master since your change was applied:

  • c3ee1a3: 8239822: Intermittent unit test failures in RegionCSSTest
  • 3150562: 8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface
  • 4eaff0d: 8239109: Update SQLite to version 3.31.1
  • d8e7f85: 8239454: LLIntData : invalid opcode returned for 16 and 32 bit wide instructions
  • 48ddd80: Merge
  • 35a01ca: 8228867: Fix mistakes in FX API docs
  • b5e65ec: 8238434: Ensemble: Update version of Lucene to 7.7.2
  • e986459: 8227619: Potential memory leak in javafx.scene.control.ListView

Your commit was automatically rebased without conflicts.

Pushed as commit ef2f9ce.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 25, 2020

Mailing list message from Johan Vos on openjfx-dev:

Changeset: ef2f9ce
Author: Johan Vos <jvos at openjdk.org>
Date: 2020-02-25 22:02:00 +0000
URL: https://git.openjdk.java.net/jfx/commit/ef2f9ce9

8238755: allow to create static lib for javafx.media on linux

Reviewed-by: kcr, almatvee

! modules/javafx.base/src/main/java/com/sun/javafx/PlatformUtil.java
! modules/javafx.base/src/main/java/module-info.java
! modules/javafx.media/src/main/java/com/sun/media/jfxmedia/locator/ConnectionHolder.java
! modules/javafx.media/src/main/java/com/sun/media/jfxmedia/locator/Locator.java
! modules/javafx.media/src/main/java/com/sun/media/jfxmediaimpl/platform/PlatformManager.java
! modules/javafx.media/src/main/java/com/sun/media/jfxmediaimpl/platform/gstreamer/GSTPlatform.java
! modules/javafx.media/src/main/native/gstreamer/gstreamer-lite/gstreamer/gst/gst.c
! modules/javafx.media/src/main/native/gstreamer/plugins/av/fxavcodecplugin.c
! modules/javafx.media/src/main/native/gstreamer/plugins/fxplugins.c
! modules/javafx.media/src/main/native/jfxmedia/platform/gstreamer/GstPlatform.cpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.