search

sindresorhus / Gifski

🌈 Convert videos to high-quality GIFs on your Mac
https://sindresorhus.com/gifski
MIT License
7.79k stars 295 forks source link

Trim empty frames in preview #245

Closed codetheweb closed 3 years ago

codetheweb commented 3 years ago

Closes https://github.com/sindresorhus/Gifski/issues/190.

Test file (has black frame in preview with current version of Gifski):

https://user-images.githubusercontent.com/7410405/122309868-bcbfb100-ced4-11eb-9175-9b788dfe53f1.mp4

Note: this does not currently handle blank frames at the end of files. From my understanding, we would have to load all samples (while let sampleBuffer = readerOutput.copyNextSampleBuffer()) into memory to be able to iterate backwards through them. Happy to implement that as well if you think it's an acceptable trade-off.

sindresorhus commented 3 years ago

From my understanding, we would have to load all samples (while let sampleBuffer = readerOutput.copyNextSampleBuffer()) into memory to be able to iterate backwards through them. Happy to implement that as well if you think it's an acceptable trade-off.

It's unclear to me whether or not this is a problem. From my understanding, the samples are not actually the decoded frames, but rather just metadata, so it should be pretty efficient. Needs to be tested though, but I would like to support trimming empty frames from the end too as that's more common than empty frames at the start. And if we do trimming from the end, we could trim empty frames in the middle of the video too.

codetheweb commented 3 years ago

It's unclear to me whether or not this is a problem. From my understanding, the samples are not actually the decoded frames, but rather just metadata, so it should be pretty efficient.

I thought about this some more and not sure what I was talking about before. We can step through the whole file without loading all samples into memory...

Needs to be tested though, but I would like to support trimming empty frames from the end too as that's more common than empty frames at the start. And if we do trimming from the end, we could trim empty frames in the middle of the video too.

From what I've seen empty frames at the beginning of video files (i.e. simulator screen recordings) is caused by the start time metadata marker being negative / non zero (for example, it's start_time=0.245000 for the example I posted (ffprobe -i test.mp4 -show_format)). Do you have an example of a file with blank frames at the end?

I apologize for the number of changes required; I know in the issue you requested that the PR be 99% done when opened.

sindresorhus commented 3 years ago

I think this one has one or more blank frames at the end: Gifski.mp4.zip

codetheweb commented 3 years ago

It looks fine to me in both QuickTime and the current version of Gifski:

Screen Shot 2021-06-20 at 1 30 58 PM

Screen Shot 2021-06-20 at 1 31 10 PM

sindresorhus commented 3 years ago

If you play it in Quick Look, it's empty at the end, but I guess, it's just that the video track ends earlier than the actual video, which Gifski already handles by rewrapping the video track into a fresh asset.

I'm sure some users are having problems with empty frames at the end as I have seen it in encode failure errors on Crashlytics, but I don't think I actually have a fixture for this.

Would you be able to make a fixture for this? Maybe take the video with the empty leading frames and move those frames to the end or something.

codetheweb commented 3 years ago

Would you be able to make a fixture for this? Maybe take the video with the empty leading frames and move those frames to the end or something.

I would if I could, but as mentioned before as far as I can tell it's only possible to have blank / empty frames at the beginning of tracks, not the end (assuming the video track in a file gets moved into its own asset like Gifski does to prevent interference from audio tracks). It's easy to prepend / append a few frames of black to files with ffmpeg, but that's not the same thing as a blank frame.

I tried using -vf tpad=stop_duration=3 with ffmpeg to pad the end with empty data. It worked in that all apps seem to show a black image for the last 3 seconds, but the data size in each sample (as measured by .totalSampleSize) is still non-zero.

sindresorhus commented 3 years ago

I think you are right. Let's go with only trimming at the start then.

codetheweb commented 3 years ago

Thanks for all the detailed feedback; I really appreciate it.

sindresorhus commented 3 years ago

https://github.com/sindresorhus/Gifski/pull/245/files#r657856913 was marked as resolved, but was not resolved.

sindresorhus commented 3 years ago

Looks good now. Thanks :)