Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lily/Video] add m3u8 support #1785

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

[Lily/Video] add m3u8 support #1785

wants to merge 12 commits into from

Conversation

jexjws
Copy link

@jexjws jexjws commented Dec 11, 2024

  1. using hls.js to play m3u8
  2. bundled hls.js directly into the extension

Copy link
Author

@jexjws jexjws Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (!(await Scratch.canFetch(url))) return;

WARNING : If the resource URLs in a .m3u8 file are not on the same domain as the .m3u8 file itself, the client can fetch resources from other domains without using Scratch.fetch.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you can do a check using a custom loader for hls.js before a playlist or fragment gets loaded. This is a quick mock up but haven't tested it at all:

this.hls = new Hls({
  loader: class CustomLoader extends Hls.DefaultConfig.loader {
    load(context, config, callbacks) {
      let { type, url } = context;

      // Custom behavior - do a check first
      Scratch.canFetch(url).then(isAllowed => {
        if (!isAllowed) {
          callbacks.onErrorCallback({
            code: 'Scratch_Error',
            text: `${err}`
          });
        } else {
          super.load(context, config, callbacks);
        }
      }).catch(err => callbacks.onErrorCallback({
        code: 'Scratch_Error',
        text: `${err}`
      }));
    }
  }
});

@CubesterYT
Copy link
Member

If you use an outside library, you must make an offline copy of it and imbibe it into the extension directly, and include the license of said library in the extension as well.

@jexjws
Copy link
Author

jexjws commented Dec 12, 2024

make an offline copy of it and imbibe it into the extension directly

  1. Why would we bundle hls.js directly into the extension? It seems unnecessary to add a 400KB overhead for handling non-M3U8 videos. Is this intended for offline usage?

  2. if we do this, when the upstream project releases important updates (such as security updates), we will need to manually update the hls.js within our repository. Manual updates can cause delays. Is there a way to solve this problem (for example, by having Dependabot manage the hls.js within extensions repository? I'm not familiar with Dependabot and wouldn't know how to set that up.)

and include the license of said library in the extension as well.

OK. I will do this.

Add hls.js license and usage information
@LilyMakesThings
Copy link
Contributor

  1. It's intended for offline usage and in the unlikely event it becomes inaccessible for whatever reason.
  2. If it works, what reason would there be to update in a timely manner? You'd be amazed how many dependencies for Scratch and by extension TurboWarp have not been updated in half a decade.

@jexjws
Copy link
Author

jexjws commented Dec 13, 2024

Addressed. Please review the changes.

@yuri-kiss
Copy link
Member

I have the same question as Lily.

what reason would there be to update in a timely manner

As well as,

If the browser cannot play the video file natively, adding a library just adds overhead.
Which also gives a sort of precedence to adding library's to support other video formats.

That can be costly as most users will never need to play video formats that the browser does not support;
Leaving a large file and unused library(s) in the extension.

@yuri-kiss yuri-kiss added the pr: change existing extension Pull requests that change an existing extension label Dec 24, 2024
@jexjws
Copy link
Author

jexjws commented Dec 26, 2024

I've made a table describing the differences between each decision (we're temporarily setting aside the issue of native browser m3u8 loading violating TurboWarp's Scratch.fetch security model):

Decision Advantages Disadvantages
Embedding hls.js in Video.js Allows for offline m3u8 playback Increases size, makes future editing harder
Dynamically loading hls.js with Video.js Loads only when needed, no unnecessary overhead Requires clients to be able to connect to cdn.jsdelivr.net to play m3u8

If I had to pick one, I'd choose "Dynamically loading hls.js with Video.js", but that comes at the cost of offline m3u8 playback for users (e.g., using localhost).

But we need a solution that works for everyone. Giving Scratchers a consistent experience whether they're online or offline is important, but keeping future edits simple is also important for the extension's continued development.

I believe that the need for external JavaScript libraries is common for TurboWarp extensions (e.g., a math extension may require an external math-related library), and I even think that neither embedding nor dynamically loading third-party JavaScript libraries as presented in the table is the optimal solution.

@yuri-kiss
Copy link
Member

yuri-kiss commented Dec 26, 2024 via email

@CST1229
Copy link
Collaborator

CST1229 commented Dec 26, 2024

