polimediaupv / paella-core

Paella Player core library
Educational Community License v2.0
20 stars 15 forks source link

Re-introduce ability to change quality for progressive download sources (e.g. mp4 files) #352

Closed LukasKalbertodt closed 6 months ago

LukasKalbertodt commented 7 months ago

Closes #342

I think I managed to get this highly requested feature working. This was tested in many different browsers and devices and everything seems to work as expected. You can test this PR here (try the videos from "Mixed Test Series").

Unfortunately, this cannot be achieved by just having a separate video format plugin: the StreamProvider has to coordinate pausing and resuming. That cannot be done by each video format individually. So I was forced to touch paella-core code anyway. See the commits for details on how this was solved. I think this works robustly enough to be integrated into paella-core so that all users can just benefit from this patch.

The last commit in this PR changes the way the initial quality is selected. See the commit message for more information.

I am aware that the UX could be improved still by showing a spinner while the new quality is loading, but this PR focuses on the functionality. The spinner can easily be added later, if requested.

snoesberger commented 7 months ago

Thanks for your work, Lukas! This is a really important fix that will allow us to easily migrate to Paella 7 without having to convert all of our 75000+ videos to adaptive HLS. For us at the University of Bern, this fix is very much appreciated. I have tested this PR with different operating systems (MacOS, iOS, Windows 11, Android) and browsers (Chrome, Firefox, Safari, Opera) and it worked fine. I haven't found any problems.

LukasKalbertodt commented 7 months ago

Since the same Playwright tests fail on the main branch right now, I assume that my PR is not responsible for those failures.

ferserc1 commented 7 months ago

My problem with the mp4 quality change, and the reason why it was not added in this version of paella-core is that I have had it working several times over the years, but in each new version of iOS, Safari would introduce some change that caused it to stop working properly.

I don't want to introduce this feature in paella-core because it is my responsibility to maintain the code that is introduced, and therefore I don't want to have to take over the maintenance of a plugin that, in addition to this, is nothing more than a hack that is not officially supported by any browser.

If you need me to add the qualityChangeNeedsPause API in the video plugins I can add it, but in no case I will reintroduce the ability to change the video on the fly in mp4.

I suggest you create your own repository to add the plugin and publish it on your own. You have a complete tutorial on how to create a plugin repository for paella-core:

https://paellaplayer.upv.es/#/doc/plugin_module_tutorial.md

ferserc1 commented 7 months ago

Since the same Playwright tests fail on the main branch right now, I assume that my PR is not responsible for those failures.

I have a lot of problems with Playwright testing. Sometimes it fails, I execute them again and it works without changing anything

miesgre commented 7 months ago

Hi @LukasKalbertodt

We are not comfortable with this functionality in the core directly for the reasons expressed by @ferserc1.

Could you make a PR with the necessary changes in StreamProvider.js? So, you could make a separate video format plugin.

LukasKalbertodt commented 7 months ago

[...] is nothing more than a hack that is not officially supported by any browser.

I don't think this is correct. Changing the src of a video element is something that is covered by the spec. See this section, in particular:

If a src attribute of a media element is set or changed, the user agent must invoke the media element's media element load algorithm.

My understanding of the spec is that this is indeed intended behavior that web apps can take advantage of. And indeed many do. Just to throw one example out there, with Plyr I can also change the quality on my iPhone. And that's also using mp4 files, not HLS.

[...] it is my responsibility to maintain the code that is introduced.

I'm also maintaining several pieces of software and I can totally understand this concern, really. On some occasion, we talked about someone else taking over the maintenance of that part of the code, i.e. if it breaks in newer Safari versions, someone else from the Opencast community would fix it, without you having to do anything. Is that still an option that would be ok for you?


I will answer the remaining points later.

ferserc1 commented 7 months ago

[...] is nothing more than a hack that is not officially supported by any browser.

I don't think this is correct. Changing the src of a video element is something that is covered by the spec. See this section, in particular:

If a src attribute of a media element is set or changed, the user agent must invoke the media element's media element load algorithm.

My understanding of the spec is that this is indeed intended behavior that web apps can take advantage of. And indeed many do. Just to throw one example out there, with Plyr I can also change the quality on my iPhone. And that's also using mp4 files, not HLS.

It is true that the src attribute can be modified and the player must reload the video, but what we are trying to do here is to emulate the behavior of HLS, ie, modify the source, replay the video and jump to the last instant before changing source. To do this the browser has the Media Source Extensions APIs, with which you could implement this with MP4 sources, but that has certain requirements in the encoding. In fact, here we serve m3u8 playlists that are made up of mp4 files, but we still had to re-encode the files to meet the encoding requirements.

I'm not saying it can't be done, in fact I have been maintaining it in previous versions of paella player, my problem is that it is a feature that often fails due to changes in browsers, and also we no longer use this feature in my organization, so it is much more complicated to maintain it.

[...] it is my responsibility to maintain the code that is introduced.

I'm also maintaining several pieces of software and I can totally understand this concern, really. On some occasion, we talked about someone else taking over the maintenance of that part of the code, i.e. if it breaks in newer Safari versions, someone else from the Opencast community would fix it, without you having to do anything. Is that still an option that would be ok for you?

I will answer the remaining points later.

Our policy is to separate the functionalities in different libraries that have different responsibilities, and in fact for paella-core 2.0 we are considering to extract the HLS part to an external plugin library. In the case of the dynamic quality change in mp4 I see that it is very justified to separate that feature in a different library.

