Audio: Fix pointer arithmetic mistake in ASRC initialize #3254

singalsu · 2020-07-29T16:13:11Z

This pull request consists of two commits, see invidual descriptions

Fix for illegal and failed pointer arithmetic
Improve error codes enums documentation readability with longer lines

singalsu · 2020-07-29T16:14:38Z

I have not confirmed but previous code might have been OK in 32 bit xtensa. Testbench is 64 bits in 64 bit OS.

marc-hb · 2020-07-29T21:40:35Z

Casting (uint32_t **) to (uint32_t *) seemed very wrong before and it still seems wrong in this PR.

I don't really understand the code nor the comment but would something like src_obj->ring_buffers32[num*(1+length)] make sense? It would be cast-free and much more readable.

Please also add a Fixes: SHA1 so the bug can be dated.

The https://sof-ci.01.org/sofpr/PR3254/build6755/devicetest/ error looks like this:

2020-07-29 16:35:50 UTC [REMOTE_COMMAND] alsabat -Pplughw:0,0 --standalone -n 240000 -F 997
2020-07-29 16:35:51 UTC [REMOTE_COMMAND] alsabat -Cplughw:1,0 -F 997
 FAIL: Peak freq too high 1993.84 Hz
alsa-utils version 1.2.2

singalsu · 2020-07-30T11:11:06Z

Using this gives a segfault for test run

		src_obj->impulse_response = src_obj->ring_buffers32[
			src_obj->num_channels * (1 + src_obj->buffer_length)];

The memory layout is like this

| ring_buffers32[], array of pointers to buffer for each channel | channel buffers (16 or 32 bit) | impulse_response[] (32 bit) |

Would this be OK?

	if (src_obj->bit_depth == 32) {
		src_obj->impulse_response = 
			(int32_t *)(&src_obj->ring_buffers32[src_obj->num_channels]) +
			src_obj->num_channels * src_obj->buffer_length;
	} else if (src_obj->bit_depth == 16) {
		src_obj->impulse_response =
			(int32_t *)(&src_obj->ring_buffers32[src_obj->num_channels]) +
			src_obj->num_channels * src_obj->buffer_length / 2;
	}

Umm, I don't get how the devicetest is related. I doubt there's a test for ASRC.

paulstelian97

I feel like a more serious fix is required. I have also noticed the cast from (int32_t **) to (int32_t *) which is a recipe for disaster. This PR simply doesn't fix that. What's the logic behind that?

I want a VERY CLEAR explanation on why this cast is needed before I allow merging. What's the structure of ring_buffers32/ring_buffers16?

singalsu · 2020-07-30T11:30:14Z

@marc-hb @paulstelian97 Thanks for the attention! This is not originally my code and I would like to totally rewrite the data I/O part. It wastes RAM by writing each sample duplicated to FIR delay lines to avoid circular wrap checks when computing filters. Instead with xtensa the circular access SIMD are more efficient. ASRC is a memory hog so eventually it must be done. But I'd like to fix the currently known issues before doing that.

If you look asrc_farrow_generic.c function asrc_fir_filter32() you see how each channel filter delay line is read. The current xtensa SIMD version code is similar, but this is easier to read:

	/* Iterate over each channel */
	for (ch = 0; ch < src_obj->num_channels; ch++) {
		/* Pointer to the beginning of the impulse response */
		filter_p = &src_obj->impulse_response[0];

		/* Pointer to the buffered input data */
		buffer_p = &src_obj->ring_buffers32[ch]
			[src_obj->buffer_write_position];

		/* Initialise the accumulator */
		prod = 0;

		/* Iterate over the filter bins. Data is Q1.31, coefficients
		 * are Q1.22. They are down scaled by 1 shift. In addition
		 * there C is implementation specific right shift by 8. It
		 * gives headroom to calculate up to 256 taps FIR. The use
		 * of 24 bits of 32 bits is not a practical limitation for
		 * quality. The product is Qx.54.
		 */
		for (n = 0; n < src_obj->filter_length; n++)
			prod += (int64_t)(*buffer_p--) * (*filter_p++ >> 8);

		/* Shift left after accumulation, because interim
		 * results might saturate during filtering prod = prod
		 * << 1; will shift after last addition
		 */
		prod32 = sat_int32(Q_SHIFT(prod, 53, 31));

		/* Store 'prod' in (de-)interleaved format in the output
		 * buffers
		 */
		output_buffers[ch][i] = prod32;
	}

