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

Windows GUI: Enable 64-bit version #882

Merged
merged 7 commits into from
Jul 2, 2024
Merged

Conversation

cjee21
Copy link
Collaborator

@cjee21 cjee21 commented Jun 22, 2024

Screenshot 2024-06-22 180755
Screenshot 2024-06-22 180653

Requires building 64-bit versions of ZenLib and zlib as well. For these two, no changes required other than enabling 64-bit.

Advantages of 64-bit version:

  • Better performance?
    • 05 seconds 988 milliseconds vs 09 seconds 258 milliseconds on a single folder test
  • Large address aware
  • High Entropy VA / High-entropy 64-bit address space layout randomization (ASLR)

Issues that may be related: #575 #788 #868

@cjee21 cjee21 marked this pull request as draft June 22, 2024 12:07
@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 22, 2024

Need to clean up the project file and change paths to relative...

@cjee21 cjee21 marked this pull request as ready for review June 22, 2024 13:02
@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 22, 2024

Please see the updated first post.

I recommend having 64-bit installer version due to the advantages listed in first post, the fact that most PCs should be 64-bit already and the effect of smaller installed size on 64-bit PCs due to removal of MediaInfo_i386.dll.

@JeromeMartinez
Copy link
Member

Not tested but my understanding is that the 32-bit binaries are no more in the installer, size is smaller so it is good, but the 32-bit related code won't work.
I don't wish to have both architectures in the installer at long term but it could be a good first step for this PR, please update the PR for embedding both architectures, then we'll work on another PR for having a 32-bit only installer and update the main installer for downloading this 32-bit installer when 32-bit is detected.

We'll have to update build scripts in https://github.com/MediaArea/MediaArea-Utils/ , please some patience, it is on our side but we have other things to finish before managing that (including building the GUI twice). It won't be forever but not in June.

Thank you for pushing us in that direction, just that in the cases of EdgeView and 64-bit we are the blockers and we need some time. But don't give up, we are still very happy with you pushing us.

most PCs should be 64-bit

You can not imagine the count of emails I received when we stop WinXP support despite the fact it was end of life for years, I bet the same for 32-bit, actually even worse (we have no stat, sadly).

@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 22, 2024

Not tested but my understanding is that the 32-bit binaries are no more in the installer, size is smaller so it is good, but the 32-bit related code won't work. I don't wish to have both architectures in the installer at long term but it could be a good first step for this PR, please update the PR for embedding both architectures, then we'll work on another PR for having a 32-bit only installer and update the main installer for downloading this 32-bit installer when 32-bit is detected.

This PR makes the installer generate a universal one. It will use only 64-bit binaries on 64-bit PCs. For 32-bit PCs should be still the same as now unless I made a mistake somewhere.
On your side you just need to make sure to build 64-bit versions of ZenLib, zlib and CURL. Then for MediaInfo, just select 64-bit target and build with the updated project file in this PR.

we have no stat, sadly

Found outdated stats: https://blogs.windows.com/windowsexperience/2010/07/08/64-bit-momentum-surges-with-windows-7/

@JeromeMartinez
Copy link
Member

most PCs should be 64-bit

Now I have doubts, looks like that 32-bit Windows is less than 1% so I would think to remove it...

@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 22, 2024

most PCs should be 64-bit

Now I have doubts, looks like that 32-bit Windows is less than 1% so I would think to remove it...

Well the last time I remember seeing a 32-bit Windows PC was several years ago on a low-end small-sized Intel tablet.

@JeromeMartinez
Copy link
Member

unless I made a mistake somewhere.

I have read it again and it seems that my first read was wrong.

On your side you just need to make sure to build 64-bit versions of ZenLib, zlib and CURL. Then for MediaInfo, just select 64-bit target and build with the updated project file in this PR.

We need to find where it is in our scripts and add the right line for building the 64-bit version. Theses scripts are very old so we need to read and understand them again :-D.

@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 22, 2024

We need to find where it is in our scripts and add the right line for building the 64-bit version. Theses scripts are very old so we need to read and understand them again :-D.

Where I think it is after a quick look: https://github.com/MediaArea/MediaArea-Utils/blob/master/build_release/BuildRelease.bat#L285

@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 23, 2024

most PCs should be 64-bit

Now I have doubts, looks like that 32-bit Windows is less than 1% so I would think to remove it...

Less than 0.1% if we look at the 64 vs 32 chart. That link also shows Windows 7 is less than 0.5% in the versions chart.

Here is another source of stats: https://store.steampowered.com/hwsurvey

If we assume these stats accurately represent the actual user base and also assume that all unlisted versions are 32-bit, then the percentage of 32-bit Windows will be around 3%.

Side note: take a look at the Windows 11 share in both sources... it's time for Windows 11 explorer context menu 😉

@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 23, 2024

Corrected path for x64 CURL since I just saw its location in the repos.

Also tested the combined installer in Windows 7 VM. Did not run into any issues and also confirmed it uninstalled cleanly from install directory.

@JeromeMartinez Might want to add check to see if MediaInfo is running during uninstall or add /REBOOTOK switch as currently it appears to complete uninstall but leave behind MediaInfo.exe and the DLL if MediaInfo is running during uninstall.

we'll work on another PR for having a 32-bit only installer

Added as an additional commit. It builds but install/uninstall is not tested.

