Moebits / waifu2x

waifu2x image upscaling in Node.js
MIT License
70 stars 11 forks source link

Fps still errors out sometimes #8

Closed Specy closed 3 years ago

Specy commented 3 years ago

when i try to upscale some videos i get the error:

Error TypeError: Cannot read property '0' of null
[1]     at C:\Users\dione\Desktop\Progetti\electron\scapix\node_modules\waifu2x\dist\waifu2x.js:420:78
[1]     at step (C:\Users\dione\Desktop\Progetti\electron\scapix\node_modules\waifu2x\dist\waifu2x.js:33:23)

Looking at the code that leads to:

return [2 /*return*/, Number(str.match(/[0-9]* (?=fps,)/)[0])];

in the parseFramerate function, when runnin ffmpeg on the same file i do find the fps info from the stderr of ffmpeg, i'd say maybe add some sort of try / catch in case that function doesnt find the fps info, and use 30 fps as default? i'm not sure what is causing the error but i SOMETIMES get it, i can't reproduce it correctly

Moebits commented 3 years ago

I think that the solution is just using ffprobe, it's much more reliable then scraping the fps with regex. Is there any reason why you don't want to use ffprobe?

Specy commented 3 years ago

I would want to use ffprobe, main issue is bundle size, I would have to add a 40mb executable just to get FPS info. I'd say just use ffprobe for now, if I can find a more reliable way I'll tell you, you could keep it in an external function like it is currently so that it's easier to just swap out ffmpeg for other things in the future

Moebits commented 3 years ago

Also it would be helpful if you can find a video to reproduce this. I'm not sure if there just isn't any fps info or if the fps is a decimal or fraction (in which case the regex I used here won't work).

Specy commented 3 years ago

That's the thing I don't understand, I tried to use ffpmeg to debug the same video and the FPS text showed up and I used the regex from the text I got and it worked fine. I then tried again to upscale the exact same video without any changes and it also worked, it just happens sometimes out of nowhere

Moebits commented 3 years ago

I made the parseFramerate function public, so if you want to avoid using ffprobe you can use it as follows:

const fps = await waifu2x.parseFramerate(source, ffmpegPath).catch(() => 30)

On a sidenote, I hope that you don't mind if I work on my own GUI for waifu2x. Because I've wanted to learn Electron and it seems like a decent beginner's project.

Specy commented 3 years ago

Well I can't force you otherwise but I noticed there are already a lot of GUI's for it (50ish I think) , this also was my first project with electron, it seems pretty fun except all the mess for packaging the program, I've simplified it in mine so if you need some help just look at how I did or make an issue. Also, I haven't yet used the new update of the module but I don't understand how this works at https://github.com/Tenpi/waifu2x/blob/49a82a55f370840981b5c86b2fc0a778f40454eb/waifu2x.ts#L283 The for loop is run syncrounously and the while loop never does more than 1 iteration because of the break right after it, right?

Moebits commented 3 years ago

Yes, the while loop only runs once. It's just there to check the value of cancel (set from the value of the callback function). If it's true, it breaks earlier from the loop. At least, this is what I think it does, I haven't had time to test it thoroughly. If you run into any bugs you can open another issue.

Specy commented 3 years ago

I don't know, it looks really odd to me as the while loop won't run and instead wait for the promise.all to finish and then break the loop, you changing the value of cancel shouldn't have any effect on the while loop or for loops

Moebits commented 3 years ago

Ok, I removed the outer while loop -> v0.4.0. I just break from the for loop instead.