Remove FML_DCHECK around CanReactOnCurrentThread() in `ReactorGLES::CreateUntrackedHandle()#182329
Remove FML_DCHECK around CanReactOnCurrentThread() in `ReactorGLES::CreateUntrackedHandle()#182329b-luk wants to merge 2 commits into
FML_DCHECK around CanReactOnCurrentThread() in `ReactorGLES::CreateUntrackedHandle()#182329Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses a bug in ReactorGLES::CreateUntrackedHandle where a function call with side effects, CanReactOnCurrentThread(), was wrapped in an FML_DCHECK. This meant the call was only made in debug builds, causing issues in release builds. The fix moves the function call out of the FML_DCHECK to ensure it is always executed, while still asserting its return value in debug builds. Additionally, several previously skipped tests for OpenGL are re-enabled in run_tests.py, presumably because this fix allows them to pass now. My review includes a suggestion to add a comment explaining the unconditional call and to use const for the local variable, enhancing code clarity and maintainability.
| bool can_react = CanReactOnCurrentThread(); | ||
| FML_DCHECK(can_react); |
There was a problem hiding this comment.
This is a good fix. For improved clarity and to prevent future regressions, consider adding a comment explaining why this call is necessary outside of the FML_DCHECK. Also, can_react can be const.
// This call has side effects (e.g. setting the current GL context) and must
// be made unconditionally.
const bool can_react = CanReactOnCurrentThread();
FML_DCHECK(can_react);e8bc29b to
a698473
Compare
a698473 to
cc5db37
Compare
| [[maybe_unused]] bool can_react = CanReactOnCurrentThread(); | ||
| FML_DCHECK(can_react); |
There was a problem hiding this comment.
I'm not keen to accept this extra overhead. Do you not need this after my PR that eliminates multithreaded tests?
The issue happens even when multithreading is not enabled.
There was a problem hiding this comment.
Can we just call directly the thing you need from CanReactOnCurrentThread() directly? The setting of the context.
Can we do that setting of the context once when things are initialized instead of each time we make an untracked handle?
There was a problem hiding this comment.
I don't think setting the context here directly would be right because we'd only want to do this when a SwitchableGLContext is used (currently only in tests like with TesterContextGLES). We don't want to do this for setups that don't use a SwitchableGLContext (all cases in prod right now).
We can't set the the context once when things are initialized because different things are initialized at different times, potentially in different threads. The current case that is making the tests fail is because the context is not current when creating a texture for an image, which can happen in a different thread than the main frame loop code which also needs to set the current context to do stuff.
TesterContextGLES had to implement this SwitchableGLContext logic in order to work properly with multithreading. Since we no longer are running opengles tests with multithreading enabled, we should be able to simplify TesterContextGLES and remove all of its SwitchableGLContext logic. If we do this, these test should work without calling CanReactOnCurrentThread() here, so this PR should no longer be needed. I'm working on this change to TesterContextGLES now. I'm running into a different problem with that change, but once it's ready I'll send it out and it should make this PR obsolete.
|
Obsoleted by #183250 |
ReactorGLES::CreateUntrackedHandle()requires the current thread to have the current opengles context. In environments that useSwitchableGLContext, callingCanReactOnCurrentThread()has the side effect of setting the current opengles context. So it must be called. Having it insideFML_DCHECKmade the call only happen in unopt builds, resulting in #182105.Makes progress on #182105. The
GL_FRAMEBUFFER_INCOMPLETE_MISSING_ATTACHMENTissues no longer happen. Newly passing tests:codec_test.dartno longer has theGL_FRAMEBUFFER_INCOMPLETE_MISSING_ATTACHMENTerror, but still fails. It passes on mac, but on linux it fails with:I have a pending fix for this which I will send out afterwards.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.