feat: add downloadAlternateFFmpeg option #7247

baparham · 2022-11-08T15:21:06Z

This is a possible solution to the question proposed in #7210 addressing the need/desire to have an automatic FFmpeg download as part of downloading the standard electron distributable.

changeset-bot · 2022-11-08T15:21:09Z

🦋 Changeset detected

Latest commit: 4dcd9ff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages

Name	Type
app-builder-lib	Minor
dmg-builder	Minor
electron-builder-squirrel-windows	Minor
electron-builder	Minor
electron-forge-maker-appimage	Minor
electron-forge-maker-nsis-web	Minor
electron-forge-maker-nsis	Minor
electron-forge-maker-snap	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2022-11-08T15:21:11Z

✅ Deploy Preview for car-park-attendant-cleat-11576 ready!

Name	Link
🔨 Latest commit	`4dcd9ff`
🔍 Latest deploy log	https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/636a73e4578a310009eb0594
😎 Deploy Preview	https://deploy-preview-7247--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

mmaietta · 2022-11-10T16:25:20Z

packages/app-builder-lib/src/configuration.ts

+  /**
+   * Whether to download the alternate FFmpeg library from Electron's release assets and replace the default FFmpeg library prior to signing
+   */
+  readonly downloadAlternateFFmpeg?: boolean


What are your thoughts on renaming this to: injectElectronFFmpeg or something similar? downloadAlternate seems odd to me because it can also use ELECTRON_BUILDER_CACHE and this seems to be more Electron-specific than just a generic alternate library.

yeah, a name for this is kind of hard. It's also not just downloading it is really using the alternate ffmpeg library. I was also hesitant to call it nonProprietary since I don't really have a guarantee from electron that it is non proprietary...we as consumers are supposed to be the ones checking this sort of thing. Granted, it's built with the proprietary_codecs=false flag from chromium.

Anyway, all Electron's come with this ffmpeg library, its just that we want to use an alternative one that supposedly has only non proprietary codecs in it. With that, I'd say the best name maybe is useNonProprietaryFFmpeg ... that seems to be as descriptive and accurate as it gets.

mmaietta · 2022-11-10T16:26:36Z

packages/app-builder-lib/src/electron/ElectronFramework.ts

+    if (packager.config.downloadAlternateFFmpeg) {
+      log.info("downloading and unpacking alternate FFmpeg")
+      const ffmpegOptions = Object.assign({}, options)
+      ffmpegOptions.customFilename = `ffmpeg-v${options.version}-${options.platform}-${options.arch}.zip`


Looking at the logic above for const zipFile = electron-v${options.version}-${platformName}-${options.arch}.zip``, I think we can use platformName instead of `options.platform`? That way we're labeling for sure the same download file?

the naming logic here mirrors the unpack-electron options (since the full options object is just passed to the app-builder-bin call), since this case is only handled when we are downloading the default electron distributable (i.e. dist === null). The zipFile reference above is referencing something the user can manually override and specify custom platforms for, and we won't run this ffmpeg option in such a case. The user is expected to have the dlls in that electron distributable the way they want them.

mmaietta · 2022-11-10T16:28:14Z

packages/app-builder-lib/src/electron/ElectronFramework.ts

+      log.info("downloading and unpacking alternate FFmpeg")
+      const ffmpegOptions = Object.assign({}, options)
+      ffmpegOptions.customFilename = `ffmpeg-v${options.version}-${options.platform}-${options.arch}.zip`
+      let ffmpegLocation = path.join(appOutDir, "..")


Would path.dirname(appOutDir) work better here? Either that or path.resolve?

yeah, probably best to just stick to path.dirname(appOutDir) to avoid the ... I'll fix.

mmaietta · 2022-11-10T16:30:10Z

packages/app-builder-lib/src/electron/ElectronFramework.ts

+      if (!isEmptyOrSpaces(process.env.ELECTRON_BUILDER_CACHE)) {
+        ffmpegLocation = process.env.ELECTRON_BUILDER_CACHE
+      }
+      ffmpegOptions.cache = ffmpegLocation


Looking at the logic for options.cache = resolvedDist, I think the options.cache is supposed to be an absolute path to the actual file (as opposed to a folder, which is what I'm under the impression the BUILDER_CACHE should be)

Hmm, I was basing this off of the app-builder arguments, where it expects the cache property in the config to be a directory.

[edit: I'm bummed these permalinks below didn't auto render as code snippets, so apologies for making you jump over to a new tab to see what I am referring to]

https://github.com/develar/app-builder/blob/4e2aa6a12e2bc3d31ec0d01d661fb3a4d65248ff/pkg/electron/electronDownloader.go#L20

and

https://github.com/develar/app-builder/blob/4e2aa6a12e2bc3d31ec0d01d661fb3a4d65248ff/pkg/electron/electronDownloader.go#L62

However, I mistakenly read the ELECTRON_CACHE as ELECTRON_BUILDER_CACHE and defaulted to that if it were explicitly specified other than the default [edit: i.e. path.dirname(appOutDir)] to take advantage of caching this ffmpeg download between runs if users had specified the ELECTRON_CACHE folder.

I was really hoping to use a getCacheFolder method that I could call and not have to specify any specific directory, since we a) probably want to keep this download cached like we do electron distributables and b) we need to know where it ends up after downloading so that we can unzip it.

