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

Avoid preprocessing images that have already been preprocessed. #125

Merged
merged 2 commits into from
Aug 10, 2018
Merged

Avoid preprocessing images that have already been preprocessed. #125

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c02e5b1
Avoid preprocessing images that have already been preprocessed.
jakearchibald Aug 6, 2018
b93dbae
Using cleanMerge an cleanSet, and fixing bugs in our compression.
jakearchibald Aug 7, 2018
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
108 changes: 50 additions & 58 deletions src/components/App/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,16 @@ import {
} from '../../codecs/preprocessors';

import { decodeImage } from '../../codecs/decoders';
import { cleanMerge } from '../../lib/clean-modify';
import { cleanMerge, cleanSet } from '../../lib/clean-modify';

interface SourceImage {
file: File;
bmp: ImageBitmap;
data: ImageData;
preprocessed?: ImageData;
}

interface EncodedImage {
preprocessed?: ImageData;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The preprocessed image is unique to left & right, so I moved it here. This fixes a bug where the settings on the left would impact the image on the right.

bmp?: ImageBitmap;
file?: File;
downloadUrl?: string;
Expand All @@ -66,6 +66,10 @@ interface State {
error?: string;
}

interface UpdateImageOptions {
skipPreprocessing?: boolean;
}

const filesize = partial({});

async function preprocessImage(
Expand All @@ -79,28 +83,22 @@ async function preprocessImage(
return result;
}
async function compressImage(
source: SourceImage,
image: ImageData,
encodeData: EncoderState,
sourceFilename: string,
): Promise<File> {
// Special case for identity
if (encodeData.type === identity.type) return source.file;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This logic has moved into updateImage, as it impacts preprocessing too.


let sourceData = source.data;
if (source.preprocessed) {
sourceData = source.preprocessed;
}
const compressedData = await (() => {
switch (encodeData.type) {
case mozJPEG.type: return mozJPEG.encode(sourceData, encodeData.options);
case webP.type: return webP.encode(sourceData, encodeData.options);
case browserPNG.type: return browserPNG.encode(sourceData, encodeData.options);
case browserJPEG.type: return browserJPEG.encode(sourceData, encodeData.options);
case browserWebP.type: return browserWebP.encode(sourceData, encodeData.options);
case browserGIF.type: return browserGIF.encode(sourceData, encodeData.options);
case browserTIFF.type: return browserTIFF.encode(sourceData, encodeData.options);
case browserJP2.type: return browserJP2.encode(sourceData, encodeData.options);
case browserBMP.type: return browserBMP.encode(sourceData, encodeData.options);
case browserPDF.type: return browserPDF.encode(sourceData, encodeData.options);
case mozJPEG.type: return mozJPEG.encode(image, encodeData.options);
case webP.type: return webP.encode(image, encodeData.options);
case browserPNG.type: return browserPNG.encode(image, encodeData.options);
case browserJPEG.type: return browserJPEG.encode(image, encodeData.options);
case browserWebP.type: return browserWebP.encode(image, encodeData.options);
case browserGIF.type: return browserGIF.encode(image, encodeData.options);
case browserTIFF.type: return browserTIFF.encode(image, encodeData.options);
case browserJP2.type: return browserJP2.encode(image, encodeData.options);
case browserBMP.type: return browserBMP.encode(image, encodeData.options);
case browserPDF.type: return browserPDF.encode(image, encodeData.options);
default: throw Error(`Unexpected encoder ${JSON.stringify(encodeData)}`);
}
})();
Expand All @@ -109,7 +107,7 @@ async function compressImage(

return new File(
[compressedData],
source.file.name.replace(/\..+$/, '.' + encoder.extension),
sourceFilename.replace(/\..+$/, '.' + encoder.extension),
{ type: encoder.mimeType },
);
}
Expand Down Expand Up @@ -150,57 +148,43 @@ export default class App extends Component<Props, State> {
}
}

onChange(
index: 0 | 1,
preprocessorState: PreprocessorState,
type: EncoderType,
options?: EncoderOptions,
): void {
// Some type cheating here.
// encoderMap[type].defaultOptions is always safe.
// options should always be correct for the type, but TypeScript isn't smart enough.
const encoderState: EncoderState = {
type,
options: options || encoderMap[type].defaultOptions,
} as EncoderState;

const images = cleanMerge(this.state.images, index, { encoderState, preprocessorState });
this.setState({ images });
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems better to update the relevant parts of state in the individual change events.


onEncoderTypeChange(index: 0 | 1, newType: EncoderType): void {
this.onChange(index, this.state.images[index].preprocessorState, newType);
this.setState({
images: cleanSet(this.state.images, `${index}.encoderState`, {
type: newType,
options: encoderMap[newType].defaultOptions,
}),
});
}

onPreprocessorOptionsChange(index: 0 | 1, options: PreprocessorState): void {
this.onChange(
index,
options,
this.state.images[index].encoderState.type,
this.state.images[index].encoderState.options,
);
this.setState({
images: cleanSet(this.state.images, `${index}.preprocessorState`, options),
});
}

onEncoderOptionsChange(index: 0 | 1, options: EncoderOptions): void {
this.onChange(
index,
this.state.images[index].preprocessorState,
this.state.images[index].encoderState.type,
options,
);
this.setState({
images: cleanSet(this.state.images, `${index}.encoderState.options`, options),
});
}

componentDidUpdate(prevProps: Props, prevState: State): void {
const { source, images } = this.state;

for (const [i, image] of images.entries()) {
const prevImage = prevState.images[i];
const sourceChanged = source !== prevState.source;
const encoderChanged = image.encoderState !== prevImage.encoderState;
const preprocessorChanged = image.preprocessorState !== prevImage.preprocessorState;

// The image only needs updated if the encoder settings have changed, or the source has
// changed.
if (source !== prevState.source || image.encoderState !== prevImage.encoderState) {
if (sourceChanged || encoderChanged || preprocessorChanged) {
if (prevImage.downloadUrl) URL.revokeObjectURL(prevImage.downloadUrl);
this.updateImage(i).catch((err) => {
this.updateImage(i, {
skipPreprocessing: !sourceChanged && !preprocessorChanged,
}).catch((err) => {
console.error(err);
});
}
Expand Down Expand Up @@ -240,7 +224,8 @@ export default class App extends Component<Props, State> {
}
}

async updateImage(index: number): Promise<void> {
async updateImage(index: number, options: UpdateImageOptions = {}): Promise<void> {
const { skipPreprocessing = false } = options;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Syntactic nit: Not a fan of this visually. Nbd, especially since it’s only one option, but I prefer

options = {...defaultUpdateImageOptions, ...options};

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doesn't that just introduce more scrolling to find out what the defaults are?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(fwiw our lint rules don't allow argument overwriting)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like we had this conversation before. It probably does, but I find this incredibly noise, especially with more options. As I said, nbd.

Copy link
Collaborator Author

@jakearchibald jakearchibald Aug 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Last time we had this discussion we settled on the pattern I used 😄. But yeah, it was a compromise. I initially put the destructuring in the args, which is kinda ugly, and until it pops up as part of autocomplete, I'm not bothered.

const { source } = this.state;
if (!source) return;

Expand All @@ -258,8 +243,15 @@ export default class App extends Component<Props, State> {

let file;
try {
source.preprocessed = await preprocessImage(source, image.preprocessorState);
file = await compressImage(source, image.encoderState);
// Special case for identity
if (image.encoderState.type === identity.type) {
file = source.file;
} else {
if (!skipPreprocessing || !image.preprocessed) {
image.preprocessed = await preprocessImage(source, image.preprocessorState);
}
file = await compressImage(image.preprocessed, image.encoderState, source.file.name);
}
} catch (err) {
this.showError(`Encoding error (type=${image.encoderState.type}): ${err}`);
throw err;
Expand All @@ -273,7 +265,7 @@ export default class App extends Component<Props, State> {

let bmp;
try {
bmp = await createImageBitmap(file);
bmp = await decodeImage(file);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@surma I think this should happen, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

} catch (err) {
this.setState({ error: `Encoding error (type=${image.encoderState.type}): ${err}` });
throw err;
Expand Down