I even think that neither embedding nor dynamically loading third-party JavaScript libraries as presented in the table is the optimal solution.

One solution would be to host these libraries on extensions.turbowarp.org. This way, the desktop app would be able to load these offline (since it already has a full copy of extensions.turbowarp.org that it uses for everything else related to these extensions).
As for the packager, maybe the extension could also specify the dependency URLs somewhere (maybe as a header comment) so it can download them at package-time like the extension itself.

@jexjws
Copy link
Author

jexjws commented Dec 26, 2024

That can be costly as most users will never need to play video formats that the browser does not support

This PR exists because a friend of mine wanted to play m3u8 in turbowarp (although he eventually achieved his goal by putting a webpage that includes hls.js into the iframe extension).

Leaving a large file and unused library(s) in the extension.

Please check my commits before I was asked to make the extension usable offline. Bundling hls.js within the extension was solely to meet the requirement of offline usability.

@yuri-kiss
Copy link
Member

yuri-kiss commented Dec 26, 2024 via email

@yuri-kiss
Copy link
Member

you still have no addressed part 2 of what lily said from what I can tell

If it works, what reason would there be to update in a timely manner? You'd be amazed how many dependencies for Scratch and by extension TurboWarp have not been updated in half a decade.

CST has done something to address the issue but its out of scope for this PR unless it actually gets implemented

@GarboMuffin
Copy link
Member

i know the dependency situation is bad. some ideas floating around but nothing concrete yet.

not only does the desktop need to get the extensions offline, files generated by the packager also need to embed all the extensions and their resources so they work offline. right now that is a very simple fetch() without an analysis step. at the same time i want to preserve the property that people can copy and paste an extension's unprocessed code from github and get something that works.

sure lots of the rest of this project uses ancient dependencies but that does not mean we should stop caring for the other parts. notice that this repository is pretty up-to-date.

@yuri-kiss
Copy link
Member

yuri-kiss commented Dec 26, 2024

i know the dependency situation is bad. some ideas floating around but nothing concrete yet.

not only does the desktop need to get the extensions offline, files generated by the packager also need to embed all the extensions and their resources so they work offline. right now that is a very simple fetch() without an analysis step. at the same time i want to preserve the property that people can copy and paste an extension's unprocessed code from github and get something that works.

sure lots of the rest of this project uses ancient dependencies but that does not mean we should stop caring for the other parts. notice that this repository is pretty up-to-date.

would it be better if this pr was just marked as draft ?

@jexjws
Copy link
Author

jexjws commented Dec 26, 2024

you still have no addressed part 2 of what lily said from what I can tell

If it works, what reason would there be to update in a timely manner? You'd be amazed how many dependencies for Scratch and by extension TurboWarp have not been updated in half a decade.

Because I felt this wasn't the focus of my PR, and I don't agree with this point, I remained silent on this issue and presumptuously assumed you wouldn't care.

My silence clearly hurt you, and I apologize for that.

I should have opened a new issue to discuss versioning problems unrelated to the PR, instead of including issues unrelated to this PR within the PR itself.

@jexjws jexjws closed this Dec 26, 2024
@jexjws
Copy link
Author

jexjws commented Dec 26, 2024

A majority of people will never need this, 1 person doesnt overrule majority when it comes to this.

There's at least 1 person, not just 1. m3u8 is not an unpopular format; many websites use m3u8 to provide video services.

@FurryR
Copy link
Contributor

FurryR commented Dec 27, 2024

@yuri-kiss @CubesterYT @jexjws
This SHOULDN'T be closed right now as we now have a new solution -- see #1591 and its related issue.

@CubesterYT
Copy link
Member

Let's keep this open, it's a really good addition to the extension.

@CubesterYT CubesterYT reopened this Dec 27, 2024
@yuri-kiss
Copy link
Member

yuri-kiss commented Dec 27, 2024

Let's keep this open, it's a really good addition to the extension.

If it was closed it was closed for a reason, I'm going to mark it as draft until the dependency situation gets better, just so it stays open.

@yuri-kiss yuri-kiss marked this pull request as draft December 27, 2024 14:31
@LilyMakesThings
Copy link
Contributor

Let's keep this open, it's a really good addition to the extension.

I strongly disagree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: change existing extension Pull requests that change an existing extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants