Repository: google/filament
File: filament/backend/src/opengl/OpenGLDriver.cpp (C++)

Code before:
    if (sh) {
        GLStream const* s = handle_cast<GLStream*>(sh);
        return s->user_thread.timestamp;
    }
    return 0;
}

mat3f OpenGLDriver::getStreamTransformMatrix(Handle<HwStream> sh) {
    if (sh) {
        GLStream const* s = handle_cast<GLStream*>(sh);
        if (s->streamType == StreamType::NATIVE) {
            return mPlatform.getTransformMatrix(s->stream);
        } else {
            return s->transform;
        }
    }
    return mat3f();
}

void OpenGLDriver::destroyFence(Handle<HwFence> fh) {
    if (fh) {
        GLFence const* f = handle_cast<GLFence*>(fh);
        if (mPlatform.canCreateFence() || mContext.isES2()) {
            mPlatform.destroyFence(f->fence);
        } else {
            if (f->state.use_count() > 1) {
                // signal waiters it's time to give-up
                std::unique_lock const lock(f->state->lock);
                f->state->status = FenceStatus::ERROR;
                f->state->cond.notify_all();
            }
        }
        destruct(fh, f);
    }
}

FenceStatus OpenGLDriver::getFenceStatus(Handle<HwFence> fh) {
    return fenceWait(fh, 0);
}

FenceStatus OpenGLDriver::fenceWait(FenceHandle fh, uint64_t const timeout) {
    if (fh) {
        // we have to take into account that the STL's wait_for() actually works with
        // time_points relative to steady_clock::now() internally.
        using namespace std::chrono;
        auto const now = steady_clock::now();
        steady_clock::time_point until = steady_clock::time_point::max();
        if (now <= steady_clock::time_point::max() - nanoseconds(timeout)) {
            until = now + nanoseconds(timeout);
        }


Review comment (bug):
I think this line has a race condition with [this shared_ptr copy](https://github.com/google/filament/blob/379b63d11bc37a462bdc56a8a3bc3ff9df20d63b/filament/backend/src/opengl/OpenGLDriver.cpp#L2320), where `fenceWait` is called from the AsyncJob thread. E.g., this line could hit first prior to that shared_ptr copy, resulting in skipping this if block. So I think we can just remove `if (f->state.use_count() > 1)`.

There is another race condition with [this line](https://github.com/google/filament/blob/379b63d11bc37a462bdc56a8a3bc3ff9df20d63b/filament/backend/src/opengl/OpenGLDriver.cpp#L1812). Although both  are called from the engine thread, one could overwrite the other. (very unlikely, but theoretically possible). However, this rare overwriting doesn't affect the deadlock but just report a wrong result. So maybe it's okay to ignore this case.

My suggestion is to
1. either just remove `if (f->state.use_count() > 1)` to fix the first issue (skipping), embracing the second issue (overwriting)
2. or update the status only once by checking if it still has the default value (TIMEOUT_EXPIRED or a new one like NONE), which means it hasn't changed yet, to also handle the second issue. (including [this part](https://github.com/google/filament/blob/379b63d11bc37a462bdc56a8a3bc3ff9df20d63b/filament/backend/src/opengl/OpenGLDriver.cpp#L1812))

```
std::unique_lock const lock(f->state->lock);
if (f->state->status == FenceStatus::TIMEOUT_EXPIRED) {
    f->state->status = FenceStatus::ERROR;
    f->state->cond.notify_all();
}
```


Code after:
    if (sh) {
        GLStream const* s = handle_cast<GLStream*>(sh);
        return s->user_thread.timestamp;
    }
    return 0;
}

mat3f OpenGLDriver::getStreamTransformMatrix(Handle<HwStream> sh) {
    if (sh) {
        GLStream const* s = handle_cast<GLStream*>(sh);
        if (s->streamType == StreamType::NATIVE) {
            return mPlatform.getTransformMatrix(s->stream);
        } else {
            return s->transform;
        }
    }
    return mat3f();
}

void OpenGLDriver::destroyFence(Handle<HwFence> fh) {
    if (fh) {
        GLFence const* const f = handle_cast<GLFence*>(fh);
        if (mPlatform.canCreateFence() || mContext.isES2()) {
            mPlatform.destroyFence(f->fence);
        } else {
            // signal waiters it's time to give-up
            std::unique_lock const lock(f->state->lock);
            f->state->status = FenceStatus::ERROR;
            f->state->cond.notify_all();
        }
        destruct(fh, f);
    }
}

FenceStatus OpenGLDriver::getFenceStatus(Handle<HwFence> fh) {
    return fenceWait(fh, 0);
}

FenceStatus OpenGLDriver::fenceWait(FenceHandle fh, uint64_t const timeout) {
    if (fh) {
        // we have to take into account that the STL's wait_for() actually works with
        // time_points relative to steady_clock::now() internally.
        using namespace std::chrono;
        auto const now = steady_clock::now();
        steady_clock::time_point until = steady_clock::time_point::max();
        if (now <= steady_clock::time_point::max() - nanoseconds(timeout)) {
            until = now + nanoseconds(timeout);
        }

        GLFence const* f = handle_cast<GLFence*>(fh);
        assert_invariant(f->state);