Projektvorstellung: CGIF - GIF-Encoder in C

[Ein ehemaliger Benutzer]

Hi, ich bin Daniel und neu hier im Forum angemeldet. Lese aber schon länger mit.
Wollte hier mal ein OpenSource Projekt von mir vorstellen: cgif
cgif ist ein GIF-Encoder der in C geschrieben ist: https://github.com/dloebl/cgif
GIF ist zwar ein recht altes, aber noch ein weitverbreitetes Bildformat.
Der Encoder kann sowohl statische als auch animierte GIFs erstellen.
Gestartet habe ich das Ganze im Juni. Mittlerweile wird cgif von einem recht großen anderen Projekt verwendet: libvips/sharp

Weitere Besonderheiten:

• Schnelles Encoding - und soll noch schneller werden
• Keine externe Abhängigkeiten (außer einer libc)
• ca. 600 Zeilen C99 Code
• Ausführliche Dokumentation (hab versucht mir Mühe zu geben - gerne auch Feedback hierzu)
• Tests
• Aktives Fuzzing über OSS-Fuzz (via libvips)
• MIT Lizenz (permissive)

Für Feedback bin ich offen & dankbar. Gerade was Code-Qualität und Sicherheitsaspekte angeht.

[hustbaer]

Error-Handling ist quasi nicht vorhanden.
Deine Funktionen geben fröhlich Erfolg zurück, selbst wenn beim Schreiben ein Fehler aufgetreten ist.
Returnwert von malloc wird auch quasi nirgends überprüft.

schauder

[Ein ehemaliger Benutzer]

@hustbaer Guter Hinweis. Steht schon auf meiner ToDo für das Projekt. Macht Sinn, das mal vorzuziehen.

[yahendrik]

