sircharlo / meeting-media-manager

A cross platform app to download and present media (pictures and videos) for congregation meetings of Jehovah’s Witnesses in any language. Features include recurring and custom media management, congregation sync, and media presentation tools for hybrid, in-person or fully remote Zoom meetings.
GNU Affero General Public License v3.0
126 stars 23 forks source link

Checklist before releasing 22.11.0 #627

Closed mtdvlpr closed 1 year ago

mtdvlpr commented 1 year ago

Some things that need to be tested again before releasing:

sircharlo commented 1 year ago

Hm.

Error: CompileError: WebAssembly.instantiate(): expected magic word 00 61 73 6d, found 3c 21 64 6f @+0

image

sircharlo commented 1 year ago

WebAssembly.instantiate(): expected magic word

Perhaps related to https://github.com/WebAssembly/spec/issues/1031

sircharlo commented 1 year ago

Duh. Ran the equivalent prebuild commands.

mtdvlpr commented 1 year ago

I tested Malagasy and image zoom/pan. No issues on my end.

sircharlo commented 1 year ago

Check out my new commit, it improves the display of images when the aspect ratio isn't exactly 16:9.

sircharlo commented 1 year ago

MP3 to MP4 worked fine, ffmpeg download and ran the conversion. The progress bar didn't disappear, but not a major issue IMO.

sircharlo commented 1 year ago

MP3 to MP4 worked fine, ffmpeg download and ran the conversion. The progress bar didn't disappear, but not a major issue IMO.

This is what is displayed after the conversion finishes. No errors in console.

image

mtdvlpr commented 1 year ago

Strange that it doesn't go to 100%. After it reaches 100%, it will reset and dissapear

mtdvlpr commented 1 year ago

Found it. It doesn't call setProgress in the mp3 if branch

sircharlo commented 1 year ago

I'll test the following on Win32:

mtdvlpr commented 1 year ago

I'll test the following on Win32:

Awesome! Afterwards I think we're good to go with the release :) The Sentry issues that are still open can't be pinned down to a fix (yet), so I'll set them to resolved in next release.

sircharlo commented 1 year ago

For some reason, lff isn't extracting properly on Win32. Other publications are, but not lff. Hmm. The JWPUB file gets downloaded, but not extracted. M3 logs.-1667768249921.log

sircharlo commented 1 year ago

Are we possibly hitting some sort of memory issue on low-memory systems, since we're loadAsync'ing the JWPUB files into memory before extracting them?

mtdvlpr commented 1 year ago

Might be related to this

sircharlo commented 1 year ago

Might be related to this

I'll wait for the build to finish and try it out with the updated try/catches

sircharlo commented 1 year ago

Here's something interesting. The JWPUB file doesn't seem to be downloaded properly.

image

mtdvlpr commented 1 year ago

Ah okay. We're missing a check to see if the jwpub file is valid. No wait, we do check if the cached file size is equal to the file size from the api call

sircharlo commented 1 year ago

I deleted the 0kb JWPUB file to see what happens.

  1. Click blue button
  2. JWPUB file starts getting downloaded
  3. Zipper seems to attempt to extract JWPUB file before download is complete
  4. JWPUB file seems to change size alot, almost as if it's being downloaded twice simultaneously and overwriting the same file?!
  5. Error message
  6. Windows folder now shows the whole JWPUB file
mtdvlpr commented 1 year ago

That would be in line with the stackoverflow answer:

I believe you would get this error after you stop your execution in between, i.e when the file is getting updated and this would corrupt the excel file.

mtdvlpr commented 1 year ago

Maybe because of the async execution the lff file is referenced multiple times and therefore download multiple times at the same time. You could look at the network tab to see if that's the case

sircharlo commented 1 year ago

Maybe because of the async execution the lff file is referenced multiple times and therefore download multiple times at the same time. You could look at the network tab to see if that's the case

Bingo.

image

sircharlo commented 1 year ago

"Both" files get downloaded at the same time, overwriting the same file. Zipper errors out on the first execution because the download from the second execution is still happening.