So there the start address is retrieved from C two dimensional array and stepped with (int32_t *) pointer arithmetic. It is initialized for varying dimensions by assumption of knowing how C internally stores two dimensional arrays.

paulstelian97 · 2020-07-30T11:40:58Z

@marc-hb @paulstelian97 Thanks for the attention! This is not originally my code and I would like to totally rewrite the data I/O part. It wastes RAM by writing each sample duplicated to FIR delay lines to avoid circular wrap checks when computing filters. Instead with xtensa the circular access SIMD are more efficient. ASRC is a memory hog so eventually it must be done. But I'd like to fix the currently known issues before doing that.

If you look asrc_farrow_generic.c function asrc_fir_filter32() you see how each channel filter delay line is read. The current xtensa SIMD version code is similar, but this is easier to read:
	/* Iterate over each channel */
	for (ch = 0; ch < src_obj->num_channels; ch++) {
		/* Pointer to the beginning of the impulse response */
		filter_p = &src_obj->impulse_response[0];

		/* Pointer to the buffered input data */
		buffer_p = &src_obj->ring_buffers32[ch]
			[src_obj->buffer_write_position];

		/* Initialise the accumulator */
		prod = 0;

		/* Iterate over the filter bins. Data is Q1.31, coefficients
		 * are Q1.22. They are down scaled by 1 shift. In addition
		 * there C is implementation specific right shift by 8. It
		 * gives headroom to calculate up to 256 taps FIR. The use
		 * of 24 bits of 32 bits is not a practical limitation for
		 * quality. The product is Qx.54.
		 */
		for (n = 0; n < src_obj->filter_length; n++)
			prod += (int64_t)(*buffer_p--) * (*filter_p++ >> 8);

		/* Shift left after accumulation, because interim
		 * results might saturate during filtering prod = prod
		 * << 1; will shift after last addition
		 */
		prod32 = sat_int32(Q_SHIFT(prod, 53, 31));

		/* Store 'prod' in (de-)interleaved format in the output
		 * buffers
		 */
		output_buffers[ch][i] = prod32;
	}
So there the start address is retrieved from C two dimensional array and stepped with (int32_t *) pointer arithmetic. It is initialized for varying dimensions by assumption of knowing how C internally stores two dimensional arrays.

Wait, so ring_buffers32 is (int[something] *) or (int **)? These have different semantics in C so I still don't get it. I think the (int **) itself may be in the wrong though. When you say (int **) that is INCOMPATIBLE with the C arrangement of two-dimensional arrays. You instead get an array of pointers, each of the pointers itself leading to an array. But that is not a two-dimensional array, an array of arrays.

Sounds extremely broken to me.

singalsu · 2020-07-30T11:47:07Z

@paulstelian97 Yep, this has been all the time hard for me debug. I'll rewrite this PR more with cleaner initialization of the two dimensional array.

I will eventually replace the whole array with channel interleaved buffer like SOF normally uses but I want to make a working intermediate step as reference for RAM & more SIMD parallelized speed optimized implementation. I'll fix the stress test glitches issue first. Unfortunately this fix didn't help with that, there's still something else.

singalsu · 2020-07-30T12:58:37Z

The buffer length is always even (but I'll add check for it anyway because I will get rid of doubled data). For this part of code, is this acceptable?

	if (src_obj->bit_depth == 32) {
		src_obj->impulse_response = &src_obj->ring_buffers32[src_obj->num_channels - 1][src_obj->buffer_length - 1] + 1;
	} else if (src_obj->bit_depth == 16) {
		p16 = &src_obj->ring_buffers16[src_obj->num_channels - 1][src_obj->buffer_length  - 1] + 1;
		src_obj->impulse_response = (int32_t *)p16;
        }

If yes, I will follow in other init code C99 convention of setting up the 2-dimensional arrays for 16 and 32 bit data.

singalsu · 2020-07-30T13:19:45Z

There's examples of this in https://www.geeksforgeeks.org/dynamically-allocate-2d-array-c/ . The current code like example 4, at least that's the intent. I feel you'd prefer example 2 but with single allocation.

In doing that there's a problem in struct asrc_farrow in asrc_farrow.h. The would be two flexible array members. But I would be able to fix one dimension to PLATFORM_MAX_CHANNELS like this.

struct asrc_farrow {
	/* IO + ring_buffer data */
	int num_channels;	/*!< Number of channels processed */
				/*!< simultaneously */
	int buffer_length;	/*!< Length of the ring buffer for each */
				/*!< channel */
	int buffer_write_position;	/*!< Position of the ring buffer */
					/*!< to which will be written next */
	int32_t *ring_buffers32[PLATFORM_MAX_CHANNELS];
	int16_t *ring_buffers16[PLATFORM_MAX_CHANNELS];
        ...
}

The RAM for e.g. channels 3..8 would be not allocated if stream is stereo. Would such data stucture be OK @paulstelian97 ?

paulstelian97 · 2020-07-30T13:21:02Z

There's examples of this in https://www.geeksforgeeks.org/dynamically-allocate-2d-array-c/ . The current code like example 4, at least that's the intent. I feel you'd prefer example 2 but with single allocation.

In doing that there's a problem in struct asrc_farrow in asrc_farrow.h. The would be two flexible array members. But I would be able to fix one dimension to PLATFORM_MAX_CHANNELS like this.
struct asrc_farrow {
	/* IO + ring_buffer data */
	int num_channels;	/*!< Number of channels processed */
				/*!< simultaneously */
	int buffer_length;	/*!< Length of the ring buffer for each */
				/*!< channel */
	int buffer_write_position;	/*!< Position of the ring buffer */
					/*!< to which will be written next */
	int32_t *ring_buffers32[PLATFORM_MAX_CHANNELS];
	int16_t *ring_buffers16[PLATFORM_MAX_CHANNELS];
        ...
}
The RAM for e.g. channels 3..8 would be not allocated if stream is stereo. Would such data stucture be OK @paulstelian97 ?

Is this the existing structure or a new one? Either way it's fine. It's just that ring_buffers32 CANNOT correctly be cast to (int32_t **). Simple as that. ALTHOUGH you would automatically always allocate 8 channels by this allocation so you might want to reconsider. So it would be false that "the RAM for channels 3..8 would not be allocated if the stream is stereo" -- I get the intent but it won't work as such.

I'd say: fine for a quick fix except for the memory-constrained platforms, but needs improvements in the future.

singalsu · 2020-07-30T13:40:55Z

Only the header of the array would be full array of 8. The pointers to columns (or rows?) would remain NULL for non-used channels. So the entire 8 * N matrix would not be allocated.

But since I don't need cast to (int32_t **) wouldn't the above example be OK? That wouldn't increase RAM need from current.

	if (src_obj->bit_depth == 32) {
		src_obj->impulse_response =
			&src_obj->ring_buffers32[src_obj->num_channels - 1]
				[src_obj->buffer_length - 1] + 1;
	} else if (src_obj->bit_depth == 16) {
		p16 = &src_obj->ring_buffers16[src_obj->num_channels - 1]
				[src_obj->buffer_length  - 1] + 1;
		src_obj->impulse_response = (int32_t *)p16;
	}

It compiled an ran without problems when defined as this (similarly as in example 4 in the above mentioned web page).

struct asrc_farrow {
	/* IO + ring_buffer data */
	int num_channels;	/*!< Number of channels processed */
				/*!< simultaneously */
	int buffer_length;	/*!< Length of the ring buffer for each */
				/*!< channel */
	int buffer_write_position;	/*!< Position of the ring buffer */
					/*!< to which will be written next */
	int32_t **ring_buffers32;	/*!< Pointer to the pointers to the */
					/*!< 32 bit ring buffers for each */
					/*!< channel */
	int16_t **ring_buffers16;	/*!< Pointer to the pointers to the */
					/*!< 16 bit ring buffers for each */
					/*!< channel */
       ...
}

paulstelian97 · 2020-07-30T13:50:34Z

Only the header of the array would be full array of 8. The pointers to columns (or rows?) would remain NULL for non-used channels. So the entire 8 * N matrix would not be allocated.

But since I don't need cast to (int32_t **) wouldn't the above example be OK? That wouldn't increase RAM need from current.
	if (src_obj->bit_depth == 32) {
		src_obj->impulse_response =
			&src_obj->ring_buffers32[src_obj->num_channels - 1]
				[src_obj->buffer_length - 1] + 1;
	} else if (src_obj->bit_depth == 16) {
		p16 = &src_obj->ring_buffers16[src_obj->num_channels - 1]
				[src_obj->buffer_length  - 1] + 1;
		src_obj->impulse_response = (int32_t *)p16;
	}
It compiled an ran without problems when defined as this (similarly as in example 4 in the above mentioned web page).
struct asrc_farrow {
	/* IO + ring_buffer data */
	int num_channels;	/*!< Number of channels processed */
				/*!< simultaneously */
	int buffer_length;	/*!< Length of the ring buffer for each */
				/*!< channel */
	int buffer_write_position;	/*!< Position of the ring buffer */
					/*!< to which will be written next */
	int32_t **ring_buffers32;	/*!< Pointer to the pointers to the */
					/*!< 32 bit ring buffers for each */
					/*!< channel */
	int16_t **ring_buffers16;	/*!< Pointer to the pointers to the */
					/*!< 16 bit ring buffers for each */
					/*!< channel */
       ...
}

OK, now that's a way to go. You can allocate a single large buffer and split it up into 8 distinct linear buffers, 1 per channel. Or fewer if fewer channels are required. Might want to put in some asserts just in case you accidentally try to use a channel that is unset (channel 4 in stereo, for example). Or would a null pointer dereference do the trick in that situation?

Let's see the actual code now.

marc-hb · 2020-07-30T22:50:47Z

But since I don't need cast to (int32_t **) wouldn't the above example be OK?

Anything cast-free and using array indexes [] instead of + signs for... arrays (!) is automatically better :-)

