Page MenuHomePhabricator

Efl.Ui.Animation_View: Implements Efl.Player interface
ClosedPublic

Authored by jsuya on Dec 11 2019, 9:06 PM.

Details

Summary

Chnaged API
.speed.set => Efl.Player.playback_speed.set
.speed.get => Efl.Player.playback_speed.get
.progress.get => Efl.Player.playback_progress.get
.play => Efl.Player.playing.set(true)
.stop => Efl.Player.playing.set(false)
.pause => Efl.Player.paused.set(true)
.resume => Efl.Player.paused.set(false)
.play_sector => .playing_sector
.auto_play => .autoplay
.auto_repeat => .autorepeat

Remove API
.is_playing_back
.playback => (use negative speed value)

New feature API
Efl.Player.playback_position.set
Efl.Player.playback_position.get
Efl.Player.playing.get
Efl.Player.paused.get

ref T8476

Test Plan

meson_option.txt -> remove json in evas-loaders-disabler option
elementary_test -to "animation view"

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes

I am not sure how this widget should be stabelized TBH, it looks like some features do not work without a transit obejct beeing set ?

src/lib/elementary/efl_ui_animation_view.c
715

this should be an error?

756

I think this should be an error ?

776

I think this should be an error ?

780

Can there be safety check for pd->speed beeing > 0

src/lib/elementary/efl_ui_animation_view.eo
123–124

Why is this needed ? In the animation framework playing backwards is just a negative speed maybe we could adjust to that in Efl.Player ? (cc @segfaultxavi)

This revision now requires changes to proceed.Dec 13 2019, 3:23 AM

Yeah, I don't understand the need for the playing_backward property, when the Player interface already has a playback_speed that is more generic. I would remove playing_backward.

For the future, please do not use the words "play" and "back" in the same property name: It is confusing because "playback" is already a word.

src/lib/elementary/efl_ui_animation_view.eo
53–54

typo

jsuya updated this revision to Diff 27560.Dec 15 2019, 10:13 PM

Modify some code and typo and..

Hi @bu5hm4n @segfaultxavi thanks for reviews

The playback_speed property in efl_player interface is described as "The play speed in the [0, infinity) range.]" in docs
Negative speed is undefined behavior. is right?

And before implementing efl_player, playing_set and playing_backward_set were play and playback.
We used playback as a "rewind". We thought "rewind" was not appropriate.
The speed property should not use negative values.
I don't think we should change its docs or change the existing behavior.

We need a rewind function to keep the playing_set context.
Is there a name you can recommend?

Transit objects are created when the animation file is loaded and
managed according to the animation object's life cycle and animation play behavior.

Can using transit be a problem? Should I replace transit with something else?

The playback_speed property in efl_player interface is described as "The play speed in the [0, infinity) range.]" in docs

Oh, I'm sorry, I had not realized the docs said so. Still, I think it is better to handle reverse playback using the speed property.
We can change the docs and make playback_speed accept negative speeds, as @bu5hm4n was suggesting.
I think this is much simpler to understand than having another method for reverse playback (look at all the problems you have to find a good name for it!).

Hermet added a comment.EditedDec 16 2019, 8:35 PM

@bu5hm4n @segfaultxavi theoretically, speed is a scalar, there is no negative value in speed. it's not velocity, If you learned the physics. efl_player_playing_reverse_set() / efl_player_playing_rewind_set() is much clear, imo.

@bu5hm4n @segfaultxavi
I also agree with efl_ui_animation_view_playing_reverse_set() or efl_ui_animation_view_playing_rewind_set() (or new method in efl_player interface)
If playback_speed is changed to use negative value, you should recheck the behavior of other APIs that implement playback_speed.
For example, see _efl_canvas_layout_efl_player_playback_speed_set.
Please recommend the proper way for me. I will follow it

Speed can be seen a scalar. But you can also see it as a speed with a direction, in this case, the negativity of the speed is just seen as a inversion of the direction, which makes the speed again a normal scalar. (Last but not least: we are talking about playback speed, not physical speed per se)
And this point i would say that leaving the docs in Efl_Player like they are now is enough, and simply document it in the class documentation that also negative speeds are accepted.
To me the solution right now is a bit weird, other APIs (within EFL and outside EFL) are solving the "play in reverse" problem with negative speed, so we should be inline with that IMO.

