Rotate #322

jakearchibald · 2018-11-19T11:33:19Z

Ready!

jakearchibald · 2018-11-19T11:34:21Z

src/codecs/resize/options.tsx

@@ -135,7 +135,7 @@ export default class ResizerOptions extends Component<Props, State> {
                onChange={this.onChange}
              >
                <option value="stretch">Stretch</option>
-                <option value="cover">Cover</option>
+                <option value="contain">Contain</option>


I have no idea why this was called "cover" before. It's clearly "contain", and other bits of code referred to it as "contain".

jakearchibald · 2018-11-19T11:35:24Z

src/codecs/rotate-flip/options.tsx

+            onInput={this.onChange}
+          >
+            Rotate:
+          </Range>


I'm not sure if this is the best input to use. Alternatively we could have a number input with buttons either side that increment/decrement by 90.

This is okay for now imo

It would be nice to also mention the direction of the rotation (e.g "clockwise" or "CW").

jakearchibald · 2018-11-19T11:37:29Z

src/codecs/rotate-flip/processor.ts

+      yield ((d1 * d1Multiplier) + (d2 * d2Multiplier)) * bpp;
+    }
+  }
+}


I was happy when I wrote this, but now it looks like an unmaintainable mess. I'm going to give offscreen canvas a try, and for other browsers… main thread canvas? It'd avoid forking the logic.

jakearchibald · 2018-11-19T12:30:37Z

Hmm, stand by, there are bugs that I didn't notice in dev.

jakearchibald · 2018-11-19T12:31:58Z

Oh great. The bugs don't appear in dev.

jakearchibald · 2018-11-19T12:47:48Z

@developit one for you GoogleChromeLabs/critters#17

jakearchibald · 2018-11-19T13:01:57Z

I've hacked around the issue for now, but we shouldn't merge with the hack.

jakearchibald · 2018-11-19T13:21:07Z

Yeah, OffscreenCanvas seems worth it. Rotating the red panda image goes from 900ms to 300ms.

…no wait, lol, the offscreen canvas causes Chrome to switch OSX to the discrete GPU. That means serious jank when the worker loads the offscreen canvas code, and another jank when the worker is terminated and it switches back to the integrated GPU. Ok, I'll create a test case and file.

jakearchibald · 2018-11-19T14:06:43Z

https://bugs.chromium.org/p/chromium/issues/detail?id=906619

jakearchibald · 2018-11-20T08:20:38Z

Given that OffscreenCanvas is off the table, I should write/find a C++ solution and WASM it.

jakearchibald · 2018-11-20T08:36:01Z

Maybe it's time to bring in https://github.com/ImageMagick/ImageMagick. Once we have that it'll be easy to add loads of different transforms.

surma · 2018-11-20T11:05:18Z

We could implement 90° image rotation in JavaScript using ImageData and create a new RGBA buffer. Probably much smaller and sufficiently fast.

ImageMagick seems quite heavy for this but I’m not sure what else you had in mind. More scaling algorithms?

jakearchibald · 2018-11-20T12:05:16Z

We could implement 90° image rotation in JavaScript using ImageData and create a new RGBA buffer. Probably much smaller and sufficiently fast.

That's what this PR is currently doing.

ImageMagick seems quite heavy for this but I’m not sure what else you had in mind. More scaling algorithms?

Yeah, the resize algorithms I'd like to add are all in ImageMagick. But maybe I'm overcomplicating things, and we could just land my JS solution for now.

jakearchibald · 2018-11-20T12:13:34Z

Hah, I've just brought my implementation down from 900ms to 300ms, same as the offscreen canvas. It could probably be faster with multithreading too.

I was using a generator in the previous implementation. Removed it.

surma

My bad! I though you did OffscreenCanvas only, lol.

