[Proposal] Switch from a Manifest class to an IManifest interface

[peaBerberian]

The Manifest object was until now a class describing a content regardless of the streaming protocol (e.g. DASH, Smooth etc.) used:

• Its URL (at which it can be refreshed)
• Its minimum and maximum seekable position
• The description of the available audio/video/text tracks and qualities
• How to request initialization and media segments
• decryption initialization data
• and many other things

It is thus a central part of the RxPlayer.

However, the fact that it is defined as a class despite it doing some processing at instantiation made us encounter several issues over time:

1. No asynchronous operation can happen at object construction.

   This mainly have been problematic recently, as I wanted to use an asynchronous Web API (MediaCapabilities's decodingInfo object) to define a property of a Representation (which is another class inside the Manifest's instance).

   In the end, I may decide to not continue this road (defining the property as instantiation) for various unrelated reasons, but the possible need to define asynchronous properties still stand for the future.

2. At instantiation, no other value (excepted the constructed Manifest object) can be returned.

   This was already problematic as various minor issues can occur while instantiating a Manifest (e.g. a track has no supported codec).

   Due to this, the idea was previously to define a new property of the Manifest, called contentWarnings, which contained all those issues. This was kind of a hack.

3. I remember wanting in the past to have multiple potential constructors, with different arguments given, to unlock advanced features while staying readable.

   This is not really possible with classes, at least not easily and readably.

Those are the reason why this PR is a proposal to define an IManifest interface instead and having regular functions constructing it.
However doing this has also several disadvantages:

• Property names and method signatures are duplicated: once at type definition and the other time in the function that creates it.

  As they are in two separate places, we might also more easily forget to update e.g. the documentation when doing changes.

• tsserver's "go to definition" feature found in most editors is going to favour the interface over the code.

  This can be good as that's where the documentation is, but also bad as that's not where the real logic is.

• it's not possible to call instanceof anymore on it, and the most probable replacement is doing good ol' and ugly duck typing.

  However this was done at only one place in the code (checking if the initialManifest loadVideo option is a Manifest instance).

[jilles-sg]

This defines the methods with a new function object for each IManifest object, which is less efficient: not only does this allocate more function objects, but all call sites that invoke IManifest methods become polymorphic when the second IManifest object is created.

Also note that this approach will make the methods work when called without the proper this, like const g = someManifest.getPeriod; const period = g();, so getting rid of the polymorphic call sites later may be difficult.

Some alternatives:

• Continue to define a Manifest class but only export a function that returns IManifest.
• Define the methods at the top level of the file like function getPeriod(this : IManifest, periodId : string) : IPeriod | undefined and only use this to refer to the IManifest object.

[peaBerberian]

Hi,

not only does this allocate more function objects

Speaking strictly in terms of IManifest, there's usually a single object at a time - at least in the majority of cases where there's only a single video player on screen.

Though you could make the same point for subparts of an IManifest like IPeriod, IAdaptation and so on. Though even here, I 'm unsure that this is a real issue, at least in terms of memory imprint and performance - if that's what you mean.
To test.

all call sites that invoke IManifest methods become polymorphic when the second IManifest object is created

Here, I'm not sure of what you exactly mean (in terms of "what problem does it create").
How I get it is: calling those methods will go, in the JS engine, through some sort of dynamic dispatch to get to the right function whereas having a single external function could unleash performance and memory optimizations inside the JS engine by allowing static dispatch to an uniquely-declared function. Is that it?

If that's so, I'm unsure that this is a real problem in the end, we're not at that level of optimization for now and we favored readability and simplicity (more information on why we do not rely on the alternatives below).

Also note that this approach will make the methods work when called without the proper this

This is taken into account, as there's no reason to use this in that code. In those inner functions, we're neither inside a class nor inside a well-defined JavaScript Object. That's the reason we're relying on manifestObject instead.
So I'm unsure if that's an issue here?

---

For the alternatives:

Continue to define a Manifest class but only export a function that returns IManifest.

