ofTheo / ofxKinect

legacy openFrameworks wrapper for the xbox kinect (OF pre-0.8.0+ only) - ofxKinect is now included and is being maintained in OF releases
MIT License
540 stars 105 forks source link

ofxKinect crashes now on exit #79

Closed ofTheo closed 13 years ago

ofTheo commented 13 years ago

just noticed this since the all the new fixes. the previous develop version I was using never crashed on exit. maybe something with the threading - need to stop the thread first?

ofxKinect* kinect = kinectContext.getKinect(dev);

if(kinect->kinectDevice == dev && kinect->lock()) {
    try {
        freenect_frame_mode curMode = freenect_get_current_video_mode(dev);
here ------->   memcpy(kinect->videoPixelsBack, rgb, curMode.bytes);
        kinect->bNeedsUpdate = true;
    }
    catch (...) {
        ofLog(OF_LOG_ERROR, "ofxKinect: Rgb memcpy failed for device %d",
            kinect->getDeviceId());
    }
    kinect->unlock();
}
kylemcdonald commented 13 years ago

which develop version were you using previously? if you know, we can do a blame and figure out what thread-related stuff has changed. i'm pretty sure all the changes have been superficial though. (personally, i've gotten crashes on exit intermittently since the first ofxKinect.)

danomatika commented 13 years ago

I haven't noticed this problem.

Where is that code from? One of your apps? videoPixelsBack is cleared on exit ... perhaps you should check if it's null before doing a memcpy.

ofTheo commented 13 years ago

Just hit me again, during running too! :) Curious why there is a try catch around memcpy - I'm pretty sure it doesn't throw exceptions

I believe it was the develop from here: https://github.com/ofTheo/ofxKinect/commit/41f13ee504365001ed3db044f8325d29c4754c07

but I was never using rgb - so maybe that is why I didn't see it.

danomatika commented 13 years ago

I may see the problem.

ofThread.lock() returns a boolean only if it's non blocking, otherwise it always returns true. The thread is set to blocking in ofxKinect::open(), so kinect->lock() always returns true ... as it's being called within the if statement, there may be an issue where the lock isn't being called properly between the time the thread stops and freenect_stop_video() is called.

It would be safer to move the locks to a line before the try/catch block.

danomatika commented 13 years ago

Are you using multiple kinects or ?

danomatika commented 13 years ago

Curious why there is a try catch around memcpy - I'm pretty sure it doesn't throw exceptions

I didn't know it dosen't throw exceptions. You learn something new every day ...

ofTheo commented 13 years ago

this is what I get to crash it.

    kinect.init(true, true, true);
    kinect.open(0);

    kinect.update();

    kinect.draw(0, 0);

looking at ofThread lock - I don't see a problem with the implementation

if it is blocking it returns true because mutex.lock() blocks - so it has to succeed. or more correctly it will wait forever till it succeeds.

//------------------------------------------------- 
//returns false if it can't lock 
bool ofThread::lock(){ 

    if ( blocking )
    {
        if(verbose)printf("ofThread: waiting till mutex is unlocked\n"); 
        mutex.lock();
    }
    else
    {
        if ( !mutex.tryLock() )
        {
            if(verbose)printf("ofThread: mutex is busy - already locked\n"); 
            return false; 
        }
    }
    if(verbose)printf("ofThread: we are in -- mutex is now locked \n"); 

    return true; 
}
kylemcdonald commented 13 years ago

there is no problem with ofThread::lock(). the issue is in the way the lock() is used in ofxKinect. if you have a blocking lock() it's misleading to write if(lock()) because that makes it look like it's non-blocking.

danomatika commented 13 years ago

the issue is in the way the lock() is used in ofxKinect.

Right. Even though it's probably not the issue at hand, that's what I'm noting ... the blames on you for that one :D

ofTheo commented 13 years ago

hmm looks like the issue is when you request the IR point image instead of the color image.

kinect.init(false, true, true); is fine

kinect.init(true, true, true); crashes 1 out of 4 times for me.

just using one kinect.

T

danomatika commented 13 years ago

Yeah I see it. I ran the example 10 times with no problem. Then I commented out setRegistration and an instant crash.

danomatika commented 13 years ago

Ok, I seemed to have fixed this. Will commit to develop soon.

danomatika commented 13 years ago

Try the latest develop and see if that fixes your problems.

danomatika commented 13 years ago

... so has anyone tested the updates in develop? It's working for me.

ofTheo commented 13 years ago

works for me!