This is just a suggestion, but we could make use of DOMMatrix to tighten up the code, but it’s a bit more... math-y. I just whipped this up in a scratchpad, so this might not be 100% correct, but you see where I’m going.

  const width = 200, height = 300;
  const angle = 90;
  const m = new DOMMatrix()
  // Rotate points 90 degrees around origin
  m.rotateSelf(0, 0, angle);
  // Calculate where the extreme point (width, height) is mapped to
  // so we can figure out what translation to apply to make sure
  // all result coordinates remain positive
  const extreme = m.transformPoint(new DOMPoint(width, height));
  // We have to apply that translation *before* the rotation. So we
  // undo the rotation, apply the translation, then re-apply the rotation.
  m.rotateSelf(0, 0, -angle);
  m.translateSelf(extreme.x < 0 ? -extreme.x : 0, extreme.y < 0 ? -extreme.y : 0);  
  m.rotateSelf(0, 0, angle);
  
  for(let py = 0; py < height; py++) {
    for(let px = 0; px < width; px++) {
      const p = new DOMPoint(px, py);
	  const {x, y} = m.transformPoint(dp);
  	  for(let i = 0; i < 4; i++) {
        // etc bla bla bla
      }
  });

surma · 2018-11-20T12:34:50Z

src/codecs/rotate-flip/options.tsx

+            onInput={this.onChange}
+          >
+            Rotate:
+          </Range>


This is okay for now imo

surma · 2018-11-20T13:07:36Z

Also: Should the rotate/flip transform be special and get applied to original image as well?

jakearchibald · 2018-11-20T14:10:26Z

@surma DOMMatrix isn't in Safari, and the CSS matrix fallback isn't available in a worker.

I started looking at bringing in gl-matrix for this. WDYT?

surma · 2018-11-20T14:13:39Z

Of course it isn’t. Eh, gl-matrix is probably more code than what you’ve written at this point, so let’s stay on your code and we can look at ImageMagick eventually.

surma · 2018-11-20T14:25:23Z

Alternatively: Conditionally load a polyfill?

jakearchibald · 2018-11-20T15:36:51Z

I'll give the polyfill a go. It would make the code a whole lot more readable.

jakearchibald · 2018-11-22T07:03:16Z

This PR breaks the pinch zoom's boundary checking, since the element is centered using transforms. I should go back to the previous method.

Also, the Chrome graphics glitch is back, probably because I introduced a wrapping element but didn't move the fix into that element.

Haprog · 2018-11-26T08:06:53Z

Also: Should the rotate/flip transform be special and get applied to original image as well?

I think yes. Otherwise the split view is just plain weird and not that useful anymore.

One idea: Would it be simpler to apply the real image data rotation only when saving/exporting, and rotate the whole split view area (including both images) via a CSS transform only (just to get the preview showing in correct orientation, and then the original doesn't need to be rotated in a different way)?

Or would it make sense to have an option to preserve the Exif Orientation data in the output image (even if stripping all other Exif data) instead of doing a rotation on the image data?

jakearchibald · 2018-11-26T17:26:06Z

Otherwise the split view is just plain weird and not that useful anymore.

You can always apply the same transform to both sides.

Would it be simpler to apply the real image data rotation only when saving/exporting

Then the visual and file size preview would be wrong, which doesn't seem great.

jakearchibald · 2018-11-26T17:58:23Z

I gave DOMMatrix a spin:

export function rotateFlip(data: ImageData, opts: RotateFlipOptions): ImageData {
  const flipDimensions = opts.rotate % 180 !== 0;
  const width = flipDimensions ? data.height : data.width;
  const height = flipDimensions ? data.width : data.height;
  const out = new ImageData(width, height);
  const { width: inputWidth, height: inputHeight } = data;

  const matrix = new DOMMatrix();
  matrix.translateSelf(width / 2, height / 2);
  matrix.scaleSelf(
    opts.flipHorizontal ? -1 : 1,
    opts.flipVertical ? -1 : 1,
    1,
  );
  matrix.rotateSelf(opts.rotate);
  matrix.translateSelf(-inputWidth / 2, -inputHeight / 2);

  const point = new DOMPoint();

  for (let y = 0; y < inputHeight; y += 1) {
    for (let x = 0; x < inputWidth; x += 1) {
      point.x = x;
      point.y = y;
      const destPoint = matrix.transformPoint(point);
      const fromStart = (y * inputWidth + x) * bpp;
      const destStart = (Math.round(destPoint.y) * width + Math.round(destPoint.x)) * bpp;
      for (let i = 0; i < bpp; i += 1) {
        out.data[destStart + i] = data.data[fromStart + i];
      }
    }
  }
  return out;
}

Much simpler code, but rotating the red panda image now takes 37.5 seconds, compared to 280ms. I'm guessing the creation of point objects and the necessary rounding makes the difference. Using gl-matrix will remove the object creation, but we'd still have the rounding to deal with.

Going to stick with the currently pushed solution.

jakearchibald · 2018-11-29T12:28:59Z

I've added a rotate button next to the zoom controls. I've created the idea of an "input processor", which is a set of things that happens to the input before it's split across the two sides.

I'm going to add flipping, and do some of my own reviews & testing, but I'd like feedback on the UX.

https://deploy-preview-322--squoosh.netlify.com/

jakearchibald · 2018-11-29T12:30:16Z

src/components/App/index.tsx

-  data: ImageData;
-  vectorImage?: HTMLImageElement;
-}
-