The point was to remove the idea of the Manifest being defined as a class to simplify several points: allowing asynchronous construction, complex return values while constructing without impacting the original object, and allowing several constructors. I will also add now: simplify cross-worker communication of the Manifest object.

So here, the fact that the IManifest type exists instead of the Manifest class is more of a (sad) side-effect of what we want to do.

Define the methods at the top level of the file like function getPeriod(this : IManifest, periodId : string) : IPeriod | undefined and only use this to refer to the IManifest object.

We though about some versions of this, but this creates an usability issue when actually calling it:

• either you import and call the function directly at call sites, where it's much less convenient than just relying on methods (it may though be the solution if we ever end up relying on webworker, like we plan to long term)

• either you do things like bind the this inside the createManifestObject function, but there I'm unsure about the readability of the thing compared to what we have here.
  Again, I'm not sure that the performance issue you're noticing are big enough to being taken care of? I may be wrong though. That solution could be taken if we do see an issue with how this is currently developed, of in the future again if we want to do some sort of cross-worker whole manifest communication.

[jilles-sg]

not only does this allocate more function objects

Speaking strictly in terms of IManifest, there's usually a single object at a time - at least in the majority of cases where there's only a single video player on screen.

Though you could make the same point for subparts of an IManifest like IPeriod, IAdaptation and so on. Though even here, I 'm unsure that this is a real issue, at least in terms of memory imprint and performance - if that's what you mean. To test.

all call sites that invoke IManifest methods become polymorphic when the second IManifest object is created

Here, I'm not sure of what you exactly mean (in terms of "what problem does it create"). How I get it is: calling those methods will go, in the JS engine, through some sort of dynamic dispatch to get to the right function whereas having a single external function could unleash performance and memory optimizations inside the JS engine by allowing static dispatch to an uniquely-declared function. Is that it?

Exactly.

If that's so, I'm unsure that this is a real problem in the end, we're not at that level of optimization for now and we favored readability and simplicity (more information on why we do not rely on the alternatives below).

My point is that "JS where methods have one function object per object" is a slower language than "JS where methods are shared between objects of a given type". Replacing a single class with this new mechanism may not have a measurable effect, but doing it in a major way is likely to slow down execution uniformly (that is, the slowdown will not appear as a hot spot).

Callbacks will often be polymorphic or megamorphic, and that's OK. The point is to avoid it where easily possible, in the general way of writing code.

See for example https://www.builder.io/blog/monomorphic-javascript .

Many of the blog posts on Google have the fallacy that fixing hot spots is all you need for good performance, though.

Also note that this approach will make the methods work when called without the proper this

This is taken into account, as there's no reason to use this in that code. In those inner functions, we're neither inside a class nor inside a well-defined JavaScript Object. That's the reason we're relying on manifestObject instead. So I'm unsure if that's an issue here?

My point was that, if such objects are exposed to the outer world and the methods work without this, external code will start to depend on it and it will be hard to go back.

For the alternatives:

Continue to define a Manifest class but only export a function that returns IManifest.

The point was to remove the idea of the Manifest being defined as a class to simplify several points: allowing asynchronous construction, complex return values while constructing without impacting the original object, and allowing several constructors. I will also add now: simplify cross-worker communication of the Manifest object.

To deal with this kind of thing (except cross-worker communication), I've often marked the constructor private and created static factory methods for outside code to create objects. The factory method can be async, can return additional information and there can be multiple of them.

So here, the fact that the IManifest type exists instead of the Manifest class is more of a (sad) side-effect of what we want to do.

Define the methods at the top level of the file like function getPeriod(this : IManifest, periodId : string) : IPeriod | undefined and only use this to refer to the IManifest object.

We though about some versions of this, but this creates an usability issue when actually calling it:

* either you import and call the function directly at call sites, where it's much less convenient than just relying on methods (it may though be the solution if we ever end up relying on webworker, like we plan to long term)

Functions acting on pure data are an option there, but indeed it is harder to find those functions. Unfortunately, a feature like extension methods would be against the goals of TypeScript.

An alternative could be to wrap a class around the pure data that can be transferred across workers.

