8240694: [macos 10.15] JavaFX Media hangs on some video files on Cata…

…lina Reviewed-by: kcr, ghb
openjdk · Apr 9, 2020 · e1cb191 · e1cb191
1 parent c154538
commit e1cb191
Show file tree

Hide file tree

Showing 11 changed files with 54 additions and 33 deletions.
diff --git a/modules/javafx.media/src/main/java/com/sun/media/jfxmedia/events/AudioSpectrumEvent.java b/modules/javafx.media/src/main/java/com/sun/media/jfxmedia/events/AudioSpectrumEvent.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -31,22 +31,42 @@ public class AudioSpectrumEvent extends PlayerEvent {
     private AudioSpectrum source;
     private double        timestamp;
     private double        duration;
+    private boolean       queryTimestamp;
 
-    public AudioSpectrumEvent(AudioSpectrum source, double timestamp, double duration) {
+    /*
+     * Value of timestamp will be ignored if queryTimestamp is set true and
+     * timestamp will be requested from EventQueueThread when spectrum event is
+     * received instead. We do not use -1.0 (GST_CLOCK_TIME_NONE), since
+     * GStreamer might send us such events in case if something fails, so we using
+     * queryTimestamp to know for sure that we need to ask for timestamp from
+     * event queue. Note: Only OSX platfrom sets it true. GStreamer platfrom
+     * should not use it unless such usage is tested.
+     */
+    public AudioSpectrumEvent(AudioSpectrum source, double timestamp,
+                              double duration, boolean queryTimestamp) {
         this.source = source;
         this.timestamp = timestamp;
         this.duration = duration;
+        this.queryTimestamp = queryTimestamp;
     }
 
     public final AudioSpectrum getSource() {
         return source;
     }
 
+    public final void setTimestamp(double timestamp) {
+        this.timestamp = timestamp;
+    }
+
     public final double getTimestamp() {
         return timestamp;
     }
 
     public final double getDuration() {
         return duration;
     }
+
+    public final boolean queryTimestamp() {
+        return queryTimestamp;
+    }
 }
diff --git a/modules/javafx.media/src/main/java/com/sun/media/jfxmediaimpl/NativeMediaPlayer.java b/modules/javafx.media/src/main/java/com/sun/media/jfxmediaimpl/NativeMediaPlayer.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -719,6 +719,14 @@ private void HandleAudioSpectrumEvents(AudioSpectrumEvent evt) {
             for (ListIterator<WeakReference<AudioSpectrumListener>> it = audioSpectrumListeners.listIterator(); it.hasNext();) {
                 AudioSpectrumListener listener = it.next().get();
                 if (listener != null) {
+                    // OSXPlatfrom will set queryTimestamp to true, so we can request
+                    // time here from EventQueueThread, since requesting time from
+                    // audio processing thread might hang. See JDK-8240694.
+                    if (evt.queryTimestamp()) {
+                        double timestamp = playerGetPresentationTime();
+                        evt.setTimestamp(timestamp);
+                    }
+
                     listener.onAudioSpectrumEvent(evt);
                 } else {
                     it.remove();
@@ -1551,8 +1559,8 @@ protected void sendBufferProgressEvent(double clipDuration, long bufferStart, lo
         sendPlayerEvent(new BufferProgressEvent(clipDuration, bufferStart, bufferStop, bufferPosition));
     }
 
-    protected void sendAudioSpectrumEvent(double timestamp, double duration) {
-        sendPlayerEvent(new AudioSpectrumEvent(getAudioSpectrum(), timestamp, duration));
+    protected void sendAudioSpectrumEvent(double timestamp, double duration, boolean queryTimestamp) {
+        sendPlayerEvent(new AudioSpectrumEvent(getAudioSpectrum(), timestamp, duration, queryTimestamp));
     }
 
     @Override

diff --git a/modules/javafx.media/src/main/native/jfxmedia/PipelineManagement/PlayerEventDispatcher.h b/modules/javafx.media/src/main/native/jfxmedia/PipelineManagement/PlayerEventDispatcher.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -51,7 +51,7 @@ class CPlayerEventDispatcher
     virtual bool SendMarkerEvent(string name, double time) = 0;
     virtual bool SendBufferProgressEvent(double clipDuration, int64_t start, int64_t stop, int64_t position) = 0;
     virtual bool SendDurationUpdateEvent(double time) = 0;
-    virtual bool SendAudioSpectrumEvent(double time, double duration) = 0;
+    virtual bool SendAudioSpectrumEvent(double time, double duration, bool queryTimestamp) = 0;
     virtual void Warning(int warningCode, const char* warningMessage) = 0;
 };
 #endif // _PLAYER_EVENT_DISPATCHER_H_
diff --git a/modules/javafx.media/src/main/native/jfxmedia/jni/JavaPlayerEventDispatcher.cpp b/modules/javafx.media/src/main/native/jfxmedia/jni/JavaPlayerEventDispatcher.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -155,7 +155,7 @@ void CJavaPlayerEventDispatcher::Init(JNIEnv *env, jobject PlayerInstance, CMedi
 
         if (!hasException)
         {
-            m_SendAudioSpectrumEventMethod  = env->GetMethodID(klass, "sendAudioSpectrumEvent", "(DD)V");
+            m_SendAudioSpectrumEventMethod  = env->GetMethodID(klass, "sendAudioSpectrumEvent", "(DDZ)V");
             hasException = javaEnv.reportException();
         }
 
@@ -541,15 +541,17 @@ bool CJavaPlayerEventDispatcher::SendDurationUpdateEvent(double time)
     return bSucceeded;
 }
 
-bool CJavaPlayerEventDispatcher::SendAudioSpectrumEvent(double time, double duration)
+bool CJavaPlayerEventDispatcher::SendAudioSpectrumEvent(double time, double duration,
+                                                        bool queryTimestamp)
 {
     bool bSucceeded = false;
     CJavaEnvironment jenv(m_PlayerVM);
     JNIEnv *pEnv = jenv.getEnvironment();
     if (pEnv) {
         jobject localPlayer = pEnv->NewLocalRef(m_PlayerInstance);
         if (localPlayer) {
-            pEnv->CallVoidMethod(localPlayer, m_SendAudioSpectrumEventMethod, time, duration);
+            pEnv->CallVoidMethod(localPlayer, m_SendAudioSpectrumEventMethod, time,
+                                              duration, queryTimestamp);
             pEnv->DeleteLocalRef(localPlayer);
 
             bSucceeded = !jenv.reportException();

diff --git a/modules/javafx.media/src/main/native/jfxmedia/jni/JavaPlayerEventDispatcher.h b/modules/javafx.media/src/main/native/jfxmedia/jni/JavaPlayerEventDispatcher.h
@@ -59,7 +59,7 @@ class CJavaPlayerEventDispatcher : public CPlayerEventDispatcher
     virtual bool SendMarkerEvent(string name, double time);
     virtual bool SendBufferProgressEvent(double clipDuration, int64_t start, int64_t stop, int64_t position);
     virtual bool SendDurationUpdateEvent(double time);
-    virtual bool SendAudioSpectrumEvent(double time, double duration);
+    virtual bool SendAudioSpectrumEvent(double time, double duration, bool queryTimestamp);
     virtual void Warning(int warningCode, const char* warningMessage);
 
 private:

diff --git a/...les/javafx.media/src/main/native/jfxmedia/platform/gstreamer/GstAudioPlaybackPipeline.cpp b/...les/javafx.media/src/main/native/jfxmedia/platform/gstreamer/GstAudioPlaybackPipeline.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -1451,7 +1451,9 @@ gboolean CGstAudioPlaybackPipeline::BusCallback(GstBus* bus, GstMessage* msg, sB
                 }
 
                 if (!pPipeline->m_pEventDispatcher->SendAudioSpectrumEvent(GST_TIME_AS_SECONDS((double)timestamp),
-                    GST_TIME_AS_SECONDS((double)duration)))
+                    GST_TIME_AS_SECONDS((double)duration), false)) // Always false, since GStreamer does not need it,
+                                                                   // but if it will be required such case needs to be
+                                                                   // tested.
                 {
                     if(!pPipeline->m_pEventDispatcher->SendPlayerMediaErrorEvent(ERROR_JNI_SEND_AUDIO_SPECTRUM_EVENT))
                     {

diff --git a/modules/javafx.media/src/main/native/jfxmedia/platform/ios/EventDispatcher.m b/modules/javafx.media/src/main/native/jfxmedia/platform/ios/EventDispatcher.m
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -103,7 +103,7 @@ - (void) initMethodIDs: (JNIEnv *) env
     midSendAudioSpectrumEvent = (*env)->GetMethodID(env,
                                                     klass,
                                                     "sendAudioSpectrumEvent",
-                                                    "(DD)V");
+                                                    "(DDZ)V");
 
 }
 

diff --git a/modules/javafx.media/src/main/native/jfxmedia/platform/osx/avf/AVFAudioProcessor.mm b/modules/javafx.media/src/main/native/jfxmedia/platform/osx/avf/AVFAudioProcessor.mm
@@ -279,8 +279,4 @@ void ProcessAudioTap(MTAudioProcessingTapRef tapRef,
             return;
         }
     }
-
-    if (context->audioSpectrum != nullptr) {
-        context->audioSpectrum.get()->SetFirstBufferDelivered(true);
-    }
 }
diff --git a/modules/javafx.media/src/main/native/jfxmedia/platform/osx/avf/AVFAudioSpectrumUnit.cpp b/modules/javafx.media/src/main/native/jfxmedia/platform/osx/avf/AVFAudioSpectrumUnit.cpp
@@ -41,7 +41,6 @@ AVFAudioSpectrumUnit::AVFAudioSpectrumUnit() : mSpectrumCallbackProc(NULL),
                                                mMaxFrames(0),
                                                mSamplesPerInterval(0),
                                                mRebuildCrunch(true),
-                                               mFirstBufferDelivered(false),
                                                mSpectrumElement(NULL),
                                                mSpectrum(NULL) {
     mMixBuffer.mNumberBuffers = 1;
@@ -192,8 +191,10 @@ void AVFAudioSpectrumUnit::UpdateBands(int size, const float* magnitudes, const
     // Call our listener to dispatch the spectrum event
     if (mSpectrumCallbackProc) {
         double duration = (double) mSamplesPerInterval / (double) 44100;
-        double timestamp = mFirstBufferDelivered ? -1.0 : 0.0;
-        mSpectrumCallbackProc(mSpectrumCallbackContext, duration, timestamp);
+        // We do not provide timestamp here. It will be queried from EventQueueThread
+        // due to reading current time from AVPlayer might hang when called
+        // from audio processing thread. This function is called from this thread.
+        mSpectrumCallbackProc(mSpectrumCallbackContext, duration, -1.0);
     }
 
     unlockBands();
@@ -216,10 +217,6 @@ void AVFAudioSpectrumUnit::SetSpectrumCallbackProc(AVFSpectrumUnitCallbackProc p
     mSpectrumCallbackContext = context;
 }
 
-void AVFAudioSpectrumUnit::SetFirstBufferDelivered(bool isFirstBufferDelivered) {
-    mFirstBufferDelivered = isFirstBufferDelivered;
-}
-
 static gboolean PostMessageCallback(GstElement * element, GstMessage * message) {
     if (message == NULL) {
         return FALSE;

diff --git a/modules/javafx.media/src/main/native/jfxmedia/platform/osx/avf/AVFAudioSpectrumUnit.h b/modules/javafx.media/src/main/native/jfxmedia/platform/osx/avf/AVFAudioSpectrumUnit.h
@@ -81,7 +81,6 @@ class AVFAudioSpectrumUnit : public CAudioSpectrum {
     void SetChannels(UInt32 count);
     void SetMaxFrames(UInt32 maxFrames);
     void SetSpectrumCallbackProc(AVFSpectrumUnitCallbackProc proc, void *context);
-    void SetFirstBufferDelivered(bool isFirstBufferDelivered);
 
 private:
     AVFSpectrumUnitCallbackProc mSpectrumCallbackProc;
@@ -104,7 +103,6 @@ class AVFAudioSpectrumUnit : public CAudioSpectrum {
     UInt32 mSamplesPerInterval;
 
     bool mRebuildCrunch;
-    bool mFirstBufferDelivered;
 
     // GStreamer
     GstElement *mSpectrumElement;

diff --git a/modules/javafx.media/src/main/native/jfxmedia/platform/osx/avf/AVFMediaPlayer.mm b/modules/javafx.media/src/main/native/jfxmedia/platform/osx/avf/AVFMediaPlayer.mm
@@ -653,10 +653,8 @@ - (void) sendPixelBuffer:(CVPixelBufferRef)buf frameTime:(double)frameTime hostT
 
 - (void) sendSpectrumEventDuration:(double)duration timestamp:(double)timestamp {
     if (eventHandler) {
-        if (timestamp < 0) {
-            timestamp = self.currentTime;
-        }
-        eventHandler->SendAudioSpectrumEvent(timestamp, duration);
+        // Always true for queryTimestamp to avoid hang. See JDK-8240694.
+        eventHandler->SendAudioSpectrumEvent(timestamp, duration, true);
     }
 }