fix(FEC-11037): multiple decorator exist after destroy plugin with decorator #544

Yuvalke · 2021-02-23T12:40:06Z

Description of the Changes

Issue: when the plugin has the decorator and the player destroyed the decorator reregister.
Solution: filter out the previous decorator provider and keep the new one only.

CheckLists

changes have been done against master branch, and PR does not conflict
new unit / functional tests have been added (whenever applicable)
test are passing in local environment
Travis tests are passing (or test results are not worse than on master branch :))
Docs have been updated

Issue: when plugin has decorator and player destoryed the decorator reregister. Solution: filter out the previous decorator provider and keep the new one only

dan-ziv · 2021-02-23T13:33:15Z

src/engines/engine-decorator.js

-        EngineDecorator._decoratorProviders.push(decoratorProvider);
-      }
+      EngineDecorator._decoratorProviders = EngineDecorator._decoratorProviders.filter(
+        currentDecoratorProvider => currentDecoratorProvider.constructor.name !== decoratorProvider.constructor.name


Why name and not id?

we're not maintaining id field

OrenMe · 2021-02-23T20:25:24Z

Hi @yuval, not sure I fully understand the issue description

Yuvalke · 2021-02-23T21:08:26Z

@OrenMe every setup register decorator provider, after destroy we setup again so if plugin has decorator it will add new decorator provider for each destroy and step and create decorator for each provider, this change will keep the newest plugin instance - decorator provider and remove the older one

OrenMe · 2021-02-24T07:25:13Z

So sounds like we need to fix the registry mechanism to be by name or some hash instead of function and then we can actually check that a decorator was registered. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Yuval Keidar <[email protected]> Sent: Tuesday, February 23, 2021 11:08:41 PM To: kaltura/playkit-js <[email protected]> Cc: Oren Melamed <[email protected]>; Mention <[email protected]> Subject: Re: [kaltura/playkit-js] fix: multiple decorator exist after destroy plugin with decorator (#544) @OrenMe<https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FOrenMe&data=04%7C01%7C%7C8cd76d47b907438277c408d8d83f335a%7C0c503748de3f4e2597e26819d53a42b6%7C1%7C0%7C637497113249693344%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Ld1a7%2Bxt8FRcWx4LmDQ4npSPTyty22mbFLUTlune8qk%3D&reserved=0> every setup register decorator provider, after destroy we setup again so if plugin has decorator it will add new decorator provider for each destroy and step and create decorator for each provider, this change will keep the newest plugin instance - decorator provider and remove the older one — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fkaltura%2Fplaykit-js%2Fpull%2F544%23issuecomment-784513892&data=04%7C01%7C%7C8cd76d47b907438277c408d8d83f335a%7C0c503748de3f4e2597e26819d53a42b6%7C1%7C0%7C637497113249703332%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=M9FMQqXcA9sUiP3kGGlBVxHmLrQWFoerhxhaYfP2Png%3D&reserved=0>, or unsubscribe<https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABJVOZSO3JHEE6OQBUHDNN3TAQKNTANCNFSM4YCNGBXA&data=04%7C01%7C%7C8cd76d47b907438277c408d8d83f335a%7C0c503748de3f4e2597e26819d53a42b6%7C1%7C0%7C637497113249703332%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=lxTxthO0yBm3sk6h8fXyqTNPUopHERXjlsL4CXrj9Qc%3D&reserved=0>.

Yuvalke · 2021-02-24T07:36:25Z

@OrenMe static hash? name? I think this check is much better

OrenMe · 2021-02-24T07:58:05Z

I think the entire solution we have today is not ok - it was made when plugins were manager by the playkit lib and not relevant for kaltura player managed plugins.
We register the entire plugin instance in order to just get the decorator from the getEngineDecorator method.
Beside the fact we pass the entire plugin which might cause potential memory leaks, if we ever want to get the registered decorators it is impossible with current implementation.

Yuvalke · 2021-02-24T16:06:11Z

@OrenMe I agree, but in our case, we have to fix multiple decorators after destroy.
I didn't understand why it was OK on playkit and doesn't on kaltura?

OrenMe · 2021-02-24T16:11:49Z

The comment about being ok or not was referring to passing plugin instance from Kaltura player to Playkit.
When playkit managed the plugin instance it made sense, now Kaltura player does it so it makes no sense to pass the entire plugin instance to it just in order for it to call the getDecorator method.

… work for change media

OrenMe

General note - most changes are around naming conventions or keeping existing names

src/engines/engine-decorator.js

OrenMe · 2021-02-28T06:58:25Z

src/player.js

+   * @returns {void}
+   * @param {EngineDecoratorGenerator} decoratorGenerator - function to create the decorator
+   */
+  addEngineDecorator(decoratorGenerator: EngineDecoratorGenerator): void {


Suggested change

addEngineDecorator(decoratorGenerator: EngineDecoratorGenerator): void {

registerEngineDecoratorProvider(engineDecoratorProvider: EngineDecoratorProvider): void {

OrenMe · 2021-02-28T06:58:59Z

src/player.js

+   * @param {EngineDecoratorGenerator} decoratorGenerator - function to create the decorator
+   */
+  addEngineDecorator(decoratorGenerator: EngineDecoratorGenerator): void {
+    if (!this._decoratorManager) {


Suggested change

if (!this._decoratorManager) {

if (!this._engineDecoratorManager) {

OrenMe · 2021-02-28T07:05:54Z

src/engines/engine-decorator-manager.js

+class EngineDecoratorManager {
+  _decoratorGenerators: Array<EngineDecoratorGenerator> = [];
+
+  addDecorator(decoratorGenerator: EngineDecoratorGenerator): void {


Suggested change

addDecorator(decoratorGenerator: EngineDecoratorGenerator): void {

register(name: string, engineDecoratorProvider: EngineDecoratorProvider): void {

OrenMe · 2021-02-28T07:06:23Z

src/engines/engine-decorator-manager.js

+  _decoratorGenerators: Array<EngineDecoratorGenerator> = [];
+
+  addDecorator(decoratorGenerator: EngineDecoratorGenerator): void {
+    this._decoratorGenerators.push(decoratorGenerator);


add check to see if it is already provided, and with above addition of name it is easier then ever

OrenMe · 2021-02-28T07:10:07Z

flow-typed/interfaces/engine-decorator-provider.js

-import type {IEngine} from './engine';
-
-declare interface IEngineDecoratorProvider {
-  getEngineDecorator(engine: IEngine, dispatchEventHandler: Function): IEngineDecorator;


this needs to be part of the plugin interface, it is ok to remove it from here but need to make sure we define it for plugins in kaltura-player-js

Sure the change for kaltura player didn't upload yet

OrenMe · 2021-02-28T10:00:03Z

src/engines/engine-decorator-manager.js

+ * @class EngineDecoratorManager
+ */
+class EngineDecoratorManager {
+  _decoratorProviders: MultiMap<string, IEngineDecoratorProvider> = new MultiMap();


this can be a simple map, multi map is were the value is array but we only have one decorator per plugin

OrenMe · 2021-02-28T10:01:57Z

src/player.js

@@ -390,6 +391,12 @@ export default class Player extends FakeEventTarget {
   * @private
   */
  _aspectRatio: ?string;
+  /**
+   * The engine manager.


Suggested change

* The engine manager.

* The engine decorator manager.

OrenMe · 2021-02-28T12:00:27Z

src/engines/engine-decorator-provider.js

+ * @implements {IEngineDecorator}
+ */
+class EngineDecoratorProvider implements IEngineDecoratorProvider {
+  constructor(plugin: IEngineDecoratorProvider) {


this means we are still passing the entire plugin to playkit. lets pass only the decorator provider.

we're passing it but not keeping it

Provider will be getName and getEngineDecorator instead of the plugin itself

fix: multiple decorator exist after destroy plugin with decorator

479c763

Issue: when plugin has decorator and player destoryed the decorator reregister. Solution: filter out the previous decorator provider and keep the new one only

Yuvalke added the Type: Bug label Feb 23, 2021

Yuvalke requested a review from a team February 23, 2021 12:40

Yuvalke self-assigned this Feb 23, 2021

dan-ziv reviewed Feb 23, 2021

View reviewed changes

check the instance

2b91d08

Merge branch 'master' into fix-multiple-decorator-after-destroy

f25d3c4

Yuvalke changed the title ~~fix: multiple decorator exist after destroy plugin with decorator~~ fix(FEC-11037): multiple decorator exist after destroy plugin with decorator Feb 25, 2021

Yuvalke added 6 commits February 25, 2021 11:55

delete the static providers on destroy

cdf8ae8

Add tests

98c8d34

can't remove EngineDecorator._decoratorProviders on destroty it won't…

34eac4a

… work for change media

destroy the _decoratorProviders on player destroy

2ee2df5

Add an API for add engine decorator

2ac29dc

add type for function

cfa9a05

OrenMe suggested changes Feb 28, 2021

View reviewed changes

OrenMe reviewed Feb 28, 2021

View reviewed changes

fix naming and internal structure

f9c650b

Yuvalke requested a review from OrenMe February 28, 2021 08:31

Yuvalke added 2 commits February 28, 2021 10:51

fix tests

9bf8217

change the engine provider

6e5815b

OrenMe reviewed Feb 28, 2021

View reviewed changes

move EngineDecoratorProvider to playkit and change to map

abebf27

OrenMe reviewed Feb 28, 2021

View reviewed changes

Yuvalke added 3 commits February 28, 2021 14:41

fix flow

4a52ead

add logger to trace the decorator manager on logs

c0a3e76

add explicit to string

67e13a2

OrenMe approved these changes Feb 28, 2021

View reviewed changes

Yuvalke merged commit ee11d45 into master Feb 28, 2021

Yuvalke deleted the fix-multiple-decorator-after-destroy branch February 28, 2021 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(FEC-11037): multiple decorator exist after destroy plugin with decorator #544

fix(FEC-11037): multiple decorator exist after destroy plugin with decorator #544

Yuvalke commented Feb 23, 2021

dan-ziv Feb 23, 2021

Yuvalke Feb 23, 2021

OrenMe commented Feb 23, 2021

Yuvalke commented Feb 23, 2021

OrenMe commented Feb 24, 2021 via email

Yuvalke commented Feb 24, 2021

OrenMe commented Feb 24, 2021

Yuvalke commented Feb 24, 2021 •

edited

Loading

OrenMe commented Feb 24, 2021

OrenMe left a comment

OrenMe Feb 28, 2021

OrenMe Feb 28, 2021

OrenMe Feb 28, 2021

OrenMe Feb 28, 2021

OrenMe Feb 28, 2021

Yuvalke Feb 28, 2021

OrenMe Feb 28, 2021

OrenMe Feb 28, 2021

OrenMe Feb 28, 2021

Yuvalke Feb 28, 2021

Yuvalke Feb 28, 2021

	addEngineDecorator(decoratorGenerator: EngineDecoratorGenerator): void {
	registerEngineDecoratorProvider(engineDecoratorProvider: EngineDecoratorProvider): void {

	if (!this._decoratorManager) {
	if (!this._engineDecoratorManager) {

	addDecorator(decoratorGenerator: EngineDecoratorGenerator): void {
	register(name: string, engineDecoratorProvider: EngineDecoratorProvider): void {

fix(FEC-11037): multiple decorator exist after destroy plugin with decorator #544

fix(FEC-11037): multiple decorator exist after destroy plugin with decorator #544

Conversation

Yuvalke commented Feb 23, 2021

Description of the Changes

CheckLists

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OrenMe commented Feb 23, 2021

Yuvalke commented Feb 23, 2021

OrenMe commented Feb 24, 2021 via email

Yuvalke commented Feb 24, 2021

OrenMe commented Feb 24, 2021

Yuvalke commented Feb 24, 2021 • edited Loading

OrenMe commented Feb 24, 2021

OrenMe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuvalke commented Feb 24, 2021 •

edited

Loading