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

Real lossless ugoira #1550

Closed
wants to merge 10 commits into from
Closed

Real lossless ugoira #1550

wants to merge 10 commits into from

Conversation

fmnijk
Copy link

@fmnijk fmnijk commented May 13, 2021

I used the "--ugoira-conv-lossless" argument to generated lossless webm file.
But the framehash values calculated by ffmpeg framehash muxer are different to original jpg files in zip file.

So I add a "--ugoira-conv-copy" argument.
It used the method in the accepted answer of here.
Compared to "--ugoira-conv-lossless" argument, the resulted file is smaller and the conversion is faster.

I also add a "repeat-last-frame" option in ugoira.py.

@nisehime
Copy link

Don't use -i %6d.jpg and -framerate instead of concat.
-c copy option doesn't seem to work with all formats. WebM and GIF won't work.

I don't think VP9 encoding with the -lossless option actually loses quality. The differences in hashes could be due to pixel format conversions or something like that (though pixel format conversions could be considered quality loss?)

@fmnijk
Copy link
Author

fmnijk commented May 14, 2021

Okay. Forgot the imagehash. But I cannot find an easy way to prove a resulted webm file is really lossless. Even if it is, there is no benefit. Just wasting time and get a larger file.
Also not sure the conversion from yuvj444p to yuv420p will loss quality or not.

-c copy will make -r invalid, that's why I use -framerate. But -framerate is not compatible with ffconcat. So I use -i %6d.jpg. Maybe there is a better way. I couldn't find it but I tried.

@nisehime
Copy link

Maybe there is a better way. I couldn't find it but I tried.

Just disable -r option (set framerate to null) and leave concat, it should be fine. I actually think framerate option should be set to null by default anyway.

Even if it is, there is no benefit.

Not everyone would want a mkv/mp4 file, but as an additional option why not I guess.

@nisehime
Copy link

By the way, you can expand this is issue to not only about video quality but also preserving the original ugoira timings, because FFmpeg has a problem with it. But you have to use mkvmerge for it.

@mikf
Copy link
Owner

mikf commented May 14, 2021

actually think framerate option should be set to null by default anyway.

Without -r, FFmpeg defaults to 25 fps, at least when using a concat file as input.
The resulting .webm file from converting a Ugoira with 34 ms frame delay and 150 frames only has 130 frames, the missing 20 get silently dropped.

This doesn't apply when using -c copy and putting all frames into a .mkv, though. ffprobe still reports 25 fps, but it has all 150 frames.

Also not sure the conversion from yuvj444p to yuv420p will loss quality or not.

It most likely does, but some players can't play a yuvj444p video.

@nisehime
Copy link

The resulting .webm file from converting a Ugoira with 34 ms frame delay and 150 frames only has 130 frames, the missing 20 get silently dropped.