I have no idea what this was doing here. Probably left over from when we split the compressor out into its own module.

jakearchibald · 2018-11-29T12:31:14Z

src/components/Options/index.tsx

+    let newState = this.props.preprocessorState;
+
+    newState = cleanSet(newState, `${preprocessor}.enabled`, el.checked);
+    this.props.onPreprocessorOptionsChange(newState);


I'll revert this change.

jakearchibald · 2018-11-29T12:34:01Z

src/components/Output/index.tsx

+                  objectFit: leftImgContain ? 'contain' : '',
+                }}
+              />
+            </div>


I'm going to try and remove the need for this boxing.

jakearchibald · 2018-11-29T12:38:08Z

src/components/Output/style.scss

+  align-items: center;
+  // This is a hack to work around https://github.com/GoogleChromeLabs/critters/issues/17.
+  // DO NOT LET THIS MERGE. I'm just putting it here so the UI can be reviewed in the PR.
+  contain: layout !important;


Might not need this hack anymore. Will check.

jakearchibald · 2018-11-29T12:38:49Z

src/components/compress/result-cache.ts

@@ -15,7 +14,7 @@ interface CacheResult {
 interface CacheEntry extends CacheResult {
  preprocessorState: PreprocessorState;
  encoderState: EncoderState;
-  source: SourceImage;
+  sourceData: ImageData;


The source image now has data that doesn't matter to the result cache (eg, rotation settings).

surma · 2018-11-29T12:39:21Z

UI choice looks really good for me!

jakearchibald · 2018-11-29T12:39:28Z

src/lib/icons.tsx

+  return (
+    <Icon {...otherProps}>
+      <path
+        transform={`rotate(${ direction === 'horizontal' ? 90 : 0 })`}


This transform is almost certainly wrong, but I'll catch it when I add the buttons properly.

jakearchibald · 2018-11-29T12:51:52Z

@surma @kosamari @developit how important do we think 'flip' is? It's easy to add, but I'm worried about usefulness vs the UI clutter.

surma · 2018-11-29T12:53:01Z

Fairly low, imo. IIUC, the main demand is coming from importing pictures from your phone, where native pixel order is almost always landscape, even if the picture was taken portrait.

jakearchibald · 2018-11-29T13:15:58Z

I've removed flip. We can always bring it back later.

jakearchibald · 2018-11-29T16:18:20Z

I think this is ready. @surma mind taking another look?

surma · 2018-11-29T16:22:39Z

src/codecs/processor-worker/index.ts

  const { process } = await import(
    /* webpackChunkName: "process-imagequant" */
    '../imagequant/processor',
  );
  return process(data, opts);
 }

+async function rotateFlip(


Nit: Remove flip from the name

Hah well spotted. I thought I caught all of these. Should have done a search.

surma · 2018-11-29T16:23:40Z

src/codecs/rotate/processor-meta.ts

@@ -0,0 +1,3 @@
+export type OptionType = 0 | 90 | 180 | 270;


I think we should do an interface here like with the other encoder options. The naming is weird right now and interfaces are easier to extend.

As in, an interface with a single property? { rotate }?

So calls would be:

rotate(data, { rotate: 90 });

surma · 2018-11-29T16:25:16Z

src/codecs/rotate/processor.ts

+  const width = flipDimensions ? data.height : data.width;
+  const height = flipDimensions ? data.width : data.height;
+  const out = new ImageData(width, height);
+  const { width: inputWidth, height: inputHeight } = data;


Super small nit: Move this up and use inputWidth and inputHeight when calculating width and height.

* Basic rotate & flip * Flipping resize when orientation changes * Hack around critters issue. * Removing generator. Huge perf boost. * Stable positioning * Creating input processors * Allowing rotation to be changed * Reverting old change * Adding tooltips * No more flip * Removing need for wrapper element boxing * Adding comment * Addressing nits * Bleh

jakearchibald requested review from developit, surma and kosamari November 19, 2018 11:33

jakearchibald commented Nov 19, 2018

View reviewed changes

jakearchibald force-pushed the rotate branch from 92a6ac1 to 7d96c80 Compare November 19, 2018 12:26

jakearchibald mentioned this pull request Nov 19, 2018

Maintain order of CSS GoogleChromeLabs/critters#17

Closed

jakearchibald mentioned this pull request Nov 19, 2018

Inspect EXIF data for rotation - Image orientation/rotation is wrong for some images #299

Open

surma approved these changes Nov 20, 2018

View reviewed changes

src/codecs/rotate-flip/options.tsx Outdated

onInput={this.onChange}

>

Rotate:

</Range>

Copy link

Collaborator

surma Nov 20, 2018

Choose a reason for hiding this comment

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

This is okay for now imo

jakearchibald added 5 commits November 29, 2018 08:46

Hack around critters issue.

89ec7e4

Removing generator. Huge perf boost.

276e1c8

Stable positioning

a58cbbf

Creating input processors

cdb4365

Allowing rotation to be changed

cca7368

jakearchibald force-pushed the rotate branch from 3b80b5c to cca7368 Compare November 29, 2018 12:25

jakearchibald commented Nov 29, 2018

View reviewed changes

jakearchibald added 3 commits November 29, 2018 12:54

Reverting old change

686c74f

Adding tooltips

be673e8

No more flip

ae66043

jakearchibald added 2 commits November 29, 2018 16:03

Removing need for wrapper element boxing

3f4e679

Adding comment

d51642a

surma mentioned this pull request Nov 29, 2018

Add ability to sett matte color for transparent/opaque image #347

Closed

surma approved these changes Nov 29, 2018

View reviewed changes

jakearchibald added 2 commits November 30, 2018 10:48

Addressing nits

0b165b8

Bleh

39bc07a

jakearchibald merged commit 1b693fb into master Nov 30, 2018

jakearchibald deleted the rotate branch November 30, 2018 11:00

Rotate #322

Rotate #322

Conversation

jakearchibald commented Nov 19, 2018 • edited Loading

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

jakearchibald commented Nov 19, 2018

jakearchibald commented Nov 19, 2018

jakearchibald commented Nov 19, 2018

jakearchibald commented Nov 19, 2018

jakearchibald commented Nov 19, 2018

jakearchibald commented Nov 19, 2018

jakearchibald commented Nov 20, 2018

jakearchibald commented Nov 20, 2018

surma commented Nov 20, 2018

jakearchibald commented Nov 20, 2018 • edited Loading

jakearchibald commented Nov 20, 2018

surma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surma commented Nov 20, 2018

jakearchibald commented Nov 20, 2018

surma commented Nov 20, 2018

surma commented Nov 20, 2018

jakearchibald commented Nov 20, 2018

jakearchibald commented Nov 22, 2018

Haprog commented Nov 26, 2018

jakearchibald commented Nov 26, 2018

jakearchibald commented Nov 26, 2018

jakearchibald commented Nov 29, 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

surma commented Nov 29, 2018

Choose a reason for hiding this comment

jakearchibald commented Nov 29, 2018

surma commented Nov 29, 2018

jakearchibald commented Nov 29, 2018

jakearchibald commented Nov 29, 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

jakearchibald commented Nov 19, 2018 •

edited

Loading

jakearchibald commented Nov 20, 2018 •

edited

Loading