Discussion about gifski HOT 49 CLOSED

Here's my progress so far with the save dialog. Feedback welcome.

from gifski.

Regarding the icon. I like the rainbow lines, but the icon shouldn't have any text, and I think the computer thing is a little bit overdone.

How about something simple like this?

from gifski.

I've pushed another commit now (b3e328c) with the finished initial app. I plan to publish it to the Mac App Store when I have your 👍. Can you try it out and let me know?

I will make this repo public when it's out in the Mac App Store and we can announce it then.

from gifski.

The buffer looks like it's ABGR rather than BGRA. You may need to byte-swap the pixels.

gifski_write() must be called before gifski_end_adding_frames(). It buffers only up to 4 frames, and then waits for ability to write them to disk.

I'll make a PR

from gifski.

gifski_write() must be called before gifski_end_adding_frames(). It buffers only up to 4 frames, and then waits for ability to write them to disk.

I was following the docs example which calls gifski_end_adding_frames() before gifski_write().

from gifski.

I'm sorry, I forgot to publish the docs fix (now live)

from gifski.

97d3655?w=1

from gifski.

Ah, ok. That makes it clearer. Thanks. I'm still not sure how to do byte-swapping of the pixels though.

from gifski.

From the docs:

The callback is called once per frame with NULL, and then once with non-null message on end.

It never returns a non-nil message:

Frame: 0
Frame: 1
Frame: 2
Frame: 3
Frame: 4
Frame: 5
Frame: 6
Frame: 7
isDone nil
Frame: 8
isDone nil
Frame: 9
isDone nil
Frame: 10
isDone nil
Frame: 11
isDone nil
Frame: 12
isDone nil
Frame: 13
isDone nil
Frame: 14
isDone nil
Frame: 15
isDone nil
Frame: 16
isDone nil
Frame: 17
isDone nil
Frame: 18
isDone nil
Frame: 19
isDone nil
Frame: 20
isDone nil
Frame: 21
isDone nil
Frame: 22
isDone nil
Frame: 23
isDone nil
Frame: 24
isDone nil
Frame: 25
isDone nil
Frame: 26
isDone nil
Frame: 27
isDone nil
Frame: 28
isDone nil
Frame: 29
isDone nil
Frame: 30
isDone nil
Frame: 31
isDone nil
Frame: 32
isDone nil
Frame: 33
isDone nil
Frame: 34
isDone nil
Frame: 35
isDone nil
Frame: 36
isDone nil
Frame: 37
isDone nil
Frame: 38
isDone nil
Frame: 39
isDone nil
Frame: 40
isDone nil
Frame: 41
isDone nil
Frame: 42
isDone nil
Frame: 43
isDone nil
Frame: 44
isDone nil
Frame: 45
isDone nil
Frame: 46
isDone nil
Frame: 47
isDone nil
Frame: 48
isDone nil
Frame: 49
isDone nil
Frame: 50
isDone nil
Frame: 51
isDone nil
Frame: 52
isDone nil
Frame: 53
isDone nil
Frame: 54
isDone nil
Frame: 55
Frame: 56
isDone nil
Frame: 57
Frame: 58
Frame: 59
isDone nil
Frame: 60
Frame: 61
isDone nil
Frame: 62
isDone nil
isDone nil
isDone nil
isDone nil
isDone nil
Frame: 63
isDone nil
Frame: 64
isDone nil
Frame: 65
isDone nil
Frame: 66
isDone nil
Frame: 67
isDone nil
Frame: 68
isDone nil
Frame: 69
isDone nil
Frame: 70
isDone nil
Frame: 71
isDone nil
Frame: 72
isDone nil
Frame: 73
isDone nil
Frame: 74
isDone nil
Frame: 75
isDone nil
Frame: 76
isDone nil
Frame: 77
isDone nil
Frame: 78
isDone nil
Frame: 79
isDone nil
Frame: 80
isDone nil
Frame: 81
isDone nil
Frame: 82
isDone nil
Frame: 83
isDone nil
Frame: 84
isDone nil
Frame: 85
isDone nil
Frame: 86
isDone nil
Frame: 87
isDone nil
Frame: 88
isDone nil
Frame: 89
isDone nil
Frame: 90
isDone nil
Frame: 91
isDone nil
Frame: 92
isDone nil
Frame: 93
isDone nil
Frame: 94
isDone nil
Frame: 95
isDone nil
Frame: 96
isDone nil
Frame: 97
isDone nil
Frame: 98
isDone nil
Frame: 99
isDone nil
Frame: 100
isDone nil
Frame: 101
isDone nil
Frame: 102
isDone nil
Frame: 103
isDone nil
Frame: 104
isDone nil
Frame: 105
isDone nil
Frame: 106
isDone nil
Frame: 107
isDone nil
Frame: 108
isDone nil
Frame: 109
isDone nil
Frame: 110
isDone nil
Frame: 111
isDone nil
Frame: 112
isDone nil
Frame: 113
isDone nil
Frame: 114
isDone nil
Frame: 115
isDone nil
Frame: 116
isDone nil
Frame: 117
isDone nil
Frame: 118
isDone nil
Frame: 119
isDone nil
isDone nil
isDone nil
isDone nil
isDone nil
isDone nil
isDone nil
isDone nil

