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

8283218: Update GStreamer to 1.20.1 #768

Closed
wants to merge 5 commits into from

Conversation

sashamatveev
Copy link
Member

@sashamatveev sashamatveev commented Apr 8, 2022

  • GStreamer updated to 1.20.1 and GLib updated to 2.72.0.
  • No changes to our code, except in GstAudioSpectrum.cpp g_atomic_pointer_compare_and_exchange() was changed to g_atomic_pointer_set(). For some reason I was not able to get code compiled with g_atomic_pointer_compare_and_exchange() used from C++ code. Also, I do not see a need to use g_atomic_pointer_compare_and_exchange(), since m_pHolder always equals to old_holder.
  • Tested on all platforms with all supported media streams.

Progress

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

Issues

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 768

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 8, 2022

👋 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
Copy link
Member Author

/solves 8283403: Update Glib to 2.72.0

@openjdk openjdk bot added the rfr Ready for review label Apr 8, 2022
@openjdk
Copy link

openjdk bot commented Apr 8, 2022

@sashamatveev
Adding additional issue to solves list: 8283403: Update Glib to 2.72.0.

@mlbridge
Copy link

mlbridge bot commented Apr 8, 2022

Webrevs

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Apr 11, 2022

@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 1 of role reviewers, 1 of role authors).

@kevinrushforth
Copy link
Member

This still uses the system Glib library on Linux, right? What Linux platforms have you tried this on? It fails for me on my local Ubuntu 16.04 system.

In file included from ../../../gstreamer-lite/gstreamer/gst/gstbuffer.c:137:
../../../gstreamer-lite/gstreamer/gst/gstbuffer.c: In function 'gst_buffer_new_m
emdup':
../../../gstreamer-lite/gstreamer/gst/glib-compat-private.h:34:90: error: implicit declaration of function 'g_abort'; did you mean 'abort'? [-Werror=implicit-function-declaration]
   34 | #define g_memdup2(ptr,sz) ((G_LIKELY(((guint64)(sz)) < G_MAXUINT)) ? g_memdup(ptr,sz) : (g_abort(),NULL))
      |                                                                                          ^~~~~~~

We compile with -Werror=implicit-function-declaration, so this fails the build.

@kevinrushforth
Copy link
Member

The updated version gets past the earlier error, but still fails:

../../../gstreamer-lite/gstreamer/gst/gstelementfactory.c:494:28: error: implicit declaration of function 'g_object_new_with_properties'; did you mean 'g_object_class_list_properties'? [-Werror=implicit-function-declaration]
  494 |   element = (GstElement *) g_object_new_with_properties (factory->type, n,
      |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                            g_object_class_list_properties

@sashamatveev
Copy link
Member Author

sashamatveev commented Apr 16, 2022

Update GStreamer to 1.20.1 [v2]

  • Made caps writable before changing them in qtdemux, otherwise values are not set and critical error printed in console.
  • Fixed compilation error in GstAudioSpectrum.cpp by casting pointer.
  • Added g_abort() if it is not defined (building with older GLib).

Update GStreamer to 1.20.1 [v3]

  • Added -Werror=deprecated-declarations and GLIB_VERSION_MIN_REQUIRED/GLIB_VERSION_MAX_ALLOWED=2.48.0 to Linux makefiles. GLIB_VERSION_MIN_REQUIRED/GLIB_VERSION_MAX_ALLOWED will give deprecated warnings when code uses APIs from GLib which where added after 2.48.0. -Werror=deprecated-declarations will fail build if such warning detected. This is needed to make sure that we can build and run with older GLib starting with 2.48.0 and up.
  • avplugin does not have -Werror=deprecated-declarations, because avplugin uses deprecated APIs from libavcodec and build fails with this flag and fixing avplugin is out of scope of GStreamer update.
  • Fixed build issues that were discovered after above was implemented, so we can build/run with GLib 2.48.0 and up.

@kevinrushforth
Copy link
Member

I see one remaining build failure on Ubuntu 16.04, which should be easy to fix:

../../../gstreamer-lite/gstreamer/libs/gst/base/gstbytereader.h:367:3: error: implicit declaration of function 'memcpy' [-Werror=implicit-function-declaration]
  367 |   memcpy (dup_data, data, size);
      |   ^~~~~~
../../../gstreamer-lite/gstreamer/libs/gst/base/gstbytereader.h:367:3: warning: incompatible implicit declaration of built-in function 'memcpy'
../../../gstreamer-lite/gstreamer/libs/gst/base/gstbytereader.h:27:1: note: include '<string.h>' or provide a declaration of 'memcpy'
   26 | #include <gst/base/base-prelude.h>
  +++ |+#include <string.h>
   27 |
In file included from ../../../gstreamer-lite/gstreamer/libs/gst/base/gstbytewriter.h:25,
                 from ../../../gstreamer-lite/gstreamer/libs/gst/base/gstbytewriter.c:26:
../../../gstreamer-lite/gstreamer/libs/gst/base/gstbytereader.h: In function 'gst_byte_reader_dup_data_unchecked':
../../../gstreamer-lite/gstreamer/libs/gst/base/gstbytereader.h:367:3: error: implicit declaration of function 'memcpy' [-Werror=implicit-function-declaration]
  367 |   memcpy (dup_data, data, size);
      |   ^~~~~~
../../../gstreamer-lite/gstreamer/libs/gst/base/gstbytereader.h:367:3: warning: incompatible implicit declaration of built-in function 'memcpy'

The workaround you needed to do this time suggests we will need a different approach for the next update. I think we will need to consider one of two things:

  1. Build and link glib-lite on Linux, as we do for the other platforms
  2. Bump the minimum version of GLib needed to build or run JavaFX media. This would mean we would no longer run on Ubuntu 16.0.4 (and probably older versions of RHEL / Oracle Linux).

We can decide as it gets closer to the next update.

@sashamatveev
Copy link
Member Author

8283218: Update GStreamer to 1.20.1 [v4]

  • Added missing include file (string.h).

@sashamatveev
Copy link
Member Author

8283218: Update GStreamer to 1.20.1 [v5]

  • Added missing define which required for Windows 32-bit build.

@kevinrushforth
Copy link
Member

The latest patch builds and runs for me now on Ubuntu 16.04. I do want to make sure that regardless of whatever system we build it on, the binary is able to run on the oldest version we are targeting. Otherwise, a binary built on, say, Oracle Linux 7.x or Ubuntu 20.04 might not run on an Ubuntu 16.04 system. Basically what we don't want is to be dependent on a particular version of libraries at compile time.

The more I think about it, the best long term solution (not this time) is probably to build and deliver glib-lite.so on Linux.

@johanvos
Copy link
Collaborator

I have this working on Ubuntu 18.04, and that gives me enough confidence that with the appropriate toolchain it can be built for 16.04. Exploring glib-lite.so sounds like a good plan.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answering my own question from above, I was able to take a gstreamer-lite binary built on Oracle Linux 7.x and run it on my Ubuntu 16.04 system, so it looks like we are good to go.

Alexander or I will file a follow-on bug to build and ship glib-lite on Linux. It will allow some of the custom code to be reverted, and will avoid this problem the next time we update GLib. It also matches what we do for the other platforms (and what we do for Linux on Oracle's JDK 8u releases, so we have a data point that shows it to be a feasible approach).

@openjdk
Copy link

openjdk bot commented Apr 21, 2022

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

8283218: Update GStreamer to 1.20.1
8283403: Update Glib to 2.72.0

Reviewed-by: jvos, 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 17 new commits pushed to the master branch:

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 Ready to be integrated label Apr 21, 2022
@sashamatveev
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Apr 21, 2022

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

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 21, 2022
@openjdk openjdk bot closed this Apr 21, 2022
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Apr 21, 2022
@openjdk
Copy link

openjdk bot commented Apr 21, 2022

@sashamatveev Pushed as commit c4b1a72.

💡 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 Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

3 participants