Page MenuHomePhabricator

evas engines: do not immediately free native surface when unsetting it
ClosedPublic

Authored by zmike on May 31 2018, 4:11 AM.

Details

Summary

this is a longstanding issue which was exposed by recent patches to standardize
object lifecycles. when a native surface is used by multiple images, unsetting
the surface from one image must not destroy the native surface or else the
remaining images

this patch is incomplete

fix T6970

@fix

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zmike created this revision.May 31 2018, 4:11 AM
zmike requested review of this revision.May 31 2018, 4:11 AM
zmike added a comment.May 31 2018, 4:21 AM

This is a sample patch which needs some work/testing and also needs to be propagated to other engines (in future revisions of this patch, not in separate patches).

Do not merge this for now, I'm only posting so we can start a discussion about it.

zmike updated this revision to Diff 14856.May 31 2018, 6:59 AM
zmike retitled this revision from gl_drm: do not immediately free native surface when unsetting it to evas engines: do not immediately free native surface when unsetting it.
zmike edited the summary of this revision. (Show Details)

add handling for some other engines

zmike added a comment.May 31 2018, 7:00 AM

There's some similar code in software_XYZ engines as well as wayland_shm, but I'm not sure if it's going to exhibit the same issues. @ManMower can you check these out in a bit more depth?

I can confirm that the gl_drm change makes rendering appear as it did before the recent lifecycle changes landed.

This is ringing alarm bells for me because I've made changes in these bits before and I can't remember exactly where. I think there may be some code outside of our control that's expecting "if (!ns) return img;" to continue happen?

Is native.func.free() supposed to be doing some kind of refcounting here? It feels like that would have similar effect?

The wayland_egl engine appears to be a copy paste of the same code here - I don't think it can have the same issue, but I think would benefit from the same fix just to keep the code in sync. Would you like to follow up with your same change on that engine in another revision so it's all in one patch?

The wayland_shm engine is quite different, and I think needs no change.

Definitely looks to me like you've got the root cause here, sorry for the bike sheddy questions.

zmike added a comment.Jun 4 2018, 4:20 AM

I've added the wayland-egl changes in. Native surface internals don't appear to have any refcounting despite being used in a manner which requires it--likely because Evas_GL_Image has refcounting and should be the only direct user of it for cases like this? I'm not sure if that's accurate but it seems like it should be the case based on how the code is written.

The idea behind the change is to avoid bypassing this refcounting, as (I think?) the only time a native surface should be used is for a gl engine. With all of that said, there is native surface code in the software engines, but it looks like none of these get shared like the native surfaces in GL, so this is probably okay for now.

ManMower requested changes to this revision.Jun 4 2018, 10:21 AM
ManMower added inline comments.
src/modules/evas/engines/gl_generic/evas_engine.c
491

I think this shouldn't be glsym prefixed in this occurrence. Other calls in this file are direct, and I get a symbol resolution error from glsym_...

This revision now requires changes to proceed.Jun 4 2018, 10:21 AM
zmike updated this revision to Diff 14877.Jun 5 2018, 7:02 AM

fix symbol resolution error

zmike added inline comments.Jun 5 2018, 7:14 AM
src/modules/evas/engines/gl_generic/evas_engine.c
491

Good catch.

ManMower accepted this revision.Jun 5 2018, 10:12 AM

This works on the drm engines for me. I'm unable to test gl_x11 but I'll assume it's resolved the same problem there for you.

Normally I'd like to see further review and testing for something this far reaching, but given the severity of the bug and the dialog it created on the mailing list I'm going to roll the dice and push this out now.

I'll remove the "this patch is incomplete" warning while landing.

Thanks for taking care of this.

This revision is now accepted and ready to land.Jun 5 2018, 10:12 AM
This revision was automatically updated to reflect the committed changes.