Add an option to read an image as raw bytes rather than PNG

[mortie]

There's a lot of tools, such as i3lock-fancy-rapid, which work by creating an image, encoding that image as PNG, and running i3lock -i <path>. I tried using that i3lock-fancy-rapid on my machine, which has a 4k display, and found that just encoding the PNG image and writing it to a file took seconds; longer than the actual blur in some cases.

This patch makes it possible to just dump a buffer of raw pixel bytes into i3lock. I made my own fork of i3lock-fancy-rapid, and found that with my preferred level of blurring, it's twice as fast; going from 0.68 seconds to 0.33 seconds. Without the blur, the difference is even bigger; from 0.65 seconds to 0.2 seconds.

I completely understand if this is out of scope for i3lock or something, I'll just keep maintaining my own branch, but I really think the performance gains are worth it. It also fits the "you should do all pre-processing in external tools" idea.

[Airblader]

I somewhat agree that compared to other proposals of supporting JPEG, BMP, … this is at least a fairly basic/generic approach. At the same time I'm not sure how much this would be used as most people don't deal with raw data. @stapelberg What do you think?

[mortie]

Most people don't manually deal with raw data, but a lot of people use tools like i3lock-fancy or their own imagemagick scripts to generate a lock screen image. It's not a huge change for users to modify their imagemagick-based scripts to save the result as .rgb instead of .png and provide a --raw option.

Now, having looked into it a bit, it seems like imagemagick dosen't support generating an ARGB image like cairo natively expects; it just supports creating an RGB or RGBA image. If it does make sense for i3lock to support raw formats, it should be able to at least convert from what imagemagick can create.

I also noticed that my error handling code has a couple of issues, like first setting img = NULL and then calling cairo_surface_destroy(img); I just pushed a fix to my branch. I'll work on pixel format conversion and push a commit for that later too.

[mortie]

I pushed a commit which adds support for 24 bits per pixel RGB.

I created a small lock script which just takes a screenshot with scrot, pixelates it with imagemagick, uses it as a lock screen image, both with and without my patch:

#!/bin/sh
scrot /tmp/screenshot.bmp

fast() {
    dims="$(file /tmp/screenshot.bmp | awk '{print $7 "x" $9}')"
    convert /tmp/screenshot.bmp -scale 5% -scale 2000% RGB:- \
        | i3lock --raw "$dims:rgb" --image /dev/stdin
    rm -f /tmp/screenshot.bmp
}

slow() {
    convert /tmp/screenshot.bmp -scale 5% -scale 2000% /tmp/screenshot.png
    i3lock --image /tmp/screenshot.png
    rm -f /tmp/screenshot.bmp /tmp/screenshot.bmp
}

if [ "$1" = fast ]; then
    fast
else
    slow
fi

For me, the "slow" (i.e without this patch) option takes 0.85-0.9 seconds according to time, while the fast option takes 0.45-0.5 seconds. That's already a pretty noticeable difference, and will be even more noticeable with dedicated tools which can screenshot and manipulate the image without ever serializing it to the filesystem such as that i3lock-fancy-rapid fork of mine.

[stapelberg]

I somewhat agree that compared to other proposals of supporting JPEG, BMP, … this is at least a fairly basic/generic approach. At the same time I'm not sure how much this would be used as most people don't deal with raw data. @stapelberg What do you think?

I agree that supporting raw pixels is preferable over supporting additional image formats, provided we can implement this in a way where we’re reasonably certain that the input is sanitized/safe.

In this case, that would probably mean ensuring that the correct number of bytes is read (preventing out of bounds).

I briefly looked at the documentation for cairo_image_surface_get_data and it seems like this use-case is supported. If there are other pieces of software which do the same thing, that would be an additional boost in confidence.

In general, I’m sympathetic to the performance argument and think that providing raw image support as a building block for third-party wrappers makes sense.

[mortie]

I went digging a bit for places where people are directly writing bytes to the buffer returned by cairo_image_surface_get_data, and found these:

• GTK modifies it when blurring: https://github.com/GNOME/gtk/blob/3f61360fa11fbc9e61585f40e197b4738f963829/gsk/gskcairoblur.c#L250
• Gecko seems to use a cairo buffer as a buffer to draw arbitrary to: https://github.com/mozilla/gecko-dev/blob/a759d6f62bdac9defd8ec7bbbef74e50474f3050/widget/gtk/nsDragService.cpp#L410

Something I should have done though, is to respect cairo's stride, not just assume that the number of bytes per line is the image width times the number of bytes per pixel. It also looks like I should probably have called cairo_surface_mark_dirty after drawing to it, and maybe cairo_surface_flush before drawing to it. I pushed changes to fix that.

---