src/audio/asrc/asrc_farrow.c

mmaka1 · 2020-07-31T16:36:31Z

src/audio/asrc/asrc_farrow.c

It is always even atm since computed as src_obj->filter_length * 2 by initialise_buffer(). Should this check be moved to initialise_buffer() instead, as an assert() with a comment that change to the formula would result in unaligned pointer later?

I have in plans to remove the duplicated data delay line so I added this check to not forget it. But you are right it's unnecessary now. I can remove it since you mentioned.

src/audio/asrc/asrc_farrow.c

singalsu · 2020-08-03T11:18:01Z

@mmaka That looks cleaner, thanks. Though there's an additional vector of pointers for dynamic 2D array initialize. I could also reorder the impulse response and ring buffers location to not have bit depth in the pointers calculate code.

I'll add the check vs. 128 and 256, good point. I can later optimize the sizes.

singalsu · 2020-08-04T10:01:55Z

Thanks for all the feedback @marc-hb @lyakh @mmaka1 @paulstelian97 , here's a new version that works and passes valgrind. Is it like you recommended?

paulstelian97 · 2020-08-04T10:32:00Z

src/audio/asrc/asrc_farrow.c

Wait, how does this work? There is a bit of a reserved region of size filter_length before the first of the buffers? Also, 2D array is incompatible with (int32_t **)

I moved the filter coefficients array to be before buffers32/16. This way I don't need to take into account the variable word length of buffers when computing next array location. The buffers32/16 is the last part of the allocated blob.

As far as I understand a dynamic size 2D array needs to use (int32_t **) pointer.

Dynamic sized 2D array isn't a continuous memory region unlike statically allocated 2D arrays. Just to be aware of that again.

Of course, when the lower size varies you sure can only go the array-of-pointers with each pointer being an individual array way, and if you want allocate the individual arrays continuously in a big chunk -- but you still need a separate memory region to hold the pointers.

Yes I know. The array of pointers to each row data allows that. However in this case the memory is allocated continuously for simplicity. I'm not aware of ways to do this in C in much cleaner way. We discussed the possibility to fix array first dimension to SOF max channels for simpler initialization but it would consume more RAM.

Now the channel # form the array rows and the channel samples are the columns for "src_obj->ring_buffers32[ch][frame]". The memory region to hold the pointers to column index zeros is in the beginning of the memory area for this "dynamic 2D array". This can be seen in function initialise_buffer().

Actually now I think there's a mistake in asrc_get_required_size(). When computing "size += ASRC_MAX_BUFFER_LENGTH * num_channels * (bit_depth / 8);" the array of pointers to colums data is missing. I think if should be num_channels * sizeof(int32 **). The bug has not triggered because the used filters are significantly shorter than MAX. Do you agree?