from gifski.

The buffer looks like it's ABGR rather than BGRA.

Why is the method named gifski_add_frame_rgba and accepts RGBA8, while it's really BGRA it accepts? Sorry, my knowledge about this is very thin.

from gifski.

I managed to implement an inefficient byte-swapping: 231408c I'm certain it can be done more efficiently, so help welcome. Probably with some kind of bit-shifting https://stackoverflow.com/a/42133405/64949

from gifski.

I'll fix the isDone being nil. That's a bug.

BGRA was a typo in my github comment. It's supposed to be RGBA8.

from gifski.

The inefficient byte swapping is straightforward enough that hopefully the compiler will be able to optimize it nicely.

from gifski.

The progress callback with non-null arg was a bad idea, and it wouldn't work in the library implementation. I've dropped the argument (currently in git master, I'll release it as 0.8.0 later).

Now it's called only for frames. To know when the whole process is done, wait for gifski_write() to finish.

from gifski.

BGRA was a typo in my github comment. It's supposed to be RGBA8.

Ah ok. That means the original buffer is actually ARGB.

from gifski.

The inefficient byte swapping is straightforward enough that hopefully the compiler will be able to optimize it nicely.

Unfortunately not. Some quick testing:

debug
39 sec with byte swapping
14 sec without byte swapping

release
25 sec with byte swapping
9 sec without byte swapping

But we can worry about that later. I just want to get something out soon.

from gifski.

OK, I can add swapping in Rust then (edit: done)

from gifski.

I tried the latest master now and seems the byte-swapping is just as slow with Rust. I got 25 sec on release build now too. Can you reproduce that?

from gifski.

Are you sure it's byte swapping? It doesn't show up in profiling.

1 second per frame is normal for gifski. Almost all of the time is spent on quantization and remapping, and that can't be parallelized well, because each frame depends on all previous frames.

from gifski.

Using gifski_add_frame_rgba (The incorrect method) it only takes 10 seconds. I'm not sure if that's because of byte-swapping or because it has to do less work because the order is incorrect. Was just an observation that it got much slower after using byte-swapping.

from gifski.

Any way to use OpenMP with the API?

from gifski.

gifski_add_frame_rgba adds frames to a queue, and waits only if the queue is full. It doesn't perform any work itself, so it's not a reliable measure of time to encode. gifski_write() is.

from gifski.

Xcode uses clang, and clang doesn't support OpenMP, so it's tricky.

1. Install gcc brew install gcc
2. In gifski.xcodeproj Cargo target > Info: change build $(CARGO_FLAGS) to build $(CARGO_FLAGS) --features=openmp
3. In theory: gifski.xcodeproj Cargo target > Build Settings: add user-defined setting CC equal to gcc-7. In practice Xcode is so buggy that this panel doesn't work, so you need to edit gifski.xcodeproj/project.pbxproj manually and add CC = "gcc-7"; You probably need to add OPENMP_STATIC=1 as well, otherwise the executable may require libraries from homebrew.

from gifski.

There's also fast flag in GifskiSettings. It should double the speed at cost of ~20% of quality.

from gifski.

Oh, hold off with the fast flag. I just found a bug in it when OpenMP is used :)

from gifski.

OK, the fast flag is fixed. Clean build directory in Xcode to get the fix.

from gifski.

@kornelski I'm done with the initial version of the app. (FYI, I forced pushed to clean up the commit history) There are still many things missing, like the ability to cancel the conversion, but I'd like to get it out ASAP and we can continue from there. Could you try it out and give me some feedback?

from gifski.

Videos with odd widths are decoded as having an even width, which leads to stride vs width confusion:

from gifski.

I am worried people will throw long-form videos in there and create GB-large gifs. Maybe hardcode some limits? e.g.

while number of frames * image size > arbitrary limit { keep halving image size }
encode max 1000 frames? these limits could be a checkbox in the save window, or removed in a later version if it gets trimming or at least abort function.

(GIF uses about 2-4bits per pixel, so hugeness of the output is predictable in advance)