Perhaps the caching is a premature optimization and it's easiest and cleanest to just keep it up in path.dirname(appOutDir) for now without the caching logic, since this zip is ever only around 1mb.

Gotcha gotcha. Yes, great callout as well, I think the caching is an unnecessary optimization for now when it's that is only 1mb.

Just to double check, is this working in your personal/test project?
Have you given any thought on how we could integration test this? I'll continue thinking from my end. Is there a way for us to detect whether a new FFmpeg was downloaded or not? As we do have a post-packaging hook for verifying app contents.

mmaietta · 2022-11-10T16:33:18Z

packages/app-builder-lib/scheme.json

@@ -6416,6 +6416,11 @@
      ],
      "description": "macOS DMG options."
    },
+    "downloadAlternateFFmpeg": {


For the schema, we'll also need to run pnpm generate-all to update the docs+schema together

aha, ok, thanks for the tip. I'll run that and submit another commit with the updates.

baparham

Thanks for the feedback! I've answered your questions though it seems like there's still some more discussion needed before I push anything back up to the branch, so I'll hold off until I hear your thoughts.

baparham · 2022-11-10T20:07:21Z

packages/app-builder-lib/scheme.json

@@ -6416,6 +6416,11 @@
      ],
      "description": "macOS DMG options."
    },
+    "downloadAlternateFFmpeg": {


aha, ok, thanks for the tip. I'll run that and submit another commit with the updates.

baparham · 2022-11-10T20:13:09Z

packages/app-builder-lib/src/configuration.ts

+  /**
+   * Whether to download the alternate FFmpeg library from Electron's release assets and replace the default FFmpeg library prior to signing
+   */
+  readonly downloadAlternateFFmpeg?: boolean


yeah, a name for this is kind of hard. It's also not just downloading it is really using the alternate ffmpeg library. I was also hesitant to call it nonProprietary since I don't really have a guarantee from electron that it is non proprietary...we as consumers are supposed to be the ones checking this sort of thing. Granted, it's built with the proprietary_codecs=false flag from chromium.

Anyway, all Electron's come with this ffmpeg library, its just that we want to use an alternative one that supposedly has only non proprietary codecs in it. With that, I'd say the best name maybe is useNonProprietaryFFmpeg ... that seems to be as descriptive and accurate as it gets.

baparham · 2022-11-10T20:15:56Z

packages/app-builder-lib/src/electron/ElectronFramework.ts

+    if (packager.config.downloadAlternateFFmpeg) {
+      log.info("downloading and unpacking alternate FFmpeg")
+      const ffmpegOptions = Object.assign({}, options)
+      ffmpegOptions.customFilename = `ffmpeg-v${options.version}-${options.platform}-${options.arch}.zip`


the naming logic here mirrors the unpack-electron options (since the full options object is just passed to the app-builder-bin call), since this case is only handled when we are downloading the default electron distributable (i.e. dist === null). The zipFile reference above is referencing something the user can manually override and specify custom platforms for, and we won't run this ffmpeg option in such a case. The user is expected to have the dlls in that electron distributable the way they want them.

baparham · 2022-11-10T20:16:32Z

packages/app-builder-lib/src/electron/ElectronFramework.ts

+      log.info("downloading and unpacking alternate FFmpeg")
+      const ffmpegOptions = Object.assign({}, options)
+      ffmpegOptions.customFilename = `ffmpeg-v${options.version}-${options.platform}-${options.arch}.zip`
+      let ffmpegLocation = path.join(appOutDir, "..")


yeah, probably best to just stick to path.dirname(appOutDir) to avoid the ... I'll fix.

baparham · 2022-11-10T20:24:30Z

packages/app-builder-lib/src/electron/ElectronFramework.ts

+      if (!isEmptyOrSpaces(process.env.ELECTRON_BUILDER_CACHE)) {
+        ffmpegLocation = process.env.ELECTRON_BUILDER_CACHE
+      }
+      ffmpegOptions.cache = ffmpegLocation


Hmm, I was basing this off of the app-builder arguments, where it expects the cache property in the config to be a directory.

[edit: I'm bummed these permalinks below didn't auto render as code snippets, so apologies for making you jump over to a new tab to see what I am referring to]

https://github.com/develar/app-builder/blob/4e2aa6a12e2bc3d31ec0d01d661fb3a4d65248ff/pkg/electron/electronDownloader.go#L20

and

https://github.com/develar/app-builder/blob/4e2aa6a12e2bc3d31ec0d01d661fb3a4d65248ff/pkg/electron/electronDownloader.go#L62

However, I mistakenly read the ELECTRON_CACHE as ELECTRON_BUILDER_CACHE and defaulted to that if it were explicitly specified other than the default [edit: i.e. path.dirname(appOutDir)] to take advantage of caching this ffmpeg download between runs if users had specified the ELECTRON_CACHE folder.

I was really hoping to use a getCacheFolder method that I could call and not have to specify any specific directory, since we a) probably want to keep this download cached like we do electron distributables and b) we need to know where it ends up after downloading so that we can unzip it.

