Allow the GrContext to be created after IOManager etc. #7243
Conversation
dnfield
requested review from
cbracken,
chinmaygarde,
jason-simmons and
zanderso
| public: | ||
| virtual sk_sp<GrContext> CreateResourceContext() = 0; | ||
|
|
||
| // In almost all situations, this should be prefered to |
|
|
||
| // In almost all situations, this should be prefered to | ||
| // `CreateResourceContext`. This will get the currently applicable GrContext | ||
| // on the calling thread for the current GrContext, if one is available. |
There was a problem hiding this comment.
should "GrContext" be changed to "GLContext" here?
| fml::WeakPtr<GrContext> PlatformView::GetOrCreateWeakResourceContext() { | ||
| auto context = GetOrCreateResourceContext(); | ||
| if (context) { | ||
| return fml::WeakPtrFactory<GrContext>(context.get()).GetWeakPtr(); |
There was a problem hiding this comment.
I don't think this is safe. This WeakPtrFactory will be destructed immediately, resulting in invalidation of its weak pointers.
I think the PlatformView needs to hold a WeakPtrFactory allocated on the heap (similar to how IOManager::resource_context_weak_factory_ worked)
There was a problem hiding this comment.
I think you're right, but I'm not sure we can safely create this on the PlatformView either. It has to be created on the same thread as what will use it, and the methods for this class are documneted as being safe to access from any thread. I think I just have to move it back to IOManager - but it still seems fishy to me about how it's actually working there.
There was a problem hiding this comment.
Acutally I'm not sure that can make sense either. I'm not seeing how we could use a WeakPtr for this at all - perhaps it would make more sense to just have consumers work with the sk_sp instead, and check if the GrContext is abandoned or null.
|
@dnfield I didn't notice any embedding files changed on the Android side - did I miss something? |
|
The Android platform view implementation |
|
Ok, I'll defer entirely to @amirh on that front for this PR |
|
@matthew-carroll I think Dan meant Android platform view as in (the term "platform view" is ambiguous in the engine repository right now, @dnfield and I and a couple others discussed that, and we figured we should consider renaming one of the "platform views" at some point). |
|
Got it. @amirh @dnfield I would recommend changing the term that exists inside the engine rather than the term that has been marketed publicly. @timsneath @eseidel and @Hixie may also have some input on that. Please let me know when you begin brainstorming new names :) |
|
I'm going to close this and open a new PR with a different approach. I can't find a good way to do this so that the resource context is created on the io task runner without reverting almost everything I've done here. |
5 participants
Fixes flutter/flutter#24798
The PlatformView can be created before the GLContext is available/current. This means that whatever GrContext (if any) that is fed to IOManager, UIDartState, and DartIsolate can be incorrect.
This patch changes constructor parameters expecting a
GrContextin some shape or form to take aResourceContextManagerinstead, which is implemented byPlatformViewto ensure that callers get the correctGrContextassociated with the currentGLContext./cc @matthew-carroll - this required a change to the Android embedding.
/cc @zanderso - this will require changes in Fuchsia.