update the main installer for downloading this 32-bit installer when 32-bit is detected.

This one leave it up to your team since need server URL and I'm not good enough with NSIS to do this.

From another page:

Package size is a hot topic for us, most people don't care at all and download GB of content but some others complain loudly when there is 1 more MB in the main package...

Then you might want to update your 8-year old 7-Zip that I came across at https://github.com/MediaArea/MediaArea-Utils-Binaries/tree/master/Windows/7-Zip. I tried compressing the extracted 7z dev snapshot with my latest 7-Zip using the same command line as your script and it resulted in a slightly smaller file size.

@cjee21 cjee21 force-pushed the 64-bit branch 2 times, most recently from 7a7bdec to c264a6d Compare June 23, 2024 07:54
@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 24, 2024

Improved the installer.

The first change is adding back the option to run MediaInfo after installation has completed. I saw that it was removed "because it is run in admin privileges". It is now run at user privileges.

The second change is regarding this:

@JeromeMartinez Might want to add check to see if MediaInfo is running during uninstall or add /REBOOTOK switch as currently it appears to complete uninstall but leave behind MediaInfo.exe and the DLL if MediaInfo is running during uninstall.

Now if MediaInfo is running or something is using MediaInfo.dll during uninstall, it will now move the in-use files to %TEMP% and schedule them for deletion on next reboot. It will then ask if the user want to reboot immediately or later on completion. The Program Files\MediaInfo folder will be removed cleanly unless there is still something there. If the user defers reboot and re-installs MediaInfo, the new installation will not be affected after reboot since the old files that are scheduled for deletion have been moved/renamed.

If MediaInfo is not running and nothing is using MediaInfo.dll during uninstall, there is no change in behaviour. The Program Files\MediaInfo folder will be removed cleanly unless there is still something there. There will be no reboot prompt.

This is implemented using macros from Library.nsh. The macros can possibly be used to handle installation of all exe and dlls and registration/unregistration/removal of MediaInfo_InfoTip.dll as well but I did not attempt this to keep changes to a minimal to avoid breaking something.

@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 27, 2024

Add 64-bit only installer.

Size comparison (including WebView2 branch changes):

MediaInfo\Release>dir | find "MediaInfo_GUI"
27/06/2024  11:48 AM         9,943,938 MediaInfo_GUI_24.05_Windows.exe
27/06/2024  11:48 AM         4,999,362 MediaInfo_GUI_24.05_Windows_i386.exe
27/06/2024  11:48 AM         5,553,853 MediaInfo_GUI_24.05_Windows_x64.exe

Current version:
7,282,824 MediaInfo_GUI_24.05.1_Windows.exe

Maybe can do something like this if want to minimize download size:
https://firefox-source-docs.mozilla.org/browser/installer/windows/installer/StubInstaller.html

@cjee21 cjee21 force-pushed the 64-bit branch 2 times, most recently from d450ed5 to 8dff815 Compare June 28, 2024 03:08
@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 28, 2024

Re-based on 24.06 while ensuring version numbers in project file are updated. Also reduced number of commits and moved an unrelated commit to another branch where that file was changed over there.

@JeromeMartinez
Copy link
Member

Maybe can do something like this if want to minimize download size:

The Stub installer is also something we are thinking to add while changing to a 64-bit offer so we show 1 small installer only to all download websites without issue with 32-bit machines (there may actually be more than estimated in the wild).

@cjee21 cjee21 force-pushed the 64-bit branch 2 times, most recently from ed0cb24 to 6662470 Compare July 2, 2024 07:20
@cjee21
Copy link
Collaborator Author

cjee21 commented Jul 2, 2024

Rebased and changed 24.05 to 24.06 in installer scripts that I missed.

@JeromeMartinez JeromeMartinez merged commit 2d8829f into MediaArea:master Jul 2, 2024
3 checks passed
@JeromeMartinez
Copy link
Member

JeromeMartinez commented Jul 2, 2024

For reference MediaArea/MediaArea-Utils#297, #905 and #883 were required for having everything ready for our workflow.

@JeromeMartinez
Copy link
Member

dev snapshots

@cjee21 cjee21 deleted the 64-bit branch July 3, 2024 00:42
@cjee21
Copy link
Collaborator Author

cjee21 commented Jul 3, 2024

dev snapshots

This build is not from latest master and still has disappearing menu glitch but it's okay, I found a newer dev snapshot. I will use it until the next version to enjoy the new changes and also as additional testing.

Might want to update the project files for ZenLib and zlib to add 64-bit targets so that people who use IDE to build will not have trouble. They can easily manually add the target themselves though. I didn't expect building from CLI to work without 64-bit target in project file but it does.

@cjee21
Copy link
Collaborator Author

cjee21 commented Jul 3, 2024

First bug discovered. If open a file (the file must be type that has graph view) from explorer context menu then immediately switch from Basic to Graph view, this pops up:
Screenshot 2024-07-03 120730

If switch to HTML view first then to Graph view, it works. Graph is rendered correctly.

If open MediaInfo without opening any files then immediately switch to Graph view no error. If open a file type that does not have Graph view, Graph view page with message loads fine too.

If use my locally built version with WebView2 branch/PR (just replace MediaInfo.exe without changing the other DLLs), cannot reproduce this error at all even when fallback to IE engine. Graph is rendered correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants