[impeller] OpenGL: Add support for threads and contexts. #33575
Conversation
I have temporarily made GL errors log instead of assert. But the only issues we run into are with the "create on bind" resources and setting debug object labels on them. I am going to rework setting debug labels right now. But, in the meantime, the OpenGL ES backend can show cross-context textures. There is also still an outstanding issue where cross-context textures will be uploaded on the main context (in the same sharegroup) if the thread on which the operation is dispatched doesn't have a context current. This will also be fixed to pin those tasks to the worker context in an upcoming patch.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
The screenshot reminds me, there is no mip-mapping just yet either. |
| { | ||
| Lock ops_lock(ops_mutex_); | ||
| std::swap(gl_handles_to_collect_, gl_handles_to_collect); | ||
| FML_DCHECK(gl_handles_to_collect_.empty()); |
There was a problem hiding this comment.
Is this runtime asserting the behavior of swap?
There was a problem hiding this comment.
Umm, yeah. Thats possibly a bit much. I'll get rid of it in a subsequent patch.
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
Change seems good, it'd be good to make sure this builds in google3 before landing
Is there anything in particular that seems unique about this patch vis-a-vis building in g3? I thought this was much like the rest of the work on ES2. |
|
impeller isn't getting rolled into google3, so I added some compiler guards around other code that references impeller classes/headers. You seem to have added some more in this patch, specifically to files that don't match |
|
(We're in a world right now where the android engine needs to be able to build as if the |
|
The two files that are not _impeller or in the impeller/ dep are using forward declarations already. So I think we are good. |
chinmaygarde
added
the
waiting for tree to go green
|
Worst frame time started to vary wildly after this PR: It sounds like that might be expected. Is this addressed by #33598 ? |
|
The issue with the varying worst frame times is because the texture might be uploaded using the main context instead of the resource context. See the caveat in the original commit message: #33598 doesn't address this but the commit right after should as that patch sets up the necessary refactors for addressing pinned tasks. Rest assured, the regressions are due to images actually being uploaded and displayed, just using the wrong context. If we didn't have a resource context in Flutter, thats what the benchmarks would look like. I'll keep a lookout on the benchmarks once we call the OpenGL ES backend work done for this milestone. Thanks. |
4 participants
I have temporarily made GL errors log instead of assert. But the only issues we
run into are with the "create on bind" resources and setting debug object labels
on them. I am going to rework setting debug labels right now. But, in the
meantime, the OpenGL ES backend can show cross-context textures.
There is also still an outstanding issue where cross-context textures will be
uploaded on the main context (in the same sharegroup) if the thread on which the
operation is dispatched doesn't have a context current. This will also be fixed
to pin those tasks to the worker context in an upcoming patch.