Perhaps the caching is a premature optimization and it's easiest and cleanest to just keep it up in path.dirname(appOutDir) for now without the caching logic, since this zip is ever only around 1mb.

baparham · 2022-12-01T22:58:10Z

@mmaietta are you waiting on me to do anything else?

baparham · 2022-12-01T22:58:53Z

@mmaietta are you waiting on me to do anything else?

ah, sorry, I see some things I said I would fix. My mistake.

calisven · 2023-01-23T18:54:49Z

@mmaietta are you waiting on me to do anything else?

ah, sorry, I see some things I said I would fix. My mistake.

@baparham Hello, is there any update on when this will be released? Appreciate your and the team's great work!

baparham · 2023-01-23T18:59:15Z

Hey, been in holiday mode and then sick. Hopefully I can get back on it and get this merged in within a few weeks

baparham · 2023-02-17T20:26:13Z

Hey, been in holiday mode and then sick. Hopefully I can get back on it and get this merged in within a few weeks

Still on my to-do list, but I haven't forgotten! Thanks for the patience

mmaietta · 2023-03-15T02:04:29Z

Hey @baparham ! During some other research, I came across this npm package for handling non-proprietary ffmpeg (https://www.npmjs.com/package/electron-packager-plugin-non-proprietary-codecs-ffmpeg)

I gave it a shot. Would you be willing to test it after merge? I don't know how to set up a test for this PR #7477

baparham · 2023-03-15T09:17:25Z

The only test I've come up with is doing like an objdump or dumpbin.exe on windows to look for some specific strings like x264 or aac but even that is subjective, since they are still defined, there are just many fewer occurrences of that string when proprietary_codecs=false, it makes for a somewhat fragile test, but it's something.

mmaietta · 2023-03-19T17:14:51Z

Closing. Added functionality via #7477 :)

feat: add downloadAlternateFFmpeg option

4dcd9ff

github-actions bot added the app-builder-lib label Nov 8, 2022

baparham mentioned this pull request Nov 9, 2022

Question: Is there value putting together a PR for non-proprietary FFmpeg download? #7210

Closed

mmaietta reviewed Nov 10, 2022

View reviewed changes

baparham commented Nov 10, 2022

View reviewed changes

mmaietta mentioned this pull request Mar 15, 2023

feat: Adding new downloadAlternateFFmpeg option to download non-proprietary ffmpeg library #7477

Merged

baparham mentioned this pull request Mar 15, 2023

Feature Request: non-patented FFMPEG Option electron/packager#270

Closed

mmaietta closed this Mar 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add downloadAlternateFFmpeg option #7247

feat: add downloadAlternateFFmpeg option #7247

baparham commented Nov 8, 2022

changeset-bot bot commented Nov 8, 2022

netlify bot commented Nov 8, 2022 •

edited

mmaietta Nov 10, 2022

baparham Nov 10, 2022

mmaietta Nov 10, 2022

baparham Nov 10, 2022

mmaietta Nov 10, 2022

baparham Nov 10, 2022

mmaietta Nov 10, 2022 •

edited

baparham Nov 10, 2022 •

edited

mmaietta Nov 15, 2022

mmaietta Nov 10, 2022

baparham Nov 10, 2022

baparham left a comment

baparham Nov 10, 2022

baparham Nov 10, 2022

baparham Nov 10, 2022

baparham Nov 10, 2022

baparham Nov 10, 2022 •

edited

baparham commented Dec 1, 2022

baparham commented Dec 1, 2022

calisven commented Jan 23, 2023

baparham commented Jan 23, 2023

baparham commented Feb 17, 2023

mmaietta commented Mar 15, 2023

baparham commented Mar 15, 2023

mmaietta commented Mar 19, 2023

feat: add downloadAlternateFFmpeg option #7247

feat: add downloadAlternateFFmpeg option #7247

Conversation

baparham commented Nov 8, 2022

changeset-bot bot commented Nov 8, 2022

🦋 Changeset detected

netlify bot commented Nov 8, 2022 • edited

✅ Deploy Preview for car-park-attendant-cleat-11576 ready!

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

mmaietta Nov 10, 2022 • edited

Choose a reason for hiding this comment

baparham Nov 10, 2022 • edited

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

baparham 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

baparham Nov 10, 2022 • edited

Choose a reason for hiding this comment

baparham commented Dec 1, 2022

baparham commented Dec 1, 2022

calisven commented Jan 23, 2023

baparham commented Jan 23, 2023

baparham commented Feb 17, 2023

mmaietta commented Mar 15, 2023

baparham commented Mar 15, 2023

mmaietta commented Mar 19, 2023

netlify bot commented Nov 8, 2022 •

edited

mmaietta Nov 10, 2022 •

edited

baparham Nov 10, 2022 •

edited

baparham Nov 10, 2022 •

edited