from gifski.

Videos with odd widths are decoded as having an even width, which leads to stride vs width confusion:

I thought videos could not have odd dimensions. Do you know if the problem is at the app-level here or in libgifski? I'm not really sure how to handle this.

while number of frames * image size > arbitrary limit { keep halving image size }

I don't fully understand this. You want to reduce the image size automatically? Wouldn't that be unexpected for the user. I think I better solution would be to have a image size option (in percentage of original?) in the export window.

or at least abort function.

I can easily implement the abort function.

(GIF uses about 2-4bits per pixel, so hugeness of the output is predictable in advance)

Does that mean we could estimate the resulting GIF size in the export dialog? That would be super useful

from gifski.

So instead of trying to invent a heuristic for this, I think it's better to just present the estimated size to the user and give them the ability to downscale the GIF, set quality, and eventually trim it. What do you think?

from gifski.

Yeah, that's doable.

The file size estimate can be as simple as bytes = width*height*number of frames/3.

I could add actual number of bytes written as an extra arg in the progress callback.

from gifski.

Videos can have odd sizes, the same way JPEG can, despite being limited to multiple of 8 internally. The frames are encoded at size rounded up. It's most likely a quirk of the video decoder - it leaves unused pixels at end of each row. it probably reports bytes per row or stride somewhere as a separate dimension from frame width.

from gifski.

it probably reports bytes per row or stride somewhere as a separate dimension from frame width.

CGImage has a bytesPerRow property, which we could maybe use, but I have no idea how to use it. Could gifski_add_frame_argb() accept a bytesPerRow argument and handle it in libgifski?

from gifski.

Yeah, I can add it in Rust easily. I'll update the lib tomorrow.

from gifski.

Fixed

from gifski.

This icon is for grabs: https://gif.ski/icon.png

GIFSKI.sketch.zip

from gifski.

From the gifski docs:

1-100, but useful range is 50-100. Recommended to set to 100.

Should I set the minimum to 50 then?

from gifski.

The file size estimate can be as simple as bytes = widthheightnumber of frames/3.

The quality seems to affect the resulting size too. Any suggestion how I should take that into account for the estimation?

from gifski.

Your icon is ok. The computer thing was supposed to be a VHS-era TV (since GIF is from 1989). The rainbow is inspired by VHS boxes: https://www.agostinoscafidi.com/wp-content/uploads/2016/07/VHS-box-series-by-Agostino-Scafidi-1024x707.jpg

from gifski.

I'd guess quality 50% lowers file size by 20%.

 file size = base file size * (quality + 1.5) / 2.5

from gifski.

Yes, set qualty to 50% minimum (0 on the slider could be q=50)

from gifski.

I'd guess quality 50% lowers file size by 20%.

file size = base file size * (quality + 1.5) / 2.5

Yes. Seems to be pretty accurate.

from gifski.

I'm having some problems building the app for distribution. When I build for running it works fine, but when using "Product" => "Archive" in Xcode, I end up with this error message:

SetOwnerAndGroup sindresorhus:staff /Users/sindresorhus/Library/Developer/Xcode/DerivedData/Gifski-fqctospusthmqjhbdupnamoaczvv/Build/Intermediates.noindex/ArchiveIntermediates/Gifski/InstallationBuildProductsLocation/usr/local/lib/libgifski.dylib
    cd /Users/sindresorhus/dev/private/gifski-app/gifski-api
    /usr/sbin/chown -RH sindresorhus:staff /Users/sindresorhus/Library/Developer/Xcode/DerivedData/Gifski-fqctospusthmqjhbdupnamoaczvv/Build/Intermediates.noindex/ArchiveIntermediates/Gifski/InstallationBuildProductsLocation/usr/local/lib/libgifski.dylib

chown: /Users/sindresorhus/Library/Developer/Xcode/DerivedData/Gifski-fqctospusthmqjhbdupnamoaczvv/Build/Intermediates.noindex/ArchiveIntermediates/Gifski/InstallationBuildProductsLocation/usr/local/lib/libgifski.dylib: No such file or directory
Command /usr/sbin/chown failed with exit code 1

Any ideas?

from gifski.

You have to link with the static library (libgifski.a), not dylib.

from gifski.

Scaling & filesize estimate help a lot! It's nice.

Can you add me to the About panel?

from gifski.

Can you add me to the About panel?

Of course, done: 8ba2b77 The about dialog is now just a RTF file. So feel free to add a mention of the gifski library there too ;)

from gifski.

Thanks!

The app is good to go!

from gifski.

The repo is now public as we're linking to it in the app and it's a requirement that it exists for the review to be accepted.

from gifski.