sircharlo commented 1 year ago

I can confirm that running the sync once the lff file has been fully downloaded (by a previous sync) results in a correctly unzipped lff folder and no further errors.

sircharlo commented 1 year ago

Makes sense that this is a new issue, seeing as pre-refactor, publications were always downloaded sequentially. If a pub already existed because it had been previously downloaded (no matter if it was 1 minute or 1 month ago), then the download was simply skipped.

This is definitely a scenario that would need to be accounted for, as I could see this causing errors in real-world usage, especially on newer installations that don't have publications like lff cached yet.

sircharlo commented 1 year ago

Would there be a way to modify downloadIfRequired to:

If it has:

If it hasn't:

mtdvlpr commented 1 year ago

Working on it

mtdvlpr commented 1 year ago

see #634

mtdvlpr commented 1 year ago

It seems to work, because I'm now only seeing one call after clearing the cache and no errors

mtdvlpr commented 1 year ago

@sircharlo, can you check if it also works on win32?

mtdvlpr commented 1 year ago

I'm signing off. Time to go to sleep😴

sircharlo commented 1 year ago

@sircharlo, can you check if it also works on win32?

Negative, unfortunately.

image

Probably related to the low amount of memory of 32-bit systems (https://stackoverflow.com/questions/45920143/how-to-handle-array-buffer-allocation-failed-in-nodejs)

Edit: the error went away after relaunching the app, so I'm gonna call this a "you should really have more than 4GB of RAM to run media at meetings" situation.

Other than that, the repetitive-download behavior is in fact gone.

sircharlo commented 1 year ago

Slight regression introduced, see https://github.com/sircharlo/meeting-media-manager/pull/634#issuecomment-1304952468

mtdvlpr commented 1 year ago

Slight regression introduced, see #634 (comment)

Fixed in #635

mtdvlpr commented 1 year ago

I got my virtual Windows 32-bit machine working too! Everything seems to work, except converting mp3 to mp4. That goes wrong every time. The error is now caught and a human friendly warning is given. Since converting mp3 to mp4 is a real edge case, and it only goes wrong on (low-memory?) 32-bit machines, I'd say forget about it.

sircharlo commented 1 year ago

I got my virtual Windows 32-bit machine working too! Everything seems to work, except converting mp3 to mp4. That goes wrong every time. The error is now caught and a human friendly warning is given. Since converting mp3 to mp4 is a real edge case, and it only goes wrong on (low-memory?) 32-bit machines, I'd say forget about it.

Even worse, it works for me in Win32 😆 But agreed, not the end of the world.

mtdvlpr commented 1 year ago

Haha, then it's probably a memory thing.

mtdvlpr commented 1 year ago

I'd say we're good to go then

sircharlo commented 1 year ago

Will you do the honors, or me?

mtdvlpr commented 1 year ago

It's easier for you, since you can push directly to master. I'd have to create a pull request which I'm not sure has the desired outcome.

mtdvlpr commented 1 year ago

@sircharlo, so what happened is you wanted to push the release commit, but got blocked because you had to git pull first. Then when you pushed the merge commit got evaluated instead of the release commit, so no release workflow was executed. You'll have to do another chore(release)

sircharlo commented 1 year ago

@sircharlo, so what happened is you wanted to push the release commit, but got blocked because you had to git pull first. Then when you pushed the merge commit got evaluated instead of the release commit, so no release workflow was executed. You'll have to do another chore(release)

Yeah my bad, I redid it and it worked after. :)

sircharlo commented 1 year ago

Weirdly enough, the autoupdate didn't work for me until I deleted the %localappdata%\meeting-media-manager-updater folder.

mtdvlpr commented 1 year ago

Hmm, it worked fine for me. The desktop shortcut also correctly updated this time.

mtdvlpr commented 1 year ago

Did you have a locally generated version installed by chance?

sircharlo commented 1 year ago

Nah, I didn't build on my machine at all. Must be a glitch with my setup, I wouldn't worry about it too much