Have you tried -vsync 0? The -r option before input modifies the frame timecodes of the input stream to match this fps, which bothers me, since ugoira techically can have variable framerate. (But this is not the problem I'm talking about, even without -r option FFmpeg loses original frame duration unfortunately)

@fmnijk
Copy link
Author

fmnijk commented May 15, 2021

By the way, you can expand this is issue to not only about video quality but also preserving the original ugoira timings, because FFmpeg has a problem with it. But you have to use mkvmerge for it.

mkvmerge seems like not designed for merging images to a video. Maybe I should wait for the problem of ffmpeg to be fixed.

I think -r is needed to preserve original timings since the default is 25. If the default is 100 then the timings could be fine.

@nisehime
Copy link

nisehime commented May 15, 2021

mkvmerge seems like not designed for merging images to a video.

You can make mkv with FFmpeg, then use mkvmerge to fix timecodes, mkvmerge doesn't re-encode video in this case afaik.

Maybe I should wait for the problem of ffmpeg to be fixed.

This problem seems to be quiet old, but barely anyone noticed it. I reported it to FFmpeg devs recently but most likely it can take them very long to do something about it. Just like this issue with duplicating the last frame.

I think -r is needed to preserve original timings since the default is 25.

-r doesn't preserve the original timings, it basically forces constant fps on the input (if put before -i), meanwhile ugoira can have variable fps.
This issue wasn't that noticed since the difference of original timings to FFmpeg's roundings are in milliseconds, so visually it should look more or less the same, but still, it exists.

@fmnijk
Copy link
Author

fmnijk commented May 15, 2021

I checked the pkt_pts, pkt_dts and other values ffprobe reported. Since the video is 25 fps, they shifted to the closest reasonable value in a 25 fps video which is multiple of 40. And I play the video using potplayer at 0.2 speed. I can see frame durations are wrong. To solve the round-off error problem, I think set fps to 100, 1000 or other reasonable value should work.

@nisehime
Copy link

nisehime commented May 15, 2021

Ok, I tested some random ugoira which had 20ms duration on all frames without -r and it turned out FFmpeg messed up timings really bad (each frame pair had the same timecodes). So yeah, unfortunatelly we have to use -r auto on input (or -framerate if it's -c copy). But this is a kludge, not a solution. As long as ugoira has stable duration on all frames it should work fine, otherwise we will have some deviations from original timings.

To solve the round-off error problem, I think set fps to 100, 1000 or other reasonable value should work.

Timestamp roundings are happening on the input stream, not output, so setting output fps to 999 or anything else won't really help.

@fmnijk
Copy link
Author

fmnijk commented May 15, 2021

Timestamp roundings are happening on the input stream, not output, so setting output fps to 999 or anything else won't really help.

You are right. I tested it. Nothing changed.
Variable duration will always round to the nearest multiples of 40 ms with or without -r.

@nisehime
Copy link

I think I found a workaround for this issue. @mikf

  1. Duration in concat file should be in seconds (I mean you don't have to divide duration from ugoira metadata by 1000)
  2. Add this arguments to the output stream: -enc_time_base 1/1000 -vf "settb=1/1000,setpts=PTS*0.001"

You definitely shouldn't put -r before -i. BUT if you put -r for output (after -i) with this value: 1000/{duration of the last frame}, you wouldn't need to duplicate the last frame to keep its duration.

I only tested it for webm format, and it was fine with those ugoiras I tested, but I'm not sure yet this can be stable.

@mikf
Copy link
Owner

mikf commented May 22, 2021

So I tested your workaround and it works perfectly for VP8/VP9 videos, but not at all for basically anything else. It doesn't work with -c copy to copy all frames to an MKV container or with -filter_complex to have better quality GIFs, and it causes libx264 to generate a whole lot of duplicate frames, probably 1000 frames per second.

You definitely shouldn't put -r before -i

Could you explain why this is bad? It works perfectly fine for ugoira with constant frame delay, i.e. the frame timestamps from ffprobe -i input.webm -show_frames -show_entries frame=pkt_pts_time -of csv=p=0 all appear correct and are evenly spaced.

For ugoira with more than one frame delay value, the current strategy puts -r after -i and the frame timestamps with that are all over the place.


And just for reference, here is Danbooru's ugoira-to-webm code:
https://github.com/danbooru/danbooru/blob/f78d10a5911e632cf92e675e6af7fe42aca2232c/app/logical/media_file/ugoira.rb

It generates a .webm and then uses mkvmerge to set the correct frame timecodes..
It also duplicates the last frame.

@nisehime
Copy link

It doesn't work with -c copy to copy all frames to an MKV container

Well, yes, -c copy doesn't work with FFmpeg filters unfortunately.

with -filter_complex to have better quality GIFs

Are you sure? Honesly, I'm not that good in FFmpeg filters, so can't say much about it, maybe it just needs to be formatted appropriately.

causes libx264 to generate a whole lot of duplicate frames, probably 1000 frames per second.

You mean mp4 format? Because libx264 in mkv looks fine so far. Adding -vsync 0 to mp4 output looks fine at first too. The actual problem is that using -r 1000/{duration of the last frame} to keep the duration doesn't work with mp4. Don't really know what to do about it currently.

You definitely shouldn't put -r before -i

It was meant for my workaround. We simply don't need to put -r before -i because it will change the input timecodes.

Without my workaround you can leave it as it is. Earlier I said that you don't need to put -r at all, but I was wrong. Also, I didn't know that your ugoira code somehow decides when to put -r before or after -i

It works perfectly fine for ugoira with constant frame delay

Yes, as long as frame delays are constant it works perfectly fine, but as I said it's not always the case.

It generates a .webm and then uses mkvmerge to set the correct frame timecodes.

I suggested using mkvmerge earlier here already, the only problem is that it works with WebM/MKV formats only.

It also duplicates the last frame.

The danbooru code is quiet old. With current mkvmerge you don't need to duplicate the last frame, I tested it.

@thatfuckingbird
Copy link
Contributor

@nisehime
Copy link

nisehime commented May 24, 2021

I've seen it. Nothing really useful tbh. Mostly it's the same as gallery-dl and has the same issue with timecodes.

I think I kinda went off-topic in this pull request. Should I open it as issue instead?

mikf added a commit that referenced this pull request May 26, 2021
Ensures exact frame timecodes with no duplicate frames.

Possible issues are the duration the last frame in an Ugoira with variable
frame durations is shown and insufficient timestamp precision of the
underlying file system (e.g. FAT32, ext3; works on ext4, tmpfs, NTFS).
@mikf
Copy link
Owner

mikf commented May 26, 2021

I believe I found a method to get accurate timecodes with only FFmpeg: the image2 demuxer and its ts_from_file option (be9547a). It requires the underling filesystem to have at least millisecond precision, but most of the ones used today do (NTFS, APFS, ext4; doesn't work on FAT32 or ext3).

The only remaining issue is the display time of the last frame in a variable-delay Ugoira. Ugoira with constant delays don't "require" duplicating the last frame with this method. (I've also added the repeat-last-frame option: 74d97e6)

Concerning this PR @fmnijk: Keep the changes in option.py except "re-encoding" and revert everything else.

@nisehime
Copy link

Ah. Well, I knew about ts_from_file, but I thought it'd be too much to bother with, so decided to drop this idea lol. That's kinda weird way to specify timecodes from FFmpeg devs.

Have you tested it? Also, on this line there's only .jpg format is set for input as I can see, but ugoira can also be .png if I'm not mistaken.
Why are you excluding .gif from repeating the last frame procedure btw? .gif also requires repeating to keep the duration.

@mikf
Copy link
Owner

mikf commented May 26, 2021

Have you tested it?

Yes, it works really well on Linux.

but ugoira can also be .png if I'm not mistaken.

Never seen one where the frames aren't .jpg, but making it file extension independent as long as all files have the same extension is easy enough. (edit: done 19a11fa)

Why are you excluding .gif from repeating the last frame procedure btw?
.gif also requires repeating to keep the duration.

This is only when using the concat demuxer, it worked like this before (ugoira.py line 93), and it is definitely not required to keep the duration.
I tested this previously and did another test just now. Duplicating the last frame in a .gif looks wrong and coverting the same ugoira to gif with image2 or concat (without repeated frames) produces the exact same output.

@nisehime
Copy link

nisehime commented May 26, 2021

https://www.pixiv.net/en/artworks/44663338

This is what I'm testing. Latest build from Actions

Made the .gif file with concat, opening it in browser and I can clearly see that last frame lasts for a really short time, like half a second, meanwhile on pixiv page the last frame stays for about 2 seconds. And it was like that before too.
Actually, I think I get you. It should be fine with constant framerate and you really wouldn't need to duplicate the last frame. But with variable fps it's a bit different.

Testing the image2 demuxer and... it doesn't work.

C:\Users\Madobe\Desktop\gallery-dl-windows-latest>gallery-dl.exe -v https://www.pixiv.net/en/artworks/44663338
[gallery-dl][debug] Version 1.17.5-dev
[gallery-dl][debug] Python 3.8.10 - Windows-8.1-6.3.9600-SP0
[gallery-dl][debug] requests 2.25.1 - urllib3 1.25.11
[gallery-dl][warning] unsupportedfile: [Errno 2] No such file or directory: 'C:\\Users\\Madobe\\Desktop\\gallery-dl-windows-latest\\logs\\unsupported.txt'
[gallery-dl][debug] Starting DownloadJob for 'https://www.pixiv.net/en/artworks/44663338'
[pixiv][debug] Using PixivWorkExtractor for 'https://www.pixiv.net/en/artworks/44663338'
[urllib3.connectionpool][debug] Starting new HTTPS connection (1): app-api.pixiv.net:443
[urllib3.connectionpool][debug] https://app-api.pixiv.net:443 "GET /v1/illust/detail?illust_id=44663338 HTTP/1.1" 200 840
[pixiv][debug] Active postprocessor modules: [UgoiraPP]
[urllib3.connectionpool][debug] https://app-api.pixiv.net:443 "GET /v1/ugoira/metadata?illust_id=44663338 HTTP/1.1" 200 230
[urllib3.connectionpool][debug] Starting new HTTPS connection (1): i.pximg.net:443
[urllib3.connectionpool][debug] https://i.pximg.net:443 "GET /img-zip-ugoira/img/2014/07/12/18/40/59/44663338_ugoira1920x1080.zip HTTP/1.1" 200 2262269
  .\galleries\pixiv_work\479376 (starmine255)\44663338_p0.gif[postprocessor.ugoira][debug] ffmpeg args: ['C:\\Users\\Madobe\\Desktop\\ffmpeg-N-102564-g2261cc6d8a-win64-gpl\\bin\\ffmpeg', '-f', 'image2', '-ts_from_file', '2', '-pattern_type', 'sequence', '-i', 'C:\\Users\\Madobe\\AppData\\Local\\Temp\\tmp37hydc5g/%06d.jpg']
ffmpeg version N-102564-g2261cc6d8a Copyright (c) 2000-2021 the FFmpeg developers
  built with gcc 10-win32 (GCC) 20210408
  configuration: --prefix=/ffbuild/prefix --pkg-config-flags=--static --pkg-config=pkg-config --cross-prefix=x86_64-w64-mingw32- --arch=x86_64 --target-os=mingw32 --enable-gpl --enable-version3 --disable-debug --disable-w32threads --enable-pthreads --enable-iconv --enable-libxml2 --enable-zlib --enable-libfreetype --enable-libfribidi --enable-gmp --enable-lzma --enable-fontconfig --enable-libvorbis --enable-opencl --enable-libvmaf --enable-vulkan --enable-amf --enable-libaom --enable-avisynth --enable-libdav1d --enable-libdavs2 --enable-ffnvcodec --enable-cuda-llvm --enable-libglslang --enable-libgme --enable-libass --enable-libbluray --enable-libmp3lame --enable-libopus --enable-libtheora --enable-libvpx --enable-libwebp --enable-lv2 --enable-libmfx --enable-libopencore-amrnb --enable-libopencore-amrwb --enable-libopenjpeg --enable-librav1e --enable-librubberband --enable-schannel --enable-sdl2 --enable-libsoxr --enable-libsrt --enable-libsvtav1 --enable-libtwolame --enable-libuavs3d
 --enable-libvidstab --enable-libx264 --enable-libx265 --enable-libxavs2 --enable-libxvid --enable-libzimg --extra-cflags=-DLIBTWOLAME_STATIC --extra-cxxflags= --extra-ldflags=-pthread --extra-ldexeflags= --extra-libs=-lgomp
  libavutil      57.  0.100 / 57.  0.100
  libavcodec     59.  1.100 / 59.  1.100
  libavformat    59.  2.100 / 59.  2.100
  libavdevice    59.  0.100 / 59.  0.100
  libavfilter     8.  0.101 /  8.  0.101
  libswscale      6.  0.100 /  6.  0.100
  libswresample   4.  0.100 /  4.  0.100
  libpostproc    56.  0.100 / 56.  0.100
[image2 @ 0000004304b821c0] POSIX.1-2008 not supported, nanosecond file timestamps unavailable
C:\Users\Madobe\AppData\Local\Temp\tmp37hydc5g/%06d.jpg: Function not implemented
* .\galleries\pixiv_work\479376 (starmine255)\44663338_p0.gif

No idea what's the problem. Is it Windows or FFmpeg itself

@mikf
Copy link
Owner

mikf commented May 26, 2021

Actually, I think I get you. It should be fine with constant framerate and you really wouldn't need to duplicate the last frame. But with variable fps it's a bit different.

Sounds like the same underlying issue I touched in my post above. The last frame only gets shown for 1/fps seconds, and fps gets calculated as 1/GCD of all frame delays. Works for constant framerates, but is usually too short for variable framerates. image2 has the same problem.

No idea what's the problem. Is it Windows or FFmpeg itself

Its FFmpeg. In Python os.stat() can return file times up to 100ns precision, which is more than enough, but FFmpeg refuses to do anything with -ts_from_file 2. 1 works, but it only interprets file times in 1s intervals. Guess image2 can't be used by default then and only works on non-Windows platforms.

@nisehime
Copy link

nisehime commented May 26, 2021

I did another tests and it looks like not only .gif, but also other formats (at least webm and mp4) don't need duplicating when fps is constant (explicitly set with -r or -framerate before -i). If only all ugoiras had constant fps.

Guess image2 can't be used by default then and only works on non-Windows platforms.

That sucks. So many problems with FFmpeg.

Well, you still can implement my solution from the post above (#1550 (comment)), make it optional and use it for webm.

mikf added a commit that referenced this pull request May 28, 2021
'image2' with nanasecond mtime timestamps doesn't work on Windows
@fmnijk
Copy link
Author

fmnijk commented Jun 8, 2021

I tried mkvmerge and it works fine! There is nothing wrong in ffprobe output. Both CFR and VFR have correct dts and duration with milliseconds precision.

But some video renderer might not keep the duration of the last frame.
I am using potplayer.
If I use VMR 9 as renderer, the last frame has correct duration.
If I use EVR(CP), the last frame has very short duration which is wrong.

@ImportTaste
Copy link
Contributor

mkvmerge does have a python wrapper, but, as stated in the README, the current documentation is a bit sparse.

@fmnijk fmnijk closed this Mar 22, 2022
mikf added a commit that referenced this pull request Mar 28, 2022
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.

None yet

5 participants