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

8273096: Add support for H.265/HEVC to JavaFX Media #649

Closed
wants to merge 5 commits into from

Conversation

sashamatveev
Copy link
Member

@sashamatveev sashamatveev commented Oct 21, 2021

  • Added support for H.265/HEVC for all 3 platforms.
  • Support is added only for .mp4 files over FILE/HTTP/HTTPS protocols. HTTP Live Streaming with H.265/HEVC is not supported.
  • On Windows mfwrapper was introduced which uses Media Foundation APIs to decode HEVC.
  • 10 and 12-bit HEVC was tested and also supported, however due to graphics pipeline not supporting 10-bit YUV rendering we will use color converter to convert video frame to 8-bit before sending it for rendering.
  • Resolution upto 4k is supported.

Additional runtime dependency requirements:
Windows: Windows 10 with HEVC Video Extensions installed.
macOS: macOS High Sierra and later
Linux: at least libavcodec56 and libswscale5

Additional build dependency:
Linux: libswscale-dev


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed
  • Change requires a CSR request to be approved

Issues

  • JDK-8273096: Add support for H.265/HEVC to JavaFX Media
  • JDK-8276975: Add support for H.265/HEVC to JavaFX Media (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/649/head:pull/649
$ git checkout pull/649

Update a local copy of the PR:
$ git checkout pull/649
$ git pull https://git.openjdk.java.net/jfx pull/649/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 649

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/649.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 21, 2021

👋 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.

@sashamatveev sashamatveev changed the title JDK-8273096: Add support for H.265/HEVC to JavaFX Media 8273096: Add support for H.265/HEVC to JavaFX Media Oct 21, 2021
@openjdk openjdk bot added the rfr label Oct 21, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 21, 2021

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 21, 2021

/reviewers 2

@kevinrushforth kevinrushforth self-requested a review Oct 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 22, 2021

Mailing list message from Dan Howard on openjfx-dev:

Very cool.

On 10/21/2021 5:49 PM, Alexander Matveev wrote:

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 3, 2021

This adds an additional supported encoding type, so we will need a CSR for this. The specification can be the changes to the docs in javafx/scene/media/package.html

/csr

@openjdk openjdk bot added the csr label Nov 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2021

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@sashamatveev please create a CSR request for issue JDK-8273096. This pull request cannot be integrated until the CSR request is approved.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 3, 2021

Sorry for the delay in looking at this.

I get this failure on Ubuntu 20.04 Linux running the oow2010-2.mp4 sample media file:

** (java:72365): WARNING **: 07:18:02.597: Failed to load plugin '.../build/sdk/lib/libavplugin-ffmpeg-58.so':
    libswscale.so.5: cannot open shared object file: No such file or directory

Regarding the additional runtime dependencies:

Additional runtime dependency requirements:
Windows: Windows 10 with HEVC Video Extensions installed.
macOS: macOS High Sierra and later
Linux: at least libavcodec56 and libswscale5

The added dependency on Linux on libswscale5 is a problem, at least as implemented in the current version of the PR. It's OK to require additional dependencies at runtime in order to play H.265 media files, but we need to continue to be able to play media that doesn't use H.265 without any additional requirements. This means that the loading of libswscale5.so needs to be optional; we can't link with it at build time, but need to dynamically load it at runtime, and only use it, or fail if not present, when decoding media formats that require it.

Similarly, it is fine to make the playing of H.265 videos dependent on macOS 10.13 High Sierra or later, but unless we bump our current minimum platform across the board, we need to be able to play videos that don't use H.265 on macOS 10.12 Sierra.

The same thing applies to Windows, where we need to be able to play non-H.265 media without the HEVC Video Extensions.

@sashamatveev
Copy link
Member Author

@sashamatveev sashamatveev commented Nov 16, 2021

Added new patch. libswscale will be loaded dynamically on Linux for H.265/HEVC 10/12-bit. Playback will not fail for other formats if it is not present. Windows should play other formats without "HEVC Video Extensions" without any issues. Same for macOS. Following MediaException will be thrown on Linux if libswscale is not present. Output is from FXMediaPlayer.
onError: MediaException: PLAYBACK_ERROR : [com.sun.media.jfxmediaimpl.platform.gstreamer.GSTMediaPlayer@a88d9ab] ERROR_MISSING_LIBSWSCALE: ERROR_MISSING_LIBSWSCALE

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 16, 2021

The recent changes for Linux look good to me. And thanks for confirming that it already works as expected for Windows and macOS.

I'll put this back on my queue to review and test.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 24, 2021

All my testing looks good. I confirm that the earlier reported problem on Linux is fixed.

I am part way through the code review and so far haven't spotted any issues.

@openjdk openjdk bot removed the csr label Nov 29, 2021
@kevinrushforth kevinrushforth self-requested a review Dec 13, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Changes look fine with one question.

@kevinrushforth kevinrushforth self-requested a review Dec 14, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Latest code changes look good with one question (in two places).

@kevinrushforth kevinrushforth requested review from johanvos and tiainen Dec 17, 2021
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 17, 2021

@johanvos @tiainen Can one of you be the second reviewer?

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Dec 17, 2021

We're building it now, and will do additional tests once building works fine (we might need a small change in the CI due to the additional Linux dependency)

Copy link
Collaborator

@johanvos johanvos left a comment

Works as expected on Linux/mac

@openjdk
Copy link

@openjdk openjdk bot commented Dec 19, 2021

@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:

8273096: Add support for H.265/HEVC to JavaFX Media

Reviewed-by: kcr, jvos

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 48 new commits pushed to the master branch:

  • 18063ad: 8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes
  • 5422a5a: 8278860: Streamline properties for Monocle
  • 4c5bf44: 8278905: JavaFX: EnumConverter has a typo in the toString method
  • 002d4f5: 8278134: Move static utility methods to infrastructure (EditAndScrollTest)
  • 1ebf7a2: 8278260: JavaFX shared libraries not stripped on Linux or macOS
  • 27f2810: 8273089: Deprecate JavaFX GTK 2 library for removal
  • 1ad11b0: 8278595: Provide more information when a pipeline can't be used
  • 1158339: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element
  • ff6e8d5: 8274274: Update JUnit to version 5.8.1
  • 4f9b047: 8201538: Remove implementation support for applets from JavaFX
  • ... and 38 more: https://git.openjdk.java.net/jfx/compare/7be0abb9aa5e86e953b13d4ad1d88000e652029c...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.

@openjdk openjdk bot added the ready label Dec 19, 2021
@sashamatveev
Copy link
Member Author

@sashamatveev sashamatveev commented Dec 20, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Dec 20, 2021

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

  • 18063ad: 8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes
  • 5422a5a: 8278860: Streamline properties for Monocle
  • 4c5bf44: 8278905: JavaFX: EnumConverter has a typo in the toString method
  • 002d4f5: 8278134: Move static utility methods to infrastructure (EditAndScrollTest)
  • 1ebf7a2: 8278260: JavaFX shared libraries not stripped on Linux or macOS
  • 27f2810: 8273089: Deprecate JavaFX GTK 2 library for removal
  • 1ad11b0: 8278595: Provide more information when a pipeline can't be used
  • 1158339: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element
  • ff6e8d5: 8274274: Update JUnit to version 5.8.1
  • 4f9b047: 8201538: Remove implementation support for applets from JavaFX
  • ... and 38 more: https://git.openjdk.java.net/jfx/compare/7be0abb9aa5e86e953b13d4ad1d88000e652029c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Dec 20, 2021
@openjdk openjdk bot closed this Dec 20, 2021
@openjdk openjdk bot removed ready rfr labels Dec 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 20, 2021

@sashamatveev Pushed as commit 1f10c63.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated
3 participants