Page MenuHomePhabricator

evas: add 'has_fixed_size' property for canvas objects
ClosedPublic

Authored by zmike on May 13 2019, 9:24 AM.

Details

Summary

this provides a hint for rendering that the object is not going to resize
for as long as the flag is set and allows for some optimizations to
be made during rendering based on this knowledge

@feature

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 13 2019, 9:24 AM
zmike requested review of this revision.May 13 2019, 9:24 AM
YOhoho added a subscriber: YOhoho.May 15 2019, 9:24 PM
YOhoho added inline comments.
src/lib/evas/include/evas_private.h
1189

We already have is_static_clip property and evas_object_static_clip_set/get legacy APIs.
What do you think to use this?

zmike added inline comments.May 16 2019, 6:43 AM
src/lib/evas/include/evas_private.h
1189

Oh I forgot about this. The legacy static clipper is conceptually different, as that is for a clipper which can be resized but not moved. This patch is for a clipper which can be moved but not resized.

Hermet added a comment.EditedMay 22 2019, 10:31 PM

I have no idea how this api is guaranteed for it's purpose... if It's unlikely work as you purposed. How user guarantee clipper won' be changed?...

I find the name confusing as it seems it has confused everyone who looked at this patch. What about clipper_static_size? @segfaultxavi an opinion on the subject?

It is indeed confusing having vars named is_static_clip and clipper_is_static in two different structures (_Evas_Object_Protected_Data and _Evas_Object_Protected_State).

First off, if this is a property of the clipper, why is it added to the clipped object instead of the clipper?
The code for _efl_canvas_object_efl_gfx_entity_size_set already shows how cumbersome this is since a clipper has to walk its list of clippees to see if any of them considers it to be static.

Regarding the API names, if both (legacy and new) are to be kept because they really have different meanings, this difference has to be explicit: clipper_has_static_size, clipper_has_fixed_size, clipper_is_unresizable...
Ideally, we would also rename the legacy API but we cannot.

Also, can both flags be moved to the same structure?

zmike added a comment.May 30 2019, 5:35 AM

It seems like there's some confusion about this API, so I'll try to explain the concept a bit better so that we can have a good discussion.

The purpose of this functionality is to provide a method of informing render internals that a given clipper's size will not change. This is important for the purpose of mask and proxy rendering because both of these create an intermediate surface (hereafter P) to draw the mask/proxy onto before rendering the final result onto the object's real surface (hereafter R). In the case where the proxy/mask is clipped to a size smaller than this real surface (and, for proxies, in the case where another proxy is not sampling from this proxy), the size of P can be reduced to the visible area in order to provide significant improvements to both memory usage and performance.

As far as naming goes, perhaps clipper_has_fixed_size is the best?

The property could indeed be moved onto the clipper object; I'll wait to do any revising until the naming is decided upon. I will not, however, make any changes to the legacy API or implementation.

Thanks for the context!

has_fixed_size looks fine to me as a property name. If it is moved to the clipper object then the clipper_ part of the name needs to be dropped, obviously.

zmike added a comment.May 30 2019, 5:55 AM

Thanks for the context!

has_fixed_size looks fine to me as a property name. If it is moved to the clipper object then the clipper_ part of the name needs to be dropped, obviously.

I'm not so sure about dropping clipper from the name; this functionality only exists for objects which are clippers, so shouldn't the naming indicate this?

segfaultxavi requested changes to this revision.May 30 2019, 7:44 AM
In D8887#166127, @zmike wrote:

I'm not so sure about dropping clipper from the name; this functionality only exists for objects which are clippers, so shouldn't the naming indicate this?

AFAICS this is purely an Efl.Canvas.Object property which tells that the object won't change its size, and prints and error if it tries to. There's nothing related to clipping here.

In a later patch this property will be used to optimize clipping calculations, but that's independent. We could also use this property to optimize something else, and then the clipper part of the name would be awkward.

This revision now requires changes to proceed.May 30 2019, 7:44 AM
zmike added a comment.Jul 12 2019, 6:53 AM

The requirement is that it does not change size, this is true, but the fully optimized render path requires that it does not change size or move. For some cases moving will not affect the speed, but it requires some deep knowledge of render internals to be able to make that distinction.

zmike updated this revision to Diff 23499.Jul 18 2019, 6:24 AM
zmike retitled this revision from evas: add 'clipper_is_static' property for canvas objects to evas: add 'has_fixed_size' property for canvas objects.

rename to has_fixed_size

segfaultxavi requested changes to this revision.Jul 18 2019, 8:28 AM
segfaultxavi added inline comments.
src/lib/evas/canvas/efl_canvas_object.eo
142

Now these docs confuse me. Who has fixed size? this object or its clipper?

I would say that this is a hint so the renderer can attempt some optimizations, for example related to clipping.

This revision now requires changes to proceed.Jul 18 2019, 8:28 AM
zmike updated this revision to Diff 23504.Jul 18 2019, 9:28 AM
zmike edited the summary of this revision. (Show Details)

reword

segfaultxavi resigned from this revision.Jul 18 2019, 9:30 AM

I have no further complains regarding documentation. Was the code ever reviewed? @Hermet ?

cedric accepted this revision.Thu, Aug 8, 4:13 PM
This revision is now accepted and ready to land.Thu, Aug 8, 4:13 PM
This revision was automatically updated to reflect the committed changes.