Sorry, no mistake, there's the "size += sizeof(int32_t *) * num_channels;". It holds the pointers in beginning of ring_buffers32/16 the pointers to columns data. So this proposal should be OK.

I'd say the array of pointers should have MAX, but null out any channels between the actual available ones and the max. So for stereo, ring_buffers32[0] and ring_buffers32[1] are valid, while ring_buffers32[2] through ring_buffers32[7] are reliably null.

It would change the initialization and consume more allocated RAM. I don't think the stream channels count could change between prepare() and copy(). So such channels could increase should not happen. Also the channel count for ASRC is initialized only in prepare(), it's not monitored/updated in copy(). Access to more than initialized would require cd->asrc_obj corruption.

Changing an allocation from 8 to 32 bytes? FWIW I don't think there are blocks in the heap smaller than 32 bytes in most memory configurations anyway. So for the array of pointers you can go ahead an allocate with [MAX] anyway, to protect against mistakes that escape review. Not gonna block on this though.

The ASRC object allocation goes to 2 kB or 4 kB pool usually. This is relatively small in that amount yes. I'll do the next version with channels max fixed length for pointers if keep the channel specific buffers. If channel interleaved (as SRC), no need for such.

src/audio/asrc/asrc_farrow.c

lyakh · 2020-08-04T10:48:31Z

src/audio/asrc/asrc_farrow.c

Today I learned something new about C... And it's even in K&R and I wasn't aware of it. Here's the thing: &(a[1]) - &(a[0]) == 1 for both int a[2]; and int *a; and in both cases it's known at compile-time. However &(a[0][1]) - &(a[0][0]) is also valid for both int a[2][3]; and e.g. int *a[2]; but - for the former it's also known at compile time and is always 1 whereas for the latter it depends on values of a[0] and a[1] and consequently only can be calculated at run-time... Maybe it's obvious to everyone here except me, then we can certainly use this code, but if I'm not the only one who finds it... confusing - to put it mildly - maybe we want to make the code a bit clearer about what exactly we mean :-)

@lyakh How does it depend on a[1] when both values are read from a[0]?

@paulstelian97 yes, sorry, see - it's that confusing (for me) :-) Make that &(a[1][0]) - &(a[0][0]);

For that one yeah, it is statically known for 2D arrays but determined as an actual runtime computation for jagged arrays (double pointer).

Please share your proposal how to make the code a bit clearer.

