Wrong Thread errors
Closed this issue · 31 comments
I snagged a copy of vlcj-javafx. I'm running it with the JDK8 release (1.8.0-ea-b112) and I get some issues trying it out:
JNA: Callback uk.co.caprica.vlcj.player.direct.DefaultDirectMediaPlayer$SetupCallback@e93bcb7 threw the following exception:
java.lang.IllegalStateException: Not on FX application thread; currentThread = Thread-25
at com.sun.javafx.tk.Toolkit.checkFxUserThread(Toolkit.java:209)
at com.sun.javafx.tk.quantum.QuantumToolkit.checkFxUserThread(QuantumToolkit.java:393)
at javafx.scene.Scene.addToDirtyList(Scene.java:516)
at javafx.scene.Node.addToSceneDirtyList(Node.java:418)
at javafx.scene.Node.impl_markDirty(Node.java:409)
at javafx.scene.canvas.Canvas.access$000(Canvas.java:74)
at javafx.scene.canvas.Canvas$1.invalidated(Canvas.java:141)
at javafx.beans.property.DoublePropertyBase.markInvalid(DoublePropertyBase.java:112)
at javafx.beans.property.DoublePropertyBase.set(DoublePropertyBase.java:146)
at javafx.scene.canvas.Canvas.setWidth(Canvas.java:128)
at uk.co.caprica.vlcj.javafx.test.JavaFXDirectRenderingTest$TestBufferFormatCallback.getBufferFormat(JavaFXDirectRenderingTest.java:185)
OK, so, figured it out myself. You'll have to add a couple imports, but basically, the pixelWriter, canvas, and scene resizing has to be invoked to 'run later' so it runs on the right thread.
in the TestMediaPlayerComponent, you need to change display() like so:
@Override
public void display(DirectMediaPlayer mediaPlayer, Memory[] nativeBuffers, final BufferFormat bufferFormat) {
Memory nativeBuffer = nativeBuffers[0];
final ByteBuffer byteBuffer = nativeBuffer.getByteBuffer(0, nativeBuffer.size());
Platform.runLater(new Runnable () {
@Override
public void run() {
pixelWriter.setPixels(0, 0, bufferFormat.getWidth(), bufferFormat.getHeight(), pixelFormat, byteBuffer, bufferFormat.getWidth()*4);
}
});
}And then just below, in the TestBufferFormatCallback, you need to change the getBufferFormat like so:
@Override
public BufferFormat getBufferFormat(int sourceWidth, int sourceHeight) {
int width;
int height;
if (useSourceSize) {
width = sourceWidth;
height = sourceHeight;
}
else {
width = WIDTH;
height = HEIGHT;
}
final int actualwidth = width;
final int actualheight = height;
Platform.runLater(new Runnable() {
@Override
public void run() {
canvas.setWidth(actualwidth);
canvas.setHeight(actualheight);
stage.setWidth(actualwidth);
stage.setHeight(actualheight);
}
});
return new RV32BufferFormat(width, height);
}I'm a novice, so I'm sure this isn't 100% right, but it worked on JDK8 and performance seems ok.
Thanks to John Hendrikx for the breadcrumbs I needed on this one.
Unfortunately, I think that approach is flawed, but I am happy to be corrected if my understanding is wrong...
There are important synchronisation considerations here...
The byte buffer you get when you call nativeBuffer.getByteBuffer is according to the JNA documentation a "direct" buffer mapped to the native memory - it is not a copy of the native buffer, it is a view directly onto the native buffer.
You then schedule some code to "runLater", i.e. asynchronously.
Some short time later, perhaps before your asynchronous code runs, the native memory buffer will be unlocked ready for the next frame of video data.
So you are in your "runLater" code accessing a block of native memory that may be in the process of being overwritten by the native process.
The only safe place to access the native memory buffer is inside that callback function.
Alright. It was working reasonably well, but I admit I didn't stress test or see how it would perform over longer periods of playback.
I don't even know enough to understand your well explained concerns, nevermind address them (I'm guessing it's not as simple as moving the getByteBuffer down into the call?). If you figure out a JDK8 safe solution, please let me know. I'd much rather have things the right way.
You can't just move getByteBuffer down into the runLater, since the native buffer encapsulated by the Memory object will still be unlocked and overwritten with the next frame of video data when the display() callback method exits.
I'm surprised this works on JavaFX on JDK7 but not on JDK8 to be honest.
I can't see a simple solution, at least not without copying the native memory to a Java buffer in that callback, then invoking runLater to draw from that Java buffer. The problem with that approach is that vlc might start sending you new frames faster than your application can render them - so you end up with a whole bunch of runLater code blocks queued up.
I'm no JavaFX expert, but this is my understanding.
I have run into the same issue. Delegating the pixelwritter calls to the FX thread works but I think we generate bunch of overhead here, since for every frame a new Runnable has to be allocated and then executed.
Since JavaFX is event driven we probably have to abuse some of FX internal constructs to get somewhere. I.e. define a timeline with the desired FPS, and in the timeline currentTimeProperty event we update the pixels using pixelwritter.
How to solve the synchronisation is the big question (without copy the backbuffer). I try to implement it with a timeline and report back.
I have now an implementation which uses the javafx timeline to handle the interval by which the backbuffer is copyied to the canvas / pixelwriter:
MediaPlayerVLC.java
Most important methods:
Start the rendering (timeline animation) when the movie starts, using the movies fps:
On each keyframe, we delegate to updateFrame which basically copies the backbuffer using pixelwritter.
/**
* Render the back-buffer into the canvas with the given fps
* @param fps Update interval for the rendering
*/
private void renderMovie(final double fps){
System.out.println("MediaPlayerVLC: render movie @ " + fps + " fps");
if(t != null)
stopRender();
t = new Timeline();
t.setCycleCount(Timeline.INDEFINITE);
double duration = 1000 / fps;
t.getKeyFrames().add(
new KeyFrame(Duration.millis(duration),
onFinished));
t.playFromStart();
}
private final EventHandler<ActionEvent> onFinished = new EventHandler<ActionEvent>() {
@Override
public void handle(ActionEvent t) {
updateFrame();
}
};
/**
* Updates the current frame (copy back-buffer to the canvas)
*
* Note: This method must be called in the FX Application thread
*/
private final void updateFrame(){
// save the references (to avoid NPE race conditions)
final ByteBuffer currentBuffer = tmpbuffer;
final BufferFormat currentFormat = tmpformat;
if(currentBuffer != null && currentFormat != null)
{
pixelWriter.setPixels(0, 0,
currentFormat.getWidth(),
currentFormat.getHeight(),
pixelFormat, currentBuffer,
currentFormat.getWidth()*4);
}
}The backbuffer itself is simply updated by each render-callback:
/**
* Callback to get the buffer format to use for video playback.
*/
private final RenderCallback renderCallback = new RenderCallback() {
@Override
public void display(DirectMediaPlayer mediaPlayer,
Memory[] nativeBuffers, final BufferFormat bufferFormat) {
final Memory nativeBuffer = nativeBuffers[0];
final ByteBuffer byteBuffer = nativeBuffer.getByteBuffer(0, nativeBuffer.size());
// we just store the latest references
tmpbuffer = byteBuffer;
tmpformat = bufferFormat;
}
};This implementation does not guarantee that each frame is rendered nor does it synchronize the access of the bytebuffer, but i could not observe any issues so far. Instead of just saving the temporary instances one could get a snapshot from the bytebuffer.
Very interesting.
Seems like a lot of bother though doesn't it.
Interesting - I'll have to tinker with it, try it out.
It does seem like a fair bit of bother, but if one is deep into JavaFX and really in love with VLCJ (thank you!), it seems necessary to solve.
While my current implementation is not generic / clean enough, I hope we can refactor a base standalone "vlc(j)-Player" out of this code, which can be shipped with vlcj.
I just updated my implementation, and switched from Canvas to ImageView. ImageView allows runtime resizing of the backbuffer, which is important if one want to enforce certain DPI (such as Retina Display support).
Agreed - I moved to ImageView per John's project. Example: https://github.com/hjohn/MediaSystem/blob/master/mediasystem-ext-player-vlc/src/main/java/hs/mediasystem/ext/player/vlc/VLCPlayer.java
Still the given example for JavaFX (vlcj-javafx) is not updated for Jdk 8. In jdk 8 given example is not working
See caprica/vlcj#277 for possible assistance.
The idea in the referenced issue is to provide, in a synchronised manner, direct access to the native memory buffer. This uses the same semaphore to lock the native buffer as the native video callbacks.
The client application (e.g. a JavaFX application) would still need to manage its own rendering timeline, but rather than copying an entire frame of video every time it could just access this buffer when its own timeline dictates.
This actually seems to work quite well with the latest vlcj snapshot version, including the change for caprica/vlcj#277 mentioned above.
I made changes in the vlcj JavaFX test class using the Timeline idea from @IsNull.
I tested on Kubuntu, JavaFX on the latest JDK 1.8, and the playback was nice and smooth with no wrong thread errors.
So finally I think this issue can be closed - at least in as much as this is probably the best solution.
I would still like to make sure we're using the most efficient way to copy the native memory to the JavaFX component.
This should become part of vlcj 3.1.0 when it is released (not yet at time of writing).
Just want to throw my 2 cents in ..
Rather than using a Timeline for the updates, it makes more sense to me to do something like so:
ObjectProperty< -InfoFromCallback- > renderProperty = new SimpleObjectProperty<>(){
@Override
public void invalidated(Observable o){
if(conditions){
pixelWriter.setPixels(0, 0,
currentFormat.getWidth(),
currentFormat.getHeight(),
pixelFormat, currentBuffer,
currentFormat.getWidth()*4);
}
}
};
Invalidated is always called on FX thread, and is much faster than adding a listener. I do a lot of 3D with javafx and Mesh updates are a bear, but this approach updates are very fast, and no threat of "wrong thread" ...
OK, so in principle I like that approach better - I was never happy with the Timeline.
But...
I'm still not clear on all of the threading semantics with that approach - this is not just about wrong thread errors for JavaFX. With the code you posted, your invalidated method looks to me like it is still accessing the native buffer in an un-synchronised way (so you might render corrupted frames).
The native buffer must be locked at the time your application renders the frame, unless your callback is copying the frame which I doubt.
I mean, I need to see a full example in context to understand it and make sure it's OK, not just this fragment.
After looking at this in more detail, I'm somewhat sceptical, but I'm not a pro JavaFX developer so perhaps I missed something.
The object property must be set on the JavaFX thread, otherwise a wrong thread error is raised.
So the render callback must do Platform.runLater() with a Runnable that updates the "object property", that then triggers the override that invokes the PixelWriter.
That being the case, why not just put all of the rendering code inside the Platform.runLater() Runnable, and forget about the object property completely?
In fact, I did exactly that and got it working, and with proper synchronisation locking/unlocking the native video buffer inside that Runnable too.
It's subjective of course, and very difficult to accurately judge, but with this approach the video playback does not seem as smooth to me, in fact I'm pretty sure it is not as smooth. I guess maybe this is because of the asynchronous nature of the runLater not running at the precise time the frame was ready.
I haven't run a profiler on it yet to compare it with the Timeline approach.
Right now I still favour the Timeline approach over this.
Yes I realized that after I went to try it on an "old" (pre jdk 8) build... So I will offer an approach that I used on a Cloth simulator (3D) ..
https://www.youtube.com/watch?v=uRsCcpbsdsg
Basically create a loop timer, It is ran in the background, and updates ui on completion ..
/**
*
* @author jdub1581
*/
public class NanoTimer extends ScheduledService<Void> {
private final long ONE_NANO = 1000000000L;
private final double ONE_NANO_INV = 1f / 1000000000L;
private long startTime, previousTime;
private double frameRate, deltaTime;
private final NanoThreadFactory tf = new NanoThreadFactory();
int cAcc = 3;
public NanoTimer() {
super();
this.setPeriod(Duration.millis(16));// equivalent to 60 fps
this.setExecutor(Executors.newCachedThreadPool(tf));
}
public double getTimeAsSeconds() {
return getTime() * ONE_NANO_INV;
}
public long getTime() {
return System.nanoTime() - startTime;
}
public long getOneSecondAsNano() {
return ONE_NANO;
}
public double getFrameRate() {
return frameRate;
}
public double getDeltaTime() {
return deltaTime;
}
private void updateTimer() {
deltaTime = (getTime() - previousTime) * (1.0f / ONE_NANO);
frameRate = 1.0f / deltaTime;
previousTime = getTime();
}
@Override
public void start() {
super.start();
if (startTime <= 0) {
startTime = System.nanoTime();
}
}
@Override
public void reset() {
super.reset();
startTime = System.nanoTime();
previousTime = getTime();
}
@Override
protected Task<Void> createTask() {
return new Task<Void>() {
@Override
protected Void call() throws Exception {
updateTimer();
// non fx thread updates ...
return null;
}
};
}
@Override
protected void succeeded() {
super.succeeded();
//update the UI here
}
@Override
protected void failed() {
getException().printStackTrace(System.err);
}
@Override
public String toString() {
return "ElapsedTime: " + getTime() + "\nTime in seconds: " + getTimeAsSeconds()
+ "\nFrame Rate: " + getFrameRate()
+ "\nDeltaTime: " + getDeltaTime();
}
/*==========================================================================
creates a daemon thread for use
*/
private class NanoThreadFactory implements ThreadFactory {
public NanoThreadFactory() {
// add other data if need be
}
@Override
public Thread newThread(Runnable r) {
Thread t = new Thread(r, "NanoTimerThread");
t.setDaemon(true);
return t;
}
}
}//=============================================================================
Can either use a property as I said and set that property in the OnSucceeded or try directly, also
you can take all the Time updates out I'm sure.. Performance was increased 2x in my simulation,
so I think with some cleverness the same could happen here ...
Thanks for posting that, I'll try investigating this approach some more...
After testing this, it is much smoother than the ObjectProperty approach. In fact, the video playback looks absolutely perfect to me.
Time now to do some CPU profiling I guess...
That's awesome to hear !
Note that there are other settings in the scheduled service that should probable be addressed,
like setting a backoff strategy, set to restart on fault maybe? and others.. also playing with the executor as well, maybe a single threaded one would be more appropriate.
This was the best way I found to make a thread loop in javafx..
Some very naive preliminary CPU profiling shows for me both the Timeline solution and the NanoTimer solution use on average between 5% and 6% CPU. There's no discernible difference between the two approaches in terms of CPU usage, at least not for me (i7-3770k).
I think someone who knows JavaFX better than me could make the NanoTimer work more optimally with vlcj, especially taking into account your latest comment. I wouldn't know where to being with backoff strategy and so on.
I think the main difference is when and how they get scheduled in the back end. Timeline has to go through a few more hoops, which is probably where that micro delay was happening. I'd also try playing with the "period" maybe more less?
And yeah, I believe the backoffStrat is just another task to run in case of too many failures..
Which you should probably add some record keeping to the Timer .. And maybe some atomic locks ..
Clearly you know this stuff better than me. :-)
trial and error, trial and error ... :)
Your 3D Cloth simulator is pretty damn impressive.
OK, so I committed a naive implementation using this nano timer to the vlcj-javafx project.
Maybe it can be improved, but right now it works and should help people integrating vlcj into their JavaFX application.
In my own tests, I was able to render full HD at 60fps using a peak of 15% CPU, which seems pretty good to me.
Awesome bud, just glad there was an improvement.. guess I'll have to
re-make my player now as well ;) (haven't touched it in a year or so (-_-)
<--)
On Wed, Jan 7, 2015 at 11:51 AM, Mark Lee notifications@github.com wrote:
OK, so I committed a naive implementation using this nano timer to the
vlcj-javafx project.Maybe it can be improved, but right now it works and should help people
integrating vlcj into their JavaFX application.In my own tests, I was able to render full HD at 60fps using a peak of 15%
CPU, which seems pretty good to me.—
Reply to this email directly or view it on GitHub
caprica/vlcj-javafx#3 (comment).
As I thought about it, if you want to try, you could also use the same concept in an AnimationTimer ..
It's a 60fps loop as well. In the vlc callback just set a boolean flag for when ready to render,
in the timer have an if(ready){writePixels();} .. may be just as viable..
I think a flag is not really needed, the timer action can just lock the native buffer and render whatever's there.
OK, I added a new test with a naive AnimationTimer implementation.
The playback looks pretty smooth to me.
This is clearly the simplest way to do it, in terms of code anyway.