Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use an actual Generator for WaveForm #12686

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
+32 −45
Draft

Use an actual Generator for WaveForm #12686

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c5a4ff
refactor(WaveForm): use an actual Generator
mxdvl Oct 25, 2024
16dffbf
chore: merge branch 'upstream/main' locally
mxdvl Oct 25, 2024
8a8baa7
docs: keep mostly as-is
mxdvl Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 32 additions & 45 deletions dotcom-rendering/src/components/AudioPlayer/components/WaveForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,57 +11,41 @@ import { useId, useMemo } from 'react';
* the same results, given the same seed.
*
* Copilot helped me with it...
*
* @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Generator
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would be to directly return an array like so:

const waveformGenerator = (
	/** the URL of the audio source file */
	url: string,
	/** the number of bars to generate, i.e. the length of the array */
	bars: number,
	/** a destination range for values */
	[min, max]: [number, number],
) => {
	const modulus = 2147483648;
	/** a sort of hash of the URL */
	const seed = 0xfab; // omitted for brevity
	const multiplier = 1103515245;
	const increment = 12345;

	let state = seed;
	const array: number[] = [];
	while (array.length < bars) {
		state = (multiplier * state + increment) % modulus;
		/** Get a number in the range [0, 1] */
		const normalised = state / modulus;
		/** Compress the amplitude of the fake audio data, like a podcast would */
		const compressed = min + (max - min) * normalised;
		/** sub-pixel precision is pointless data to send over the wire */
		const rounded = Math.round(compressed);

		array.push(rounded);
	}
	return array;
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach, generators v cool ofc, but this approach seems more in line with https://github.com/guardian/recommendations/blob/d7ff620f81a6f59758aded4d11b69965858c7fac/coding-with-empathy.md

*/
const getSeededRandomNumberGenerator = (seedString: string) => {
function* waveformGenerator(
/** the URL of the audio source file */
url: string,
/** the number of bars to generate, i.e. the length of the array */
bars: number,
/** a destination range for values */
[min, max]: [number, number],
) {
const modulus = 2147483648;
/** a sort of hash of the URL */
const seed = url
.split('')
.reduce(
(sum, character) => (sum + character.charCodeAt(0)) % modulus,
0,
);
const multiplier = 1103515245;
const increment = 12345;

// convert string to numerical seed
let hashedSeed = 0;
for (let i = 0; i < seedString.length; i++) {
const char = seedString.charCodeAt(i);
hashedSeed = (hashedSeed << 5) - hashedSeed + char;
hashedSeed |= 0; // Convert to 32bit integer
}

const seed = Math.abs(hashedSeed) % modulus;
let state = seed;

return function () {
let count = 0;
while (count++ < bars) {
state = (multiplier * state + increment) % modulus;
return state / modulus;
};
};

/**
* Compresses an of values to a range between the threshold and the existing
* maximum.
*/
const compress = (array: number[], threshold: number) => {
const minValue = Math.min(...array);
const maxValue = Math.max(...array);

return array.map(
(value) =>
((value - minValue) / (maxValue - minValue)) *
(maxValue - threshold) +
threshold,
);
};

// Generate an array of fake audio peaks based on the URL
function generateWaveform(url: string, bars: number) {
const getSeededRandomNumber = getSeededRandomNumberGenerator(url);

// Generate an array of fake peaks, pseudo random numbers seeded by the URL
const peaks = Array.from(
{ length: bars },
() => getSeededRandomNumber() * 100,
);

// Return the compressed fake audio data (like a podcast would be)
return compress(peaks, 60);
/** Get a number in the range [0, 1] */
const normalised = state / modulus;
/** Compress the amplitude of the fake audio data, like a podcast would */
const compressed = min + (max - min) * normalised;
/** sub-pixel precision is pointless data to send over the wire */
const rounded = Math.round(compressed);

yield rounded;
}
Comment on lines +17 to +48
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might be a personal thing, but although this makes for less code, i think it actually makes it harder to read and understand what's going on.

a pattern like this:

const complexFunctionA => () => {}

const complexFunctionB => () => {}

const c => () => {
	const a = complexFunctionA();
	const b = complexFunctionB();

	return a + b;
}

allows for progressive disclosure of the complexity.

it reduces context you need to make a change, because you can focus on the bit you're interested in right now, and only need to dig into specifics (implementations) if you need to (e.g. if, in this case, compression wasn't working correctly).

pulling everything into a single function means you need to understand how everything in there works before you know what you can safely ignore and what you need to look at.

it's a balance for sure (add(a, b) isn't really much clearer than a + b), and maybe it's easier if you're familiar with the code (everything's colocated), but most of the time i feel like we're looking at unfamiliar code – you've never seen it before, it's changed since you last saw it, it's been a while etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting insight — so the complexity of the PRNG “pollutes” the simplicity of the other operations: normalise & remap.

I think the bit that threw me off in the beginning was that the pseudorandom numbers were not in the [0,1) range. Maybe that's all that would be needed here?

In any case I'm happy to leave this in draft or even close it and let the actual team looking after decide on whether these are the right patterns. I just find generators powerful and appropriate to model these kinds of things, and wanted to share an actual example where they might help ❤️

Copy link
Contributor Author

@mxdvl mxdvl Oct 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thought, would function hoisting help with progressive disclosure?

If someone comes to the current module, the first piece of code they’ll encounter is actually the most complex, so you could opt instead for keeping those lower down the file. I also think (and tested) that as long as the functions are not called they could use the const declaration and not suffer from the temporal dead zone.

const generateWaveform = (url: string, bars: number) => {
	const seed = seedFromUrl(url);

	/** the first few bars are consistent with podcasts starting with the same intro */
	const waveform = [72, 84, 96, 84, 96];
	for (const pseudoRanomNumber of PRNG(seed)) {
		/**
		 * Convert from the [0, 1) range to [60,100].
		 * Starting at 60 simulates audio compression.
		 */
		const remapped = 60 + Math.round(pseudoRanomNumber * 40);
		waveform.push(remapped);
		if (waveform.length >= bars) return waveform;
	}
};

/** Convert a string to a numeric hash. Do not use for cryptographic purposes */
const seedFromUrl = (url: string) =>
	url.split('').reduce((sum, character) => sum + character.charCodeAt(0), 0);

/** Pseudo-random number generator of numbers in range [0, 1) */
function* PRNG(seed: number) {
	const modulus = 2147483648;
	/** a sort of hash of the URL */
	const multiplier = 1103515245;
	const increment = 12345;

	let state = seed % modulus;
	while (true) {
		state = (multiplier * state + increment) % modulus;
		const normalised = state / modulus;
		yield normalised;
	}
}

Note

The above code does not work with the current target according to TS:
Type 'Generator<number, void, unknown>' can only be iterated through when using the '--downlevelIteration' flag or with a '--target' of 'es2015' or higher.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the bit that threw me off in the beginning was that the pseudorandom numbers were not in the [0,1) range. Maybe that's all that would be needed here?

yeah the peril of (me) relying on machines. but this conversation (and continued offline) did lead me to https://github.com/dworthen/prng. i'm thinking there's a good case to add something from this (alea?) to @guardian/libs and use that here instead.

this is a rabbit hole i didn't even know existed, it's fascinating!

}

type Theme = {
Expand Down Expand Up @@ -97,7 +81,10 @@ export const WaveForm = ({
...props
}: Props) => {
// memoise the waveform data so they aren't recalculated on every render
const barHeights = useMemo(() => generateWaveform(src, bars), [src, bars]);
const barHeights = useMemo(
() => Array.from(waveformGenerator(src, bars, [60, 100])),
Copy link
Member

@sndrs sndrs Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i didn't realise you could do this with generators in Array.from that's really interesting!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Contributor Author

@mxdvl mxdvl Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah you most definitely can: it’s because they are Iterators and iterables, too!

You can also use for(const bar of waveformGenerator()) { }.

You could technically also [...waveformGenerator()] but that would require a different target I think.

[src, bars],
);
const totalWidth = useMemo(
() => bars * (barWidth + gap) - gap,
[bars, barWidth, gap],
Expand Down