Move to use cairo glyphs instead of text #168

PandorasFox · 2017-12-09T01:47:34Z

Requires linking fontconfig now, but that was already required at runtime for cairo anyways.

Fixes the memory leaks caused by using cairo_select_font_face, as mentioned in #167

Requires linking fontconfig now, but that was already required at runtime for cairo anyways.

Airblader · 2018-01-09T08:42:37Z

unlock_indicator.c


-            cairo_set_font_size(ctx, 14.0);
+            if (text ) {


Broken formatting

The only time the font_size was set to 14px was here. That got moved to here in this patch.

Sorry, that's not what I meant. I meant that the code formatting is wrong here because there's an extra space after text.

Ahh, my bad. I didn't notice that, I'll fix that.

Airblader · 2018-01-09T08:43:03Z

@stapelberg I can offer a review in terms of checking the code, but given the changes to the dependencies I'd assume you would want to look at this, too.

stapelberg · 2018-01-09T08:55:34Z

unlock_indicator.c

+        DEBUG("config sub failed?\n");
+        return NULL;
+    }
+    FcPattern *tmp = FcFontMatch(FcConfigGetCurrent(), pattern, &result);


You can add a FcPatternDestroy(pattern) call right below this line to get rid of the duplication below.

stapelberg · 2018-01-09T08:55:48Z

unlock_indicator.c

+
+    FcPatternDestroy(pattern);
+    pattern = tmp;
+    tmp = NULL;


Why not keep using tmp? I think that’s clearer than re-purposing a variable.

stapelberg · 2018-01-09T08:56:43Z

unlock_indicator.c

+
+    FcResult result;
+    FcBool res = FcInit();
+    if (!res) {


Why not just if (!FcInit())?

stapelberg · 2018-01-09T08:57:21Z

unlock_indicator.c

+        return NULL;
+    }
+
+    FcPattern *pattern = FcNameParse(font_face_name);


Can you add some comments to the various Fc* function calls here outlining what they do? Also, is this following some sort of example or document? If so, can you add a comment with a link to it please?

Also, is this following some sort of example or document?

Sort of. I found some partial examples for looking up fonts/drawing text with them, but none of them properly handled memory cleanup.

I can poke around for them tomorrow.

stapelberg · 2018-01-09T08:58:08Z

I don’t mind the explicit dependency on fontconfig.

Once we’re happy with the code, we should get someone with cairo knowledge to review. Perhaps psychon?

Airblader · 2018-01-09T09:00:14Z

Yes, that sounds like a good idea. At least as a sanity check. I agree that we should first in a shape where we're happy with the PR.

PandorasFox · 2018-01-09T09:28:43Z

I'll make those changes tomorrow morning. I think I developed this patch on this laptop (I'm traveling for the holidays), so I should have the test programs I made to figure out how clean up the fonctconfig structs properly.

PandorasFox · 2018-01-09T19:00:32Z

Alright, I added some comments and made the changes with variable handling/duplicate lines.

I couldn't find the files I used for reference when working on this, so I'm assuming they're on my computer back home. I'll keep poking around to see if I can find a gist or something.

stapelberg · 2018-01-10T08:27:13Z

unlock_indicator.c

+     * On successive calls, does no work and just returns true.
+     */
+    if (!FcInit()) {
+        DEBUG("Fontconfig init failed. No fonts will be available...\n");


What does “no fonts will be available” imply? Will no text be visible on screen? If so, make the message say that :)

stapelberg · 2018-01-10T08:28:17Z

unlock_indicator.c

+     * Looks up the font pattern and does some internal RenderPrepare work,
+     * then returns the result.
+     */
+    FcPattern *pattern_ready = FcFontMatch(FcConfigGetCurrent(), pattern, &result);


result is assigned but not used. Can we either check it or remove it?