About the transit object: I am a bit worried about the usage. Elm_Transit does not have a eo port, and i am not sure what the future plans are. Sure, this is just internal, but isn't there a other eo API that could be used for this ?

As @bu5hm4n says, this is playback speed, not a physical speed. All other toolkits use negative speeds to perform reverse playback:
https://doc.qt.io/qt-5/qmediaplayer.html#playbackRate-prop
https://gstreamer.freedesktop.org/documentation/tutorials/basic/playback-speed.html?gi-language=c

Rename it to playback_rate if you feel more comfortable, but you'll have a lot of renames to do :)

src/lib/elementary/efl_ui_animation_view.eo
50

We already have Efl.Ui.Autorepeat, so this auto_repeat here should also be a single word autorepeat.
Alternately, we could rename it to loop. This is more common, as you can see the docs already use this word to describe this behavior.

@bu5hm4n @segfaultxavi

I understand how to use speed as a negative value. and I know that other libraries use it that way.
What I want to say is... other APIs that implement Efl.Player.playbackspeed do not define a nagative case.

 EOLIAN static void
 _efl_ui_image_efl_player_playback_speed_set(Eo *obj EINA_UNUSED, Efl_Ui_Image_Data *sd, double factor)
 {  
    EINA_SAFETY_ON_TRUE_RETURN(factor < 0.0);
    EINA_SAFETY_ON_TRUE_RETURN(EINA_DBL_EQ(factor, 0.0));
    if (EINA_DBL_EQ(sd->playback_speed, factor)) return;
    sd->playback_speed = factor;
    if (sd->edje)
      efl_player_playback_speed_set(sd->img, factor);


EOLIAN static void
_efl_ui_image_zoomable_efl_player_playback_speed_set(Eo *obj EINA_UNUSED, Efl_Ui_Image_Zoomable_Data *sd, double factor) 
{
   EINA_SAFETY_ON_TRUE_RETURN(factor < 0.0);
   EINA_SAFETY_ON_TRUE_RETURN(EINA_DBL_EQ(factor, 0.0));
   if (EINA_DBL_EQ(sd->playback_speed, factor)) return;


 EOLIAN void
 _efl_canvas_layout_efl_player_playback_speed_set(Eo *obj EINA_UNUSED, Edje *pd , double speed)
 {
    if (speed <= 0.0) speed = 1.0;
    pd->duration_scale = 1.0/speed;

I know the speed of Efl.Canvs.object_Animation implements the negative case.
However, it does not consider speed changing in real time because it pre-defines at start animation.
I think the above APIs will not guarantee safe behavior in real time in the negative case of speed.

If I implement Efl.Canvas.Object_Animation Mixn I will consider the negative case.
but, Efl.Player is already defined and in use.
In this patch speed value of use case for other libraries is not important.
If the Efl.Player Interface makes a change that takes into account efl_ui_image and efl_canvas_layer, we will consider it.

I'm already working on that the direction of playback is changed when the sign of speed changed during playback.
But I don't think it's right. Do you think you should use negative speed nonetheless?

@Hermet
In my opinion, not implementing Efl.Player might be another way.
If so, play, stop, resume, pause, speed and so on.. should be replaced with the appropriate method name.

and
@bu5hm4n

About the transit object: I am a bit worried about the usage. Elm_Transit does not have a eo port, and i am not sure what the future plans are. Sure, this is just internal, but isn't there a other eo API that could be used for this ?

Sorry. I still do not understand.
It doesn't mean I can't change transit to something else.
However, this is only used internally and I don't think that using eo can't be an improper reason.
We will investigate further about changing elm_transit.

After carefully reading your answer, I think the main source of confusion is that supporting negative speeds should be OPTIONAL.
The docs for Efl.Player.playback_speed should say: "The range for playback_speed is [-inf, inf] but implementations of this interface are free to restrict this range."
The docs for each implementation then will specify the range they support.
So, for example, Efl.Ui.Image and Efl.Canvas.Layout will only support positive speeds, but Efl.Ui.Animation_View can support the whole range.

Now, to answer your specific doubts:

What I want to say is... other APIs that implement Efl.Player.playbackspeed do not define a nagative case.

Correct, because it is optional.

I know the speed of Efl.Canvs.object_Animation implements the negative case.
However, it does not consider speed changing in real time because it pre-defines at start animation.

Correct. Other implementations of Efl.Player are free to do the same if they want. It just needs to be documented.

I think the above APIs will not guarantee safe behavior in real time in the negative case of speed.

Probably not, but it does not matter, because they do not have to support negative speeds.

If I implement Efl.Canvas.Object_Animation Mixn I will consider the negative case.

I do not understand this. The Efl.Canvas.Object_Animation mixin is already implemented, and some classes (like Efl.Canvas.Object) include it.
But this mixin is unrelated to Efl.Player. You should not need it at all for Efl.Ui.Animation_View.

I'm already working on that the direction of playback is changed when the sign of speed changed during playback.
But I don't think it's right. Do you think you should use negative speed nonetheless?

With the consideration that the support for negative speeds is optional, do you still think this isn't right?
I still think that allowing negative speeds in the classes that support reverse playback is far more natural than having a separate method.

@Hermet
In my opinion, not implementing Efl.Player might be another way.

This is how it was initially implemented, and we requested it to implement the Efl.Player interface. It is designed specifically for this purpose, so it would be hard to explain why we don't use it here. The interface might need adjusting, which is exactly what we are doing here :)

Hermet added a comment.EditedDec 18 2019, 3:13 AM

@jsuya @segfaultxavi @bu5hm4n Speed with direction is vector. Obviously. we don't think it speed. Maybe rate() is better than speed in this case. Plus rewind and negative playing is conceptually different. rewind is revert the playing for the amount which have been progressed from the beginning. Negative playing is regardless of the rewind. it's just playing to opposite way. Thus, rewind in animation view is necessary for user convenient who really want to rewind animation (i.e cancel effect it might started effect animation or not.)

@bu5hm4n using transit inside is out of this topic. Probably we can make an another task for this.

jsuya updated this revision to Diff 27622.Dec 18 2019, 3:15 AM

Update code
.playback => (Remove: use negative speed value)
.auto_play => .autoplay
.auto_repeat => .autorepeat
('loop' is conflict with efl_loop_consumer.loop
eolian: efl_ui_animation_view.eo:60:10: function 'loop' conflicts with another symbol (at efl_loop_consumer.eo:13:10 )

This comment was removed by Hermet.
jsuya updated this revision to Diff 27623.Dec 18 2019, 3:18 AM

fix sample's bug

jsuya updated this revision to Diff 27624.Dec 18 2019, 3:20 AM
jsuya edited the summary of this revision. (Show Details)

update message

bu5hm4n requested changes to this revision.Dec 18 2019, 4:04 AM

Thank you for the change with play speed. Two last things:

src/lib/elementary/efl_ui_animation_view.c
769

These two conditions should probebly be EINA_SAFETY checks. (Thats how it is done in spin_button progressbar etc.)

788

Safety check for speed != 0 ?

This revision now requires changes to proceed.Dec 18 2019, 4:04 AM
segfaultxavi requested changes to this revision.Dec 18 2019, 4:10 AM

I just have one more comment regarding the API.
Please mark already-processed comments as DONE.

src/lib/elementary/efl_ui_animation_view.eo
105

This is a method, so its name should be a verb in infinitive, and objects should come before it (i.e. sector_play).

A more important note, though: I think it is weird we have two different mechanisms to start playback (Efl.Player.playing and Efl.Ui.Animation_View.sector_play). Why don't we create some sector-selection properties (like first_sector and last_sector) and then use them when playback starts?

The idea is that the whole EFL API looks integrated. If every object has its own "animation start" method with different parameters we do not look very integrated.

A couple more comments.

src/lib/elementary/efl_ui_animation_view.eo
78

Why do you need this property if you already implement Efl.Player.playback_position ?

142–143

This name is confusing ("play back" does not mean "play backwards"). I would remove this property since playback_speed is already available.

jsuya updated this revision to Diff 27644.Dec 18 2019, 9:57 PM
jsuya marked 9 inline comments as done.

update code and remove is_playing_back

jsuya marked an inline comment as done.Dec 18 2019, 9:57 PM
jsuya added inline comments.
src/lib/elementary/efl_ui_animation_view.c
715

I deleted this line

776

I supported negative speed

788

we supports cases with speed is zero

src/lib/elementary/efl_ui_animation_view.eo
50

I replaced auto_repeat with autorepeat.

78

Because unit of position is seconds.
I know that user can use duration to get the position they want with a progress value between 0 and 1.
But I think it should provide a way to set progress value.

The Efl.Player.playback_progress property does not have set defined.
This property can be cleared and implemented interface's set property when a set of Efl.Player.playback_progress is defined.

105

In D10870 we left the beta mark for this function.
This function has no plans to release yet. Because we can think of a better way as you think.
This feature is functionally equivalent to playing_set. So we needs to be modified.
If we have plan modify this we will create a another patch.

And..

The idea is that the whole EFL API looks integrated. If every object has its own "animation start" method with different parameters we do not look very integrated.

Efl.Canvas.Object_Animation.animation_start has parameters for speed and starting progress when starting the animation.
This is a recently created class that has although not released yet.
Why didn't this class keep that?

I think too "Looks integrated" is important .
but the naming about "playing(or play) + sector (or something)" is caused by an EO not supported 'overloading' by c.
Probably if EO supported overloading, it would be playing ()/playing (A, B).
Sometimes I think this may seem more intuitive from the point of view of C code(or c++?).

anyways...
Since Vg.object implements a sector set, we can follow that approach (.playing_sector(A, B) -> .sector_set(A, B) / .playing_set())
or we provide marker_get function(marker is sector's name), and the min and max frames could be configured by the user.

123–124

I deleted this property

142–143

We can remove this property because user can use the sign of the speed value as you say.
But I don't know if it's good for the user to do everything...

jsuya updated this revision to Diff 27645.Dec 18 2019, 10:02 PM
jsuya marked an inline comment as done.
jsuya edited the summary of this revision. (Show Details)

update comment

segfaultxavi requested changes to this revision.Dec 19 2019, 12:40 AM

Nice work! I like this API more and more with every iteration!
Just some more finishing touches.

src/lib/elementary/efl_ui_animation_view.eo
78

I much MUCH prefer that we define the missing setter for Efl.Player.playback_progress.

Otherwise we will have playback_progress (read-only) and progress (write-only). This is not nice API design.

105

OK then, let's keep it beta.

142–143

I don't think there's much difference for the user between:

if (obj.is_playing_back) { ... }

and

if (obj.playback_speed < 0) { ... }
This revision now requires changes to proceed.Dec 19 2019, 12:40 AM
jsuya marked 7 inline comments as done.Thu, Dec 19, 9:36 PM
jsuya added inline comments.
src/lib/elementary/efl_ui_animation_view.eo
78

I made a patch that add setter of playerback_progress. (D10937)

jsuya marked an inline comment as done.Thu, Dec 19, 9:37 PM
jsuya added inline comments.
src/lib/elementary/efl_ui_animation_view.eo
78

sorry D10931

segfaultxavi accepted this revision.Fri, Dec 20, 3:07 AM

OK! Then I have no further concerns regarding the API. I'll let somebody else review the code.

jsuya added a comment.Mon, Dec 23, 8:28 PM

Hi @bu5hm4n
Please check the comments you reviewed.

bu5hm4n resigned from this revision.Tue, Dec 24, 12:24 AM

I think xavi did more here than me, so I will just idle out. My concerns have been met.

This revision is now accepted and ready to land.Tue, Dec 24, 12:24 AM
zmike requested changes to this revision.Mon, Dec 30, 8:02 AM
zmike added a subscriber: zmike.

One minor change that I can see, otherwise I think this is pretty sensible now.

Do we have (or is anyone working on) unit tests for this API?

src/lib/elementary/efl_ui_animation_view.eo
7

My understanding is that this should be play_reverse now?

This revision now requires changes to proceed.Mon, Dec 30, 8:02 AM
jsuya marked an inline comment as done.Mon, Dec 30, 11:12 PM
jsuya added inline comments.
src/lib/elementary/efl_ui_animation_view.eo
7

This will be changed to playing_backwards in patch D10915.

zmike accepted this revision.Tue, Dec 31, 6:49 AM
zmike added inline comments.
src/lib/elementary/efl_ui_animation_view.eo
7

Whoops, a bit hard to keep it all in mind across the patches.

This revision is now accepted and ready to land.Tue, Dec 31, 6:49 AM
This revision was automatically updated to reflect the committed changes.