Ich habe das mal ausprobiert.
Vielleicht habe ich auch nur etwas übersehen, da ich mich jetzt auch nicht sooo lange damit beschäftigt habe.
Ich finde es eher suboptimal, dass eine Palette selber erstellt werden muss und nur 8 Bit-Bilder entgegengenommen werden. Der Inhalt ist nicht immer im Vorfeld bekannt.
Etwas mehr Komfort wäre schön, da die Quelle selten 256 Farben / 8 Bit hat. Mein Encoder, den ich vor zig Jahren mal erstellt habe, nimmt bspw. ARGB, RGBA oder RGB entgegen, wobei die row order noch angegeben werden kann (top down vs bottom up).
Er erstellt die Palette per Frame selbständig und sucht sich anschließend den besten Index heraus. Klar, auch mit deinem Encoder kann mit lokalen Paletten frameweise gearbeitet werden, aber so wie ich das sehe, muss diese Palette selbst erstellt und befüllt werden. Das ist aber imho eigentlich die Aufgabe eines Encoders.
Problematisch finde ich auch, dass pImageData (also das in CGIF_FrameConfig) nicht const ist, da überhaupt nicht klar ist, ob der Inhalt anschließend noch verwendet werden kann (zum Beispiel, wenn gleichzeitig ein gif und ein anderes Format (webp oder Video) erstellt werden soll.
Error-Handling wurde ja schon genannt.

Aber man kann zumindest jetzt schon damit nach kurzer Zeit arbeiten. Allerdings habe ich aus oben genannten Gründen die Frames vorher in Graustufen konvertiert.

[Ein ehemaliger Benutzer]

@yahendrik Danke für Dein Feedback.

Etwas mehr Komfort wäre schön, da die Quelle selten 256 Farben / 8 Bit hat.

Genau, übergeben werden immer 8-Bit Bilder (indexed). Die Palette muss entsprechend vom Nutzer gefüllt werden. Das Problem ist halt, dass wenn man beliebige RGBA Bilddaten erlaubt, ein Quantization + Dithering nötig wird (bei mehr als 256 Farben). GIF erlaubt eben nur 256 Farben in der globalen / lokalen Palette.
Quantization + Dithering ist dann wieder ein Projekt in sich selbst - Das Bild, das rauskommt, soll ja auch noch gut aussehen
Gibt da z.B. libimagequant: https://github.com/ImageOptim/libimagequant

Problematisch finde ich auch, dass pImageData (also das in CGIF_FrameConfig) nicht const ist

Guter Punkt. Die Encoding-Schicht lässt die vom Nutzer übergebenen Daten unberührt, sondern erstellt interne Kopien (wo nötig). Kann man zu const ändern.

[hustbaer]

@dloebl sagte in Projektvorstellung: CGIF - GIF-Encoder in C:

@hustbaer Guter Hinweis. Steht schon auf meiner ToDo für das Projekt. Macht Sinn, das mal vorzuziehen.

Cool

Nachdem du einen Fuzzer verwendest: schau mal ob dieser Fuzzer eine Option hat malloc Fehler bzw. allgemein libc Fehler zu injecten. Bzw. schreib Unit-Tests für Sachen wie den fehlenden Error-Check beim Schreiben. Bei sowas finde ich es immer nett wenn ich erstmal den Fehler nachstellen kann, und ihn danach erst fixe. Und idealerweise halt gleich automatisiert nachstellen, also mit einem Unit-Test und/oder sowas wie Address-Sanitizer/valgrind.

[Wutz]

• eine Funktion "write" zu nennen, ist sinnfrei
• zu viele mallocs, nur ein malloc ist gerechtfertigt, das für data
• memset 0 für struct ist sinnfrei
• Speicherverwaltung (malloc/free) gehört NICHT IN Fachfunktionen; schon gar nicht gehört ein free in den aufrufenden Kontext, wenn zuvor intern malloc verwendet wurde
• enum statt #define ints
• sinnfreie Zeigercasts [ (unsigned char*) ";" --- oh Graus ]
• was soll static vor initFrameConfig/initGIFConfig im Example?
• was soll initFrameConfig/initGIFConfig überhaupt dort? - die struct gehören bei ihrer Definition initialisiert, und zwar ohne memset/memcpy
• ...

[Ein ehemaliger Benutzer]

@Wutz

eine Funktion "write" zu nennen, ist sinnfrei

Die Funktion macht genau das, sie schreibt die Bilddaten entweder mit fwrite() oder einer write-callback (stream-basiert).

zu viele mallocs, nur ein malloc ist gerechtfertigt, das für data

Die Größe der komprimierten Bilddaten (die am Ende herauskommen) ist zu Anfang nur grob abschätzbar.
Klar man könnte die Daten direkt mit write() rausschreiben - gibt dann nur einen entsprechenden Overhead durch die hohe Anzahl der write-Aufrufe.
Also schreibe ich lieber in größeren Blöcken.

memset 0 für struct ist sinnfrei

Die Werte in einer Struct auf dem Stack/Heap sind erstmal undefiniert. Setze die mit einem memset auf einen sauberen Default.
Kann das aber auch bei der Initialisierung machen: <VarStruct> = {0};

enum statt #define ints

Guter Punkt. Du meinst wahrscheinlich die Flags in cgif.h?

sinnfreie Zeigercasts [ (unsigned char*) ";" ]

Der Clang hat sich halt beschwert:

cgif.c:659:15: warning: passing 'char [2]' to parameter of type 'const uint8_t *' (aka 'const unsigned char *') converts between pointers to integer types with different sign [-Wpointer-sign]

(ich stimme aber zu, dass man es verbessern kann)

was soll static vor initFrameConfig/initGIFConfig im Example?

Man benutzt static Funktionen, wenn diese nicht außerhalb des Moduls sichtbar sein sollen.
Das erlaubt dem Compiler (je nach Fall):

• Inlining
• Eine andere, effizientere calling-convention einzusetzen.

Ist common-practice in C.

[Wutz]

write ist kein signifikanter Name für eine Funktion, insbesondere nicht in einer Bibliothek, bspw. gibts write schon in POSIX. Bei deinen defines gibts du dir doch auch Mühe, möglichst signifikant zu sein.
static gehört als Scope-Limitierer in Bibliotheken aber nicht ins Main-Modul (standardkonform: Translation Unit). Und das ist definitiv kein "common-practice in C".
Auch dein Prinzip "alles was nicht passt wird passend gemacht" und dann auch noch per Zeiger-Cast - ist definitiv kein "common-practice in C". Zeiger-Casts sind (bis auf ganz wenige formelle Ausnahmen) immer sinnfrei und ein Anzeichen dafür, dass der Autor den Überblick über sein Programm verloren hat.
Stack/Heap gibt es gemäß C Standard nicht.
memcpy für struct zu verwenden ist sinnfrei und Blasphemie. (CGIF_FrameConfig)
Die mallocs für struct sind überflüssig, const für diese struct sind in deinem Programm oftmals angebracht.