add webp image support #31
Conversation
|
Thank you for your offered code, but before I get into the details of your commits, I have to say that I am not interested in merging webp support for two reasons:
Let's get into details now and what I would recommend, since you asked for feedback. This is the least I can do since you spent time and efforts with this merge request. Why does xwallpaper support only JPEG and PNG and why out of all formats additionally XPM?If you use Xorg, you have most likely PNG support on your machine already. An example of PNG files would be cursor files shipped with Xorg (xcursorgen, xcursor-themes for example). This is not true for all systems. An example would be OpenBSD's xenocara which has no PNG libraries on a default installation. As a last resort there might be XPM support. It's a common format used by X11 Athena Widget. To keep this short: Simply don't use it. If at all, I recommend to disable XPM support. So PNG and XPM could be considered native. JPEG is added because of its popularity. Support for JPEG and PNG means that a large coverage with only few libraries or lines of code is reached. What would be alternatives to support more file formats?Let's simplify reality: Every desktop system has a browser. And it's either (based on) Chrome/Chromium or Firefox. Both browsers use GTK in one way or the other which means that gdk-pixbuf is available. I like the code quality of gdk-pixbuf and its good coverage of automate fuzzing. It also has support for additional loaders like gif, webp, etc. So... If minimalism wouldn't be a focus of xwallpaper anymore and more formats should be supported, I would recommend to replace the xwallpaper loaders with a gdk-pixbuf interface. Separating the loaders away from Xorg socket communication by passing file descriptors through dbus would be interesting as well. And to avoid programming flaws while implementing this, using Rust sounds reasonable. But all of these things mean to move further away from raw Xorg support by dragging in more libraries, frameworks, languages. And that's why it did not happen. |
| @@ -0,0 +1,75 @@ | |||
| /* | |||
| * Copyright (c) 2021 Tobias Stoeckmann <tobias@stoeckmann.org> | |||
There was a problem hiding this comment.
The copyright is added to each file to clarify who wrote the major parts of it. In this case, it would be your copyright, not mine.
| { | ||
| pixman_image_t *img; | ||
| uint8_t *data, *data_buffer, *p; | ||
| uint32_t *pixel, *pixels; |
There was a problem hiding this comment.
Many lines differ between tab and space usage.
|
|
||
| fseek(fp, 0, SEEK_END); | ||
| data_size = ftell(fp); | ||
| fseek(fp, 0, SEEK_SET); |
There was a problem hiding this comment.
The code does not cover error conditions. The worst part would be ftell returning -1. Since data_size is an unsigned type, you would have all bits set to 1. Which leads to...
There was a problem hiding this comment.
The whole setup of fseek+ftell looks like fstat would be a better choice (but even then look out for off_t size_t differences in st_size).
| data_size = ftell(fp); | ||
| fseek(fp, 0, SEEK_SET); | ||
|
|
||
| data = (uint8_t*)WebPMalloc(data_size + 1); // + 1 for \0 terminator |
There was a problem hiding this comment.
... a possible integer overflow for large files on 32 bit systems or on all systems if ftell fails with -1. Effectively you would allocate 0 bytes which might lead to issues later on, although fread should fail before overriding unallocated memory. I have not checked WebPMalloc to see how it behaves with such an input value.
| width * sizeof(uint32_t)); | ||
|
|
||
| if (img == NULL) | ||
| errx(1, "failed to create pixman image"); |
There was a problem hiding this comment.
In this error case you have to free pixels as well.
| return NULL; | ||
| } | ||
|
|
||
| data_buffer = WebPDecodeARGB(data, data_size, &width, &height); |
There was a problem hiding this comment.
You do not check data_buffer == NULL case, i.e. error handling. The variables width and height might be uninitialized which might lead to null pointer dereferences later on.
|
Hi @stoeckmann, firstly, thank you for your very detailed answer and your code review. To be fair, the reason for why I chose xwallpaper was more the simplicity of the project rather than its security aspects. I am clearly lacking knowledge regarding these things, hence, it is interesting to see which ideas you have developed while designing this tool and which aspects one might consider also for other projects. Regarding code review: Thank you for taking time and providing useful comments. I really should have been more careful about error handling, clearly a point where I can improve. It is ok if you don't merge it, overall I implemented the feature primarily for myself. So I'll keep the patches around and reapply them whenever xwallpaper receives an update. Thank you again for your time and comments, I really appreciate this. |
|
One more thing: I just realized that the webp project also provides a cli tool to convert webp files to png and other formats, cf. https://developers.google.com/speed/webp/download. This now renders the above pull request obsolete. |
Hi,
yesterday I downloaded some wallpapers in webp format and noticed that I cannot load them with xwallpaper.
So I went ahead and implemented webp decoding and I now want to share this with you.
My implementation mimics closely the ones for jpg, png and xpm formats.
The new optional dependency is libwebp, tested with version 1.1.0_2.
I hope someone can review and comment on this and perhaps even merge it.