Race condition on Frame returned by `VideoCapture::getLastFrame`

[stereolabs]

Race condition on Frame returned by `VideoCapture::getLastFrame`

[Myzhar]

Yes, it takes advantage of the double buffering already available with UVC

[Myzhar]

Yes, a copy is always the best solution in similar cases... I will test it further

[Myzhar]

Hi @peds
thank you for reporting this issue.
I think that there are two possible solutions to fix it:

1. double buffering (or triple...)
2. returning a copied buffer

The first solution introduces a possible frame latency. The UVC driver already implements a multi-buffer mechanism (we use double buffering), but it can be not enough:

zed-open-capture/src/videocapture.cpp

Line 827 in 969d153

memcpy(mLastFrame.data, (unsigned char*) mBuffers[mCurrentIndex].start, mBuffers[mCurrentIndex].length);

The second solution requires more memory and can be slow for hosts with slow memory.

Any thoughts?

[Myzhar]

@peds can you test if this branch solves the problem:
https://github.com/stereolabs/zed-open-capture/tree/fix_race_cond

[peds]

Hi @peds thank you for reporting this issue. I think that there are two possible solutions to fix it:

1. double buffering (or triple...)

2. returning a copied buffer

The first solution introduces a possible frame latency. The UVC driver already implements a multi-buffer mechanism (we use double buffering), but it can be not enough:

zed-open-capture/src/videocapture.cpp

Line 827 in 969d153

memcpy(mLastFrame.data, (unsigned char*) mBuffers[mCurrentIndex].start, mBuffers[mCurrentIndex].length);

The second solution requires more memory and can be slow for hosts with slow memory.

Any thoughts?

Hi @Myzhar,

Thank you for getting back to me so fast!
I did implement a solution that does work by making a copy of the last frame that is only updated when getLastFrame is called. I think this is similar to your second proposed solution?

Did you implement double buffering in https://github.com/stereolabs/zed-open-capture/tree/fix_race_cond?
I will test this and get back to you.

[peds]

The issue persists even with double buffering, but it seems to be more subtle.

I think the most reliable solution would be to return a copy of mLastFrame. The following worked for me.

static void copyFrame(Frame& dstFrame, Frame& srcFrame){
    if (srcFrame.data != nullptr) {
        int bufSize = srcFrame.width * srcFrame.height * srcFrame.channels;
        if(dstFrame.data == nullptr ){
            dstFrame.data = new unsigned char[bufSize];
        }
        // memcpy( dstFrame.data, srcFrame.data, bufSize);
        std::copy( srcFrame.data, srcFrame.data + bufSize, dstFrame.data);
    }
    dstFrame.frame_id = srcFrame.frame_id;
    dstFrame.timestamp = srcFrame.timestamp;
    dstFrame.width = srcFrame.width;
    dstFrame.height = srcFrame.height;
    dstFrame.channels = srcFrame.channels;
}

const Frame& VideoCapture::getLastFrame( uint64_t timeout_msec )

{
    // ----> Wait for a new frame
    uint64_t time_count = timeout_msec*10;
    while( !mNewFrame )
    {
        if(time_count==0)
        {
            // const std::lock_guard<std::mutex> lock(mBufMutex);
            // copyFrame(mLastFrameCopy, mLastFrame);
            return mLastFrameCopy;
        }
        time_count--;
        usleep(100);
    }
    // <---- Wait for a new frame

    // Get the frame mutex
    const std::lock_guard<std::mutex> lock(mBufMutex);
    mNewFrame = false;
    copyFrame(mLastFrameCopy, mLastFrame);
    return mLastFrameCopy;
}

Declaration of Frame mLastFrameCopy is also added to

zed-open-capture/include/videocapture.hpp

Line 489 in dfa0aee

Frame mLastFrame; //!< Last grabbed frame

And

    if(mLastFrameCopy.data)
    {
        delete [] mLastFrameCopy.data;
        mLastFrameCopy.data = nullptr;
    }

is added to //deinint in

zed-open-capture/src/videocapture.cpp

Line 186 in dfa0aee

if(mLastFrame.data)

Edit:
I commented out

const std::lock_guard<std::mutex> lock(mBufMutex);
copyFrame(mLastFrameCopy, mLastFrame);

in getLastFrame before the timeout return, because the last frame has not changed.

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment otherwise it will be automatically closed in 5 days