In fact, in our internal video repository we used this functionality, but that plugin was included outside paella-core. That repository is not public, but if you are interested I can send you the code of this plugin. We stopped maintaining it because we switched to HLS, since we had complaints from iOS users from time to time, but if you are interested I can pass you the code. Also, to make it work it is not necessary to modify any function of the core. As the setQuality function is asynchronous, it is possible to load the video right there.

About the changes in StreamProvider, I noticed in your pull request that you do a parallel asynchronous loading of the videos in StreamProvider, which is much more efficient than the way it is being done now. The problem is that for a while we were having problems on some devices when changing quality, and it was managed to be corrected by loading the videos in series. It was mostly on iPad, and had to do with browser tab closures due to excess memory consumption. I'm not sure if that was the problem, but loading the videos serially helped. I think with current devices we won't have those issues, but before modifying that part I'd like to test it.

LukasKalbertodt commented 7 months ago

what we are trying to do here is to emulate the behavior of HLS, ie, modify the source, replay the video and jump to the last instant before changing source.

I don't think "emulate" is the appropriate word here. Both, HLS and progressive download with src-changing, try to implement a feature: changing the quality of the video while watching. Doing that was possible long before HLS existed. I fully agree that it works better with HLS, but huge parts of the internet are still based on normal src-changing behavior. That is a supported feature of browsers and won't go away anytime soon. That's all I'm trying to bring across here.

to make it work it is not necessary to modify any function of the core. As the setQuality function is asynchronous, it is possible to load the video right there.

I tried that first and for single stream videos, that works just fine. But for dual stream videos, both streams should be paused until both streams are loaded enough to play. So in my opinion and experience, there needs to be some synchronization between those. StreamProvider seemed like the easiest place to add that synchronization. If you can show me some code how this can be done properly for dual stream videos without changing the StreamProvider, I'd be interested in that.

your pull request that you do a parallel asynchronous loading of the videos [...]

Yes, true. If that really causes problems, it can easily be changed to serially load those.

Our policy is to separate the functionalities in different libraries that have different responsibilities, and in fact for paella-core 2.0 we are considering to extract the HLS part to an external plugin library. In the case of the dynamic quality change in mp4 I see that it is very justified to separate that feature in a different library.

As long as it's still possible to properly change quality for progressive download (see the "only resume both streams when both are loaded" above), I'm fine wiht splitting player functionality in whatever way. I personally still think this feature should be part of default Paella player, given that Paella seemingly wants to be the Opencast player and that a vast number of people in the OC community need this feature. But splitting this off into plugins seems fine. Again, as long as the ability to properly change progressive video quality isn't restricted.

ferserc1 commented 7 months ago

The mp4 player has stopped working in the latest iOS beta update, and I don't know if it happens with the stable version as well. On some videos it has started returning ReadyState=2 instead of 3, and that makes the video get stuck syncing streams. Since the stream sync is always done, even if there is only one video, the problem happens every time.

I have to make some changes that are going to pause the video when reloading the stream always, then I think this functionality makes it unnecessary to add the API.

I will keep you updated, for now I will leave everything else paused, because I would like to have the patch before they release this iOS version for everyone.

github-actions[bot] commented 7 months ago

This pull request has conflicts ☹ Please resolve those so we can review the pull request. Thanks.

LukasKalbertodt commented 6 months ago

Good day! What's the status here now btw? You said you wanted to patch for the new iOS version, but that seems to have already landed? So do I have to do something now or am I still waiting on something?

On some videos it has started returning ReadyState=2 instead of 3, and that makes the video get stuck syncing streams.

I'm sure that causes all kinds of problems but it also reminded me: maybe one should replace the waitForLoaded method that checks the readyState every 100ms with an canplay event handler? Wouldn't that solve a bunch of problems and also get rid of busy polling? But that's just a random idea, I have no idea why waitForLoaded was added in the first place.

I have to make some changes that are going to pause the video when reloading the stream always, then I think this functionality makes it unnecessary to add the API.

Did those changes already make it to the main branch or to a release? Does that mean that changing the quality of HLS streams manually also pauses the stream?

ferserc1 commented 6 months ago

The waitForLoad was implemented because the canplay callback was not appropiate (this implementation is inherited from paella player 6 and is quite old). Here the problem was that calling the callback canplay means that the video can start playing, but it may not start playing immediately. Having multiple videos that have to be synchronized, the callback was not a good method. I suspect that having changed the radyState now the current method does the same as using the callback, but I don't intend to change it right now because I'm quite busy with paella-core 2.0 development. Surely this code will be revised in version 2.

I published the version with the fix for iOS last week. This has nothing to do with the quality change in HLS, because in this case the streams are not modified (the stream quality changed is made using the MediaSourceExtensions API). The problem is that the video never started playing, so it never got to the point of the quality change.

Here is the code of the plugin we use in the UPV video server with support for mp4 quality change. We don't use it anymore, and I haven't tested it for a long time, but it has worked without problems for about two years:

https://gist.github.com/ferserc1/5526f07d9d194e2af651f5d2f5bb50a2

miesgre commented 6 months ago

New MP4 multi-quality plugin: https://github.com/polimediaupv/paella-mp4multiquality-plugin

Thanks @LukasKalbertodt!