(I'd like to do a rewrite the data I/O part as this wastes some RAM but I'll do after I've managed to fix the stress test glitches issue. I would remove the "2D" array and use normal channel interleaved circular buffers for FIR as I use in other components like EQ and SRC).

@singalsu as long as you use a dynamically allocated 2D array like int **a; or int *a[]; and you refer to individual elements using a[i][j] there doesn't seem to be much choice - C wants to have an array of pointers exactly like it's done now. If you can do that as an interleaved array and it would be easier to work with - that could be good, but if you're also fine with the current implementation - it's correct too, basically it's just me who wasn't aware of this C... peculiarity...

I'd like get this simple fix merged unless there are still violations against good practices. I don't promise when but I will do the rewrite.

If it would be just for MCPS improve it would be low priority (current speed should be quite acceptable) but the RAM usage is much more than needed. It's not a large task after there's a stable version that passes the testing. I have now fix PR for 2 of 3 different type know bug. The third is under work. If that fixes remaining issues the rewrite can be done.

This patch fixes the issue where impulse response update can randomly corrupt stack in testbench environment. The error seen in freeing cd->asrc_obj was free(): invalid next size (normal). The violation was also confirmed with valgrind with error Invalid write of size 4. The order of impulse response storage and channel buffers are swapped to simplify the pointer initialize calculations. The default sizes for them are added as macro definitions into asrc_farrow.h. Checks for used sizes are added to buffer and filter initialize functions. The clear of buffer is done with memset instead of for loop. Fixes: 4c995d0 Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

src/include/sof/audio/asrc/asrc_farrow.h

paulstelian97

For the most part I think this one is alright and, unless it breaks something critical (does the CI work? I haven't checked...) this can be merged as-is.

singalsu · 2020-08-07T11:11:28Z

I'll update the comment, don't merge yet.

paulstelian97 · 2020-08-07T11:25:49Z

I'll update the comment, don't merge yet.

I myself don't have merge perms anyway :) Guess when the second approval comes through.

The 80 characters split lines were hard to read. This change improves readability error codes enums. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu · 2020-08-07T11:30:37Z

I'll update the comment, don't merge yet.

I myself don't have merge perms anyway :) Guess when the second approval comes through.

I added second commit with longer lines for those comment. Please review it too!

lgirdwood · 2020-08-10T12:58:21Z

Jenkins CI DUT failure.

singalsu requested a review from lgirdwood July 29, 2020 16:13

singalsu requested review from dbaluta, jajanusz, lbetlej, mmaka1, plbossart and tlauda as code owners July 29, 2020 16:13

marc-hb requested a review from lyakh July 29, 2020 21:08

paulstelian97 requested changes Jul 30, 2020

View reviewed changes

singalsu force-pushed the asrc_fix_initialize_impulse_response branch from 3462ab2 to de096f4 Compare July 30, 2020 17:24

marc-hb reviewed Jul 30, 2020

View reviewed changes

src/audio/asrc/asrc_farrow.c Outdated Show resolved Hide resolved

mmaka1 requested changes Jul 31, 2020

View reviewed changes

lyakh requested changes Aug 3, 2020

View reviewed changes

src/audio/asrc/asrc_farrow.c Outdated Show resolved Hide resolved

singalsu force-pushed the asrc_fix_initialize_impulse_response branch from de096f4 to dd18969 Compare August 4, 2020 09:55

singalsu requested review from lyakh, marc-hb and paulstelian97 August 4, 2020 09:58

singalsu requested a review from mmaka1 August 4, 2020 09:58

paulstelian97 reviewed Aug 4, 2020

View reviewed changes

lyakh reviewed Aug 4, 2020

View reviewed changes

singalsu force-pushed the asrc_fix_initialize_impulse_response branch from dd18969 to f93f519 Compare August 7, 2020 11:05

singalsu requested review from lyakh and paulstelian97 August 7, 2020 11:09

paulstelian97 reviewed Aug 7, 2020

View reviewed changes

src/include/sof/audio/asrc/asrc_farrow.h Outdated Show resolved Hide resolved

singalsu requested a review from paulstelian97 August 7, 2020 11:09

paulstelian97 approved these changes Aug 7, 2020

View reviewed changes

Audio: ASRC: Reformat header file comments with longer lines

c256109

The 80 characters split lines were hard to read. This change improves readability error codes enums. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

lgirdwood approved these changes Aug 7, 2020

View reviewed changes

lgirdwood merged commit 56c29b4 into thesofproject:master Aug 10, 2020

singalsu deleted the asrc_fix_initialize_impulse_response branch August 26, 2022 11:43

Audio: Fix pointer arithmetic mistake in ASRC initialize #3254

Audio: Fix pointer arithmetic mistake in ASRC initialize #3254

Uh oh!

Conversation

singalsu commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singalsu commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singalsu commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulstelian97 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singalsu commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulstelian97 commented Jul 30, 2020

Uh oh!

singalsu commented Jul 30, 2020

Uh oh!

singalsu commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singalsu commented Jul 30, 2020

Uh oh!

paulstelian97 commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singalsu commented Jul 30, 2020

Uh oh!

paulstelian97 commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

singalsu commented Aug 3, 2020

Uh oh!

singalsu commented Aug 4, 2020

Uh oh!

paulstelian97 Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulstelian97 Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

singalsu commented Jul 29, 2020 •

edited

Loading

singalsu commented Jul 29, 2020 •

edited

Loading

marc-hb commented Jul 29, 2020 •

edited

Loading

singalsu commented Jul 30, 2020 •

edited

Loading

paulstelian97 left a comment •

edited

Loading

singalsu commented Jul 30, 2020 •

edited

Loading

singalsu commented Jul 30, 2020 •

edited

Loading

paulstelian97 commented Jul 30, 2020 •

edited

Loading

paulstelian97 commented Jul 30, 2020 •

edited

Loading

marc-hb commented Jul 30, 2020 •

edited

Loading

paulstelian97 Aug 4, 2020 •

edited

Loading

paulstelian97 Aug 7, 2020 •

edited

Loading