* either you do things like bind the `this` inside the `createManifestObject` function, but there I'm unsure about the readability of the thing compared to what we have here.

What I was thinking of was to assign the function (which internally uses this to access the IManifest) directly, so just like with a class it requires a proper this to be passed and is the same for all IManifest objects. The function would not be exported directly.

Function.prototype.bind tends to be slow both when creating the bound function and when calling it, especially on older devices, so I would not recommend that.

  Again, I'm not sure that the performance issue you're noticing are big enough to being taken care of? I may be wrong though. That solution could be taken if we do see an issue with how this is currently developed, of in the future again if we want to do some sort of cross-worker whole manifest communication.

I must admit I have not measured this ;-)

[peaBerberian]

OK I understand why that may be an issue (instinctively I would fear more the memory build-up of having a lot of completely different closures for each Representation of a content than the potential perf issues when calling those - at least in the way the RxPlayer is currently written).

In this PR - that probably won't get merged as it is outdated anyway (though is kept alive as a reference) - I was trying to see what a Manifest object created by a function instead of by a class would look like, for the aforementioned reasons. Through the few tests performed, I did not see any sensible performance or memory issues - though we're lately experimenting with much larger Manifests due to ad-switched live contents for example, even sometimes with several codecs, language and accessibility choices, so it's possible that it may become a real issue in the future.

To deal with this kind of thing (except cross-worker communication), I've often marked the constructor private and created static factory methods for outside code to create objects. The factory method can be async, can return additional information and there can be multiple of them.

Here we cannot do this because it's the the instantiation of a sub-class (the Representation which is itself in the Adaptation class, itself in the Period class, itself in the Manifest class) whose construction might become asynchronous - so even the Manifest's has to become asynchronous under that architecture - which is not possible.

Though we have other tricks up our sleeves - such as starting the asynchronous task at construction and reflect its advancement through one of those Representation's property or just perform the asynchronous task elsewhere later. All of them will keep construction synchronous.

What I was thinking of was to assign the function (which internally uses this to access the IManifest) directly, so just like with a class it requires a proper this to be passed and is the same for all IManifest objects. The function would not be exported directly.

I see. Why not if we ever actually continue down this path.

I did not play enough with TypeScript to know how to communicate what this has to be - nor if it can enforce it - I'll check.

---

Note that for now, I just keep the class format until it appears very un-adapted. Then we'll see.

For example, I recently worked-around the issue of a constructor not being able to return other values (the idea was to recuperate warnings seen at construction) by just declaring a warnings parameter on the constructor (which as to be set as an array - then filled by warnings seen at constructions) in a recent PR.
Though this isn't idiomatic much, but that's only a minor inconvenience for now.

For the asynchronous part, I was mainly thinking about using the MediaCapabilities API at construction to report a better support analysis of each Representation (a.k.a. media qualities) seen in the Manifest. As the main method here returns a Promise.
Though I'm still PoCing that thing and there's multiple possible solutions that could still let us rely on a class.

Lastly for the worker part, the issue is that we need some form of the Manifest in both the main and worker JS realms. But worker <-> main communication is mostly done by postMessage calls until now. For various reasons, we cannot communicates the full class (more specifically its methods) through that mean, only serializable properties.

Though there's also other differences we want to keep here (for example, the main thread doesn't need to know the list of available segments whereas the worker does) and we might also want to limit properties mutations (as the effect of the mutation wouldn't be automatically "seen" on the other side), so we still need to experiment a lot here.

[peaBerberian]

Closing this one. For the WebWorker pending work, we now have just an object respecting IManifestMetadata interface sent to the main thread, which is a subset of a Manifest, and we rely on external functions to exploit it.

Having to rely on separate utils function is still a little awkward for developer experience, compared to easily-discoverable methods, but I'm confident that it's a good base on which we'll be able to build better solutions.

For asynchronous tasks, we also encountered it due to async codec-checking when in a WebWorker without MSE. Here the idea was to make the corresponding isSupported's Representation property initialized to undefined, and set only once it is known. Another solution could be to set such "late properties" as promises or similar async constructs.