This is a fun one. It has to be assigned (or else fontconfig will error out), but its value on a success isn't well-defined. In fonctconfig's provided examples they also only check to see if they get back NULL or not :(

I'll update the commenting to say returns the resulting pattern to be a bit more clear, though.

stapelberg · 2018-01-10T08:29:50Z

unlock_indicator.c

+
+    /*
+     * Gets the default font for our pattern. (Gets the default sans-serif font face)
+     * Required to make the following FcFontMatch call for some FontConfig bookkeeping.


I don’t quite understand the last sentence. I think the Fc*Substitute calls are required for the FcFontMatch call to work, but what’s the bit about book-keeping?

Would “Without this step, the FcFontMatch call will fail because…” be an accurate description? If so, could you take it and complete it please?

It's pretty much just required to update FontConfig's internal state (I'm still not entirely sure why). I'll change the description.

stapelberg · 2018-01-10T08:30:26Z

unlock_indicator.c

+    }
+
+    /*
+     * Passes the given pattern into cairo, which loads it into a caito freetype font face.


type: caito → cairo

stapelberg · 2018-01-10T08:32:43Z

unlock_indicator.c

+    /*
+     * converts a font face name to a pattern for that face name
+     */
+    FcPattern *pattern = FcNameParse(font_face_name);


As font_face_name is only used once, could you just write "sans-serif" here?

Yeah, I'll just do the unsigned char* cast inline, I guess. I moved it out to a variable to make the code read a bit more cleanly.

stapelberg · 2018-01-10T08:33:49Z

unlock_indicator.c

+            text, -1,
+            &glyphs, &num_glyphs,
+            NULL, NULL, NULL);
+    if (status == CAIRO_STATUS_SUCCESS) {


Let’s inverse this conditional like so:

if (status != CAIRO_STATUS_SUCCESS) { DEBUG(…); return; } cairo_show_glyhps(…); // … `

stapelberg · 2018-01-10T08:34:56Z

unlock_indicator.c

+
+static cairo_font_face_t *get_font_face() {
+    if (cairo_font_face_get_reference_count(sans_serif)) {
+        return cairo_font_face_reference(sans_serif);


Is the reference count increase/decrease actually necessary? There’s just one location where we use the font face.

Not really. I can just change it to if (sans_serif) return sans_serif; pretty much, I guess.

stapelberg · 2018-01-10T08:36:33Z

I’m not entirely sure why the travis check doesn’t complain, but formatting the code with clang-format-3.5 -i *.[ch] shows differences for me. Before pushing, could you please run that command to ensure consistent formatting? Thanks!

no clang-format-3.5 on this machine, so I'll go do that on another

PandorasFox · 2018-01-10T18:21:15Z

Before pushing, could you please run that command to ensure consistent formatting? Thanks!

Should be good now!

stapelberg · 2018-01-11T08:50:28Z

unlock_indicator.c

@@ -80,6 +80,9 @@ static xcb_visualtype_t *vistype;
 unlock_state_t unlock_state;
 auth_state_t auth_state;

+/* Cache the font we use after looking it up once, and reference count it */


The reference count bit is no longer accurate; remove please

stapelberg · 2018-01-11T08:50:43Z

unlock_indicator.c

@@ -80,6 +80,9 @@ static xcb_visualtype_t *vistype;
 unlock_state_t unlock_state;
 auth_state_t auth_state;

+/* Cache the font we use after looking it up once, and reference count it */
+cairo_font_face_t *sans_serif = NULL;


This can be marked static, as it’s not used outside of the file, yes?

stapelberg · 2018-01-11T08:50:55Z

unlock_indicator.c

+ * Reference counts and caches the font-face, so we don't have memory leaks
+ * (and so that we don't have to go through all the parsing each time)
+ */
+


nit: remove newline between comment and function definition

stapelberg · 2018-01-11T08:51:22Z

unlock_indicator.c

+ * (and so that we don't have to go through all the parsing each time)
+ */
+
+static cairo_font_face_t *get_font_face() {


nit: C requires functions taking no parameters to read static cairo_font_face_t *get_font_face(void), otherwise they are interpreted as variadic functions

stapelberg · 2018-01-11T08:52:24Z

unlock_indicator.c

+        DEBUG("config sub failed?\n");
+        return NULL;
+    }
+    /*


nit: add empty line above for consistency

stapelberg · 2018-01-11T08:54:06Z

@psychon, would you have a minute to review this pull request? AFAIR, you’re somewhat familiar with the cairo APIs :) Thanks in advance!

psychon

Basically: I don't see the point of this and would like to see details about the memory leaks that this fixes first.

psychon · 2018-01-11T09:38:40Z

unlock_indicator.c

+                                              x, y,
+                                              text, -1,
+                                              &glyphs, &num_glyphs,
+                                              NULL, NULL, NULL);


I don't know why cairo_show_text is marked as toy API, but cairo_scaled_font_text_to_glyphs is not, but this code is (basically) doing what cairo_show_text would do (well, cairo_show_text uses and updates the current point instead of having an extra parameter for that, but that doesn't really matter, I think). So, I would suggest to keep using cairo_show_text here.

psychon · 2018-01-11T09:45:33Z

unlock_indicator.c

+    FcPatternDestroy(pattern_ready);
+    sans_serif = cairo_font_face_reference(face);
+    return face;
+}


I do not really know "font stuff" well, but why not replace the above function with

if (sans_serif == NULL) sans_serif = cairo_toy_font_face_create("sans-serif", CAIRO_FONT_SLANT_NORMAL, CAIRO_FONT_WEIGHT_NORMAL); return sans_serif;

If "memory leaks" is the reason, then I would rather fix that instead of working around it. I am pretty sure that using the toy font backend should not cause memory leaks. And the reason that the toy font API is marked as toy are AFAIK things like right-to-left text and complicated scripts, not memory leaks.

Doing that'll cause some memory leaks in fontconfig. The only way to avoid them is to make the cairo_debug_reset_static_data call, which can't be used in conjunction with our own caching.

It definitely seems like it's better to just let i3lock handle the fairly simple lookup + caching than to have to flush the static data every draw tick.

These are the specific memory leaks I get from ASAN:

==15286==ERROR: LeakSanitizer: detected memory leaks Direct leak of 256 byte(s) in 1 object(s) allocated from: #0 0x7fbb92b01ae9 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:62 #1 0x7fbb912287ee (/usr/lib/libfontconfig.so.1+0x1d7ee) Indirect leak of 96 byte(s) in 3 object(s) allocated from: #0 0x7fbb92b01ce1 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:70 #1 0x7fbb91228ed9 (/usr/lib/libfontconfig.so.1+0x1ded9) Indirect leak of 11 byte(s) in 1 object(s) allocated from: #0 0x7fbb92a983b1 in __interceptor_strdup /build/gcc/src/gcc/libsanitizer/asan/asan_interceptors.cc:560 #1 0x7fbb912281f5 in FcValueSave (/usr/lib/libfontconfig.so.1+0x1d1f5)

Hm, okay, that's not the most useful backtrace in the world. Still, I would have expected way more leak reports. When I tried to get awesome valgrind-clean, I eventually resorted to suppressions and then gave up.
For example, valgrind pango-view -t "This is a test" finds 2 definitely lost blcoks, 304 indirectly lost and 21 possibly lost. The biggest leak has this backtrace:

==8098== 24,528 bytes in 3 blocks are still reachable in loss record 948 of 948 ==8098== at 0x4C2FB6B: malloc (vg_replace_malloc.c:299) ==8098== by 0x5C29924: ??? (in /usr/lib64/libfontconfig.so.1.10.1) ==8098== by 0x5C43FCD: ??? (in /usr/lib64/libfontconfig.so.1.10.1) ==8098== by 0x7E14037: ??? (in /usr/lib64/libexpat.so.1.6.7) ==8098== by 0x7E14E0B: ??? (in /usr/lib64/libexpat.so.1.6.7) ==8098== by 0x7E12A52: ??? (in /usr/lib64/libexpat.so.1.6.7) ==8098== by 0x7E13714: ??? (in /usr/lib64/libexpat.so.1.6.7) ==8098== by 0x7E172DC: XML_ParseBuffer (in /usr/lib64/libexpat.so.1.6.7) ==8098== by 0x5C42B52: ??? (in /usr/lib64/libfontconfig.so.1.10.1) ==8098== by 0x5C42F85: FcConfigParseAndLoad (in /usr/lib64/libfontconfig.so.1.10.1) ==8098== by 0x5C42FF7: FcConfigParseAndLoad (in /usr/lib64/libfontconfig.so.1.10.1) ==8098== by 0x5C4316D: ??? (in /usr/lib64/libfontconfig.so.1.10.1) ==8098== by 0x7E14037: ??? (in /usr/lib64/libexpat.so.1.6.7) ==8098== by 0x7E14E0B: ??? (in /usr/lib64/libexpat.so.1.6.7) ==8098== by 0x7E12A52: ??? (in /usr/lib64/libexpat.so.1.6.7) ==8098== by 0x7E13714: ??? (in /usr/lib64/libexpat.so.1.6.7) ==8098== by 0x7E172DC: XML_ParseBuffer (in /usr/lib64/libexpat.so.1.6.7) ==8098== by 0x5C42B52: ??? (in /usr/lib64/libfontconfig.so.1.10.1) ==8098== by 0x5C42F85: FcConfigParseAndLoad (in /usr/lib64/libfontconfig.so.1.10.1) ==8098== by 0x5C35D53: ??? (in /usr/lib64/libfontconfig.so.1.10.1) ==8098== by 0x5C35FA5: ??? (in /usr/lib64/libfontconfig.so.1.10.1) ==8098== by 0x5C29A56: ??? (in /usr/lib64/libfontconfig.so.1.10.1) ==8098== by 0x5C2BAE4: FcConfigSubstituteWithPat (in /usr/lib64/libfontconfig.so.1.10.1) ==8098== by 0x54B3C27: ??? (in /usr/lib64/libpangocairo-1.0.so.0.4000.13) ==8098== by 0x5094761: ??? (in /usr/lib64/libpangoft2-1.0.so.0.4000.13) ==8098== by 0x4E54416: ??? (in /usr/lib64/libpango-1.0.so.0.4000.13) ==8098== by 0x4E55B9F: pango_itemize_with_base_dir (in /usr/lib64/libpango-1.0.so.0.4000.13) ==8098== by 0x4E5E55A: ??? (in /usr/lib64/libpango-1.0.so.0.4000.13) ==8098== by 0x4E6042B: ??? (in /usr/lib64/libpango-1.0.so.0.4000.13) ==8098== by 0x4E60900: pango_layout_get_pixel_extents (in /usr/lib64/libpango-1.0.so.0.4000.13) ==8098== by 0x10D609: ??? (in /usr/bin/pango-view) ==8098== by 0x10E1C7: ??? (in /usr/bin/pango-view) ==8098== by 0x110965: ??? (in /usr/bin/pango-view) ==8098== by 0x10CDDE: ??? (in /usr/bin/pango-view) ==8098== by 0x66F7009: (below main) (in /usr/lib64/libc-2.26.so)

The function FcConfigParseAndLoad sounds a lot like it is loading the fontconfig configuration in /etc/fonts (this is just a guess!). So I wonder why you aren't seeing this memory leak nor do you need to call FcFini.

Also, yes you can use cairo_debug_reset_static_data with your own caching, you just have to make sure to clear your own cache before. Actually, your patch does introduce a memory leak since your cache is never cleared. ;-)

Finally, feel free to ignore me. @stapelberg asked for some comments and I left them, but I am not the one to decide what goes into i3lock and what not.

I’m definitely on board with the approach to first try hard and fix the memory leaks in cairo :)

Also, yes you can use cairo_debug_reset_static_data with your own caching, you just have to make sure to clear your own cache before

ah, yeah, but if we've got to clear our cache every tick, it's not much of a cache, then :P

Actually, your patch does introduce a memory leak since your cache is never cleared. ;-)

Yeah, I know. There's no real elegant way of doing this all without either caching nothing (a bit less than ideal) or adding some at-exit cleanup to i3lock (probably way more complicated than needed)

We’d prefer to not have exit cleanups unless absolutely necessary (e.g. to aid leak-checker tools). The OS cleans up when a process exits, that’s good enough.

The OS cleans up when a process exits, that’s good enough.

Which means that a memory leak is only a bug if it increases over time, not if it is a static cache like it is (or at least seems to be) here?

I’d say so, yes. (Again: with the caveat that if a static leak is not recognized as such by a memory leak checker, it might still be good to eliminate it to get a more useful report.)

Yeah, I was pretty much making this pull with the goal of having a clean baseline going forwards.

psychon · 2018-01-11T13:55:18Z

Okay, one last thing:

i3lock: version 2.9.1 (2017-06-21) © 2010 Michael Stapelberg

I'm not sure how many leaks were fixes since this version, but from a quick scroll through the output of valgrind --leak-check=full --show-reachable=yes i3lock, most of the leaks are through libxkbcommon, cairo_xcb_surface_create, xcb internals and dynamic linker internals. However, I did not check everything since the report is way too long:

==8310== LEAK SUMMARY:
==8310==    definitely lost: 256 bytes in 1 blocks
==8310==    indirectly lost: 107 bytes in 4 blocks
==8310==      possibly lost: 9,652 bytes in 113 blocks
==8310==    still reachable: 741,020 bytes in 5,901 blocks
==8310==         suppressed: 0 bytes in 0 blocks

Is it just me, or are ASAN and valgrind reporting wildly different results?

PandorasFox · 2018-01-11T13:59:25Z

Yeah, I get wildly different stuff with ASAN and valgrind, but I think that's due to an ASAN-compiled program running with non-ASAN libs. I was doing some debugging for another project and ended up compiling fontconfig with ASAN and ended up getting much more similar outputs after then.

I'm guessing it's just due to how ASAN gets built into the binary.

psychon · 2018-01-11T14:40:08Z

This patch makes valgrind go from

==15126== LEAK SUMMARY:
==15126==    definitely lost: 256 bytes in 1 blocks
==15126==    indirectly lost: 107 bytes in 4 blocks
==15126==      possibly lost: 9,652 bytes in 113 blocks
==15126==    still reachable: 740,708 bytes in 5,896 blocks
==15126==         suppressed: 0 bytes in 0 blocks

to

==17259== LEAK SUMMARY:
==17259==    definitely lost: 144 bytes in 3 blocks
==17259==    indirectly lost: 0 bytes in 0 blocks
==17259==      possibly lost: 0 bytes in 0 blocks
==17259==    still reachable: 272,496 bytes in 6 blocks
==17259==         suppressed: 0 bytes in 0 blocks

Remaining leaks are:

An actual bug in cairo (cairo_debug_reset_static_data misses one cache which has 262,176 bytes and thus accounts for most of the remaining memory; thanks for making me spot this).
Weird stuff called from _dl_init which I will just ignore.
One timer that is still pending and cannot easily be freed (see patch; this is also where the definite lost comes from).

I'm not quite sure what my point here is, but it should be something like "there is no actual memory leak in cairo". I don't know why ASAN manages to find some leaks, but ignores all the other ones that valgrind finds, but... well, perhaps it is a good idea to figure out why ASAN only complains about some particular leak before adding 100 lines of code to make ASAN stop complaining.

stapelberg · 2018-10-16T06:38:14Z

@PandorasFox Sorry for letting this PR linger for so long. Could you resolve the conflicts please? We can go ahead with merging it then.

PandorasFox · 2018-10-16T15:07:39Z

I'll look into that later. I've been meaning to dig into the fontconfig errors some more and figure out if there's really a consistent way to resolve its errors (or if there's been a subsequent fontconfig release that's fixed them).

I noticed this commit, along with several others (checking the 2.13.1 changelog, I'm seeing a lot of commits that should fix the warnings/errors that I was seeing before) and am unsure if the rewrite/extra complexity is really needed anymore.

PandorasFox · 2018-10-26T21:42:27Z

I'm currently on a long flight and my laptop died shortly after I pushed that rebase. I'll do more testing on this later this weekend.

PandorasFox · 2018-10-27T17:38:55Z

I built fontconfig with -g -fsanitize=address -fno-omit-frame-pointer from this commit. When I ran the current i3lock master pulling in my local fontconfig build, which produced no ASAN output:

LD_PRELOAD=/usr/lib/libasan.so:/usr/local/lib/libfontconfig.so i3lock --nofork -> no output. Running i3lock --nofork (using Arch's fontconfig 2.13.1+12+g5f5ec56-1) produces leak warnings.

When running my fork vs the current master against Arch's fontconfig, the current master produces some ASAN leak warnings in the older fontconfig:

=================================================================
==22665==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 256 byte(s) in 1 object(s) allocated from:
    #0 0x7f7b94b2f019 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:86
    #1 0x7f7b92e40345  (/usr/lib/libfontconfig.so.1+0x21345)

Indirect leak of 96 byte(s) in 3 object(s) allocated from:
    #0 0x7f7b94b2f231 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:95
    #1 0x7f7b92e40989  (/usr/lib/libfontconfig.so.1+0x21989)

Indirect leak of 11 byte(s) in 1 object(s) allocated from:
    #0 0x7f7b94a77f01 in __interceptor_strdup /build/gcc/src/gcc/libsanitizer/asan/asan_interceptors.cc:405
    #1 0x7f7b92e3fc25 in FcValueSave (/usr/lib/libfontconfig.so.1+0x20c25)

SUMMARY: AddressSanitizer: 363 byte(s) leaked in 5 allocation(s).

So - it definitely seems that the overall issue was with fontconfig and this pull only really resolves the symptoms of it. I don't really feel that this should be necessary for correctness on i3lock's part, but I guess it might still be nice to have so that font handling is broken out and a bit easier to audit (maybe?) instead of relying on cairo functions. I'm personally leaning towards closing this.

stapelberg · 2018-10-29T16:51:21Z

So - it definitely seems that the overall issue was with fontconfig and this pull only really resolves the symptoms of it. I don't really feel that this should be necessary for correctness on i3lock's part

Let’s close this, then. The less code we have, the better :).

Thanks for looking into this!

Move to use cairo glyphs instead of text

0a03012

Requires linking fontconfig now, but that was already required at runtime for cairo anyways.

PandorasFox mentioned this pull request Dec 9, 2017

memleak in cairo_select_font_face #167

Closed

PandorasFox mentioned this pull request Dec 30, 2017

Support to set font for verification text Raymo111/i3lock-color#60

Closed

Airblader reviewed Jan 9, 2018

View reviewed changes

Airblader requested a review from stapelberg January 9, 2018 08:42

stapelberg reviewed Jan 9, 2018

View reviewed changes

add comments; tweak some naming

393bdce

remove trailing space

d7b0cf5

stapelberg reviewed Jan 10, 2018

View reviewed changes

update comments / style

a5a5a92

no clang-format-3.5 on this machine, so I'll go do that on another

stapelberg reviewed Jan 11, 2018

View reviewed changes

psychon suggested changes Jan 11, 2018

View reviewed changes

tweak draw_text, comments/whitespace/typing

a5ab346

PandorasFox mentioned this pull request Feb 1, 2018

LeakSanitizer: detected memory leaks Raymo111/i3lock-color#70

Closed

add missing FcFini call

aa0066a

rebase to master

3edb9be

clang-format

e8dae3d

stapelberg closed this Oct 29, 2018

Raymo111 deleted the vanilla branch May 12, 2020 20:37

Move to use cairo glyphs instead of text #168

Move to use cairo glyphs instead of text #168

Conversation

PandorasFox commented Dec 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Airblader commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PandorasFox Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

stapelberg commented Jan 9, 2018

Airblader commented Jan 9, 2018

PandorasFox commented Jan 9, 2018

PandorasFox commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stapelberg commented Jan 10, 2018

PandorasFox commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stapelberg commented Jan 11, 2018

psychon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psychon commented Jan 11, 2018

PandorasFox commented Jan 11, 2018

psychon commented Jan 11, 2018

stapelberg commented Oct 16, 2018

PandorasFox commented Oct 16, 2018 • edited Loading

PandorasFox commented Oct 26, 2018

PandorasFox commented Oct 27, 2018 • edited Loading

stapelberg commented Oct 29, 2018

PandorasFox Jan 9, 2018 •

edited

Loading

PandorasFox commented Oct 16, 2018 •

edited

Loading

PandorasFox commented Oct 27, 2018 •

edited

Loading