Page MenuHomePhabricator

evas_render: Process deferred callback in the sync render case.
ClosedPublic

Authored by CHAN on Mar 26 2019, 4:42 AM.

Details

Summary

The EVAS_CALLBACK_RENDER_POST callback has been deferred when the callback is registered during the render(inside_post_render flag on).

In the sync render case, the logic to call deferred callbacks is missing, and callbacks are not being called in certain cases.

@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.
CHAN created this revision.Mar 26 2019, 4:42 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

CHAN requested review of this revision.Mar 26 2019, 4:42 AM
CHAN edited the summary of this revision. (Show Details)Mar 26 2019, 4:49 AM
CHAN added reviewers: ManMower, Hermet.
zmike added a subscriber: zmike.Mar 26 2019, 6:29 AM

Can this get a unit test to avoid future breakages in case evas_render is refactored?

In D8478#153184, @zmike wrote:

Can this get a unit test to avoid future breakages in case evas_render is refactored?

We haven't used sync mode for years. Though we can make a sample but it won't work because test environment is on the async mode.

Hermet accepted this revision.Mar 26 2019, 9:10 PM
This revision is now accepted and ready to land.Mar 26 2019, 9:10 PM
Closed by commit rEFL9367dcc755ec: evas_render: Process deferred callback in the sync render case. (authored by Woochanlee <wc0917.lee@samsung.com>, committed by Hermet). · Explain WhyMar 26 2019, 9:11 PM
This revision was automatically updated to reflect the committed changes.
zmike reopened this revision.Mar 28 2019, 6:50 AM

I would like some clarification on this patch, since I'm looking again at the code and I don't see how it could possibly be triggered. For a problem to exist here, one of two struct members must be non-null:

  1. inside_post_render
  2. rendering

inside_post_render cannot be set at any point during a sync render except immediately around the actual calling of EVAS_CALLBACK_RENDER_POST, so this does not seem to be a problem case
rendering is only possible to be set during async render, so this is also not possible here.

What is the case where this could be triggered? This patch seems wrong to me.

This revision is now accepted and ready to land.Mar 28 2019, 6:50 AM
In D8478#153945, @zmike wrote:

I would like some clarification on this patch, since I'm looking again at the code and I don't see how it could possibly be triggered. For a problem to exist here, one of two struct members must be non-null:

  1. inside_post_render
  2. rendering

    inside_post_render cannot be set at any point during a sync render except immediately around the actual calling of EVAS_CALLBACK_RENDER_POST, so this does not seem to be a problem case rendering is only possible to be set during async render, so this is also not possible here.

    What is the case where this could be triggered? This patch seems wrong to me.

There is no chance to move the 'deferrered render post events list' to 'default render post events list'
In sync mode, default render post will be called properly, but deferred render post won't be triggered, they need to be moved to default render_post list before render_post event is triggered.
So, this patch moves the deferred render post events list to the default render post list (_deferred_callbacks_process() does this) prior to render_post call.

So this patch looks fine to me.

CHAN added a comment.Mar 31 2019, 10:09 PM

@zmike

If someone register a render post callback within a render post callback.

In case of async case the 'evas_render_pipe_wakeup()' is called

so that deferred render post callback is normally called,

but sync does not call deferred render post callback. still deferred...

It seems that the case of the sync render case is missing when a feature is added that defer the render post callback call.

zmike added a comment.Apr 1 2019, 6:13 AM

I understand the points that you have both raised, but if you see my previous comment it seems impossible for there ever to be any deferred post render callbacks in this scenario. If a post render callback is added prior to this point then it will never be deferred, which makes this patch wrong. What do you think about that?

raster added a subscriber: raster.Apr 1 2019, 7:10 AM

@Hermet - actually gl rendering is still sync mode... :)

Hermet added a comment.EditedApr 1 2019, 6:30 PM
In D8478#154452, @zmike wrote:

I understand the points that you have both raised, but if you see my previous comment it seems impossible for there ever to be any deferred post render callbacks in this scenario. If a post render callback is added prior to this point then it will never be deferred, which makes this patch wrong. What do you think about that?

can't catch your point... What do you mean by "this point" in "If a post render callback is added prior to this point"?

The condition is here.... if (rendering or inside_post render) in sync,

if ((e->rendering || e->inside_post_render) && type == EVAS_CALLBACK_RENDER_POST)
  {
     e->deferred_callbacks = eina_inlist_append(e->deferred_callbacks,
                                                EINA_INLIST_GET(cb_info));
  }
Hermet added a comment.Apr 1 2019, 6:35 PM

@Hermet - actually gl rendering is still sync mode... :)

@raster eh? yes it's true... thanks :)

zmike added a comment.Apr 2 2019, 5:28 AM

@Hermet - actually gl rendering is still sync mode... :)

@raster eh? yes it's true... thanks :)

Okay, I get it now, thanks for replying again.

zmike closed this revision.Apr 2 2019, 5:28 AM