Unrelated, but sorry for the large number of commits just to make CI happy. I should start compiling with CFLAGS="-Wformat -Wformat-security -Wextra -Wno-unused-parameter -Werror" like CI does. Maybe it would've been a good idea to make those warnings enabled by default in all builds?

[mortie]

What's the status on this currently? I don't mean to nag at all, and understand that things take time, but I'm excited about this, and have some i3lock-using friends I'd love to geek out over locking scripts with once this is merged.

Is there anything I can do to make the process go smoother?

[stapelberg]

Sorry, I was traveling recently and this fell through the cracks. I’ll try to take a look tonight.

[stapelberg]

Also, can you squash your commits please? We’d like this to be one logical git commit in the history after merging.

[stapelberg]

@psychon Would you have a second to take a look at this PR please? In particular, I’m hoping you might be familiar with how cairo images are laid out in memory, and whether this code gets it right regarding striping etc. — I trust the author has verified functionality, so things seem correct, but perhaps there’s some nuance to it. Thanks in advance,

[mortie]

Commits squashed.

I still don’t understand why you would use a bigger-than-necessary array? Why not size it exactly as large as it needs to be?

I don't know, mostly habit. I think of it like a buffer which has to be at least a certain size; making it a bit bigger than that doesn't hurt, decreases the chances of accidentally making the buffer slightly too small (either immediately or through changes in the future), and compilers tend to do a bunch of alignment anyways.

[mortie]

Currently, the default pixel format is "native". I'm not sure if that's correct, or if it would make more sense for "rgb" to be the default. What do you people think?

"native" sort of makes sense as a default, because it's the format which doesn't loop through each pixel in order to convert it. However, at the same time, people writing their own commands are more likely to use "rgb" from imagemagick or some other tool, while the people using "native" will mostly be people who write wrappers around i3lock, where the one who's writing the wrapper only has to write the i3lock command once, and can be expected to know more about how pixel formats work; if rgb was the default, only the people writing tools would have to specify anything.

Maybe the best way is to just not default to anything, to always require either rgb or native?

[owenthewizard]

IMO it's more realistic to expect people writing wrappers to worry about specifying native and default to rgb for the end user's sake. Not having a default sounds good too.

[stapelberg]

+1 for being explicit (no default). This is an advanced option, and spelling out all the details results in more clarity.

[stapelberg]

This seems good enough for now; we can always fix things later if required.

Thanks for your work on this!

[mortie]

Hmm, I noticed earlier today that i3lock-color, the seemingly fairly popular fork of i3lock, already uses the -r short opt. Since this feature also uses -r, that project would need to either break backwards compat, or change this feature to use a different option instead (such as -R). Does that affect amything? To what extent should i3lock try to not add options which conflict with popular forks?

It would be a quick fix to change the short opt from -r to something else if you think that's warranted.

[stapelberg]

Given that it’s a quick fix, let’s change it. We could also just not give a short option for this at all, and just use --raw, which is more descriptive anyway (and you’re not typing i3lock commands by hand anyways).

[owenthewizard]

Could an rgba format be added, identical to rgb but with an extra byte (that is ignored)? This would allow for easier compatibility with XCB screenshots.

[stapelberg]

Isn’t that what the native format does?

[mortie]

No, native is ARGB (or, well, XRGB, since the "alpha" byte is ignored), and in the machine's endianness (so since most machines are little endian, that usually means the actual bytes are stored as BGRX).

I took some time to add support for the XRGB, RGBX, BGR, XBGR, and BGRX: https://github.com/mortie/i3lock/commit/e91cc5588e0ef4d44393431649917a045d428393. What do you think @stapelberg, shall I submit a pull request?

[stapelberg]

Do we really need all the variants? Are they really all common? But yeah, submit a PR please.

[mortie]

The advantage to just supporting a lot of different formats is that it makes wasteful pixel formats conversions less likely. Maybe someone, say, uses a library which returns a buffer of native-endian RGBA data; they would want to use the XBGR format on little-endian systems and RGBX on big-endian systems. I have worked a lot with raw images, and can say that I have been extremely happy more than once for libraries providing support for weird formats (such as libYUV's NV12ToABGR).

It isn't really any more difficult to support all those formats than to just support a few of them; each format other than native is just a different configuration of how many bytes per pixel there are and what order they are in.

Another completely valid stance would be to say that i3lock just supports the "native" format for anyone who wants maximum performance and the "rgb" format for anyone who wants to use imagemagick or other command-line tools which produce raw images, and say that cases such as xcb screenshots should be solved by just converting from RGBA to "native" as the data is written to a file (or i3lock's stdin). That wouldn't really be any more or less efficient, it would just make i3lock sightly smaller and make it a bit harder to make a tool which writes raw image data to i3lock.

In any case, pull request submitted.