-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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.
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
Typescript-ify screenshot stitcher code in reporting #20061
Changes from 4 commits
d3bee8d
2a5840e
7509e56
bb5b340
9e46ecf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
// No types found for this package. May want to investigate an alternative with types. | ||
// @ts-ignore: implicit any for JS file | ||
import $streamToObservable from '@samverschueren/stream-to-observable'; | ||
import { PNG } from 'pngjs'; | ||
import * as Rx from 'rxjs'; | ||
import { ObservableInput } from 'rxjs'; | ||
import { map, mergeMap, reduce, switchMap, tap, toArray } from 'rxjs/operators'; | ||
import { Logger, Screenshot, Size } from './types'; | ||
|
||
// if we're only given one screenshot, and it matches the output dimensions | ||
// we're going to skip the combination and just use it | ||
const canUseFirstScreenshot = ( | ||
screenshots: Screenshot[], | ||
size: { width: number; height: number } | ||
) => { | ||
if (screenshots.length !== 1) { | ||
return false; | ||
} | ||
|
||
const firstScreenshot = screenshots[0]; | ||
return ( | ||
firstScreenshot.dimensions.width === size.width && | ||
firstScreenshot.dimensions.height === size.height | ||
); | ||
}; | ||
|
||
/** | ||
* Combines the screenshot clips into a single screenshot of size `outputDimensions`. | ||
* @param screenshots - Array of screenshots to combine | ||
* @param outputSize - Final output size that the screenshots should match up with | ||
* @param logger - logger for extra debug output | ||
*/ | ||
export function $combine( | ||
screenshots: Screenshot[], | ||
outputSize: Size, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed this to only take in Size, not Dimension, since the x and y were never used. |
||
logger: Logger | ||
): Rx.Observable<string> { | ||
logger.debug( | ||
`Combining screenshot clips into final, scaled output dimension of ${JSON.stringify( | ||
outputSize | ||
)}` | ||
); | ||
|
||
if (screenshots.length === 0) { | ||
return Rx.throwError('Unable to combine 0 screenshots'); | ||
} | ||
|
||
if (canUseFirstScreenshot(screenshots, outputSize)) { | ||
return Rx.of(screenshots[0].data); | ||
} | ||
|
||
// Turn the screenshot data into actual PNGs | ||
const pngs$ = Rx.from(screenshots).pipe( | ||
mergeMap( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: it seems There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please link to that? I don't see any note about deprecation in the docs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I wasn't precise enough, this form of |
||
(screenshot: Screenshot): ObservableInput<PNG> => { | ||
const png = new PNG(); | ||
const buffer = Buffer.from(screenshot.data, 'base64'); | ||
const parseAsObservable = Rx.bindNodeCallback(png.parse.bind(png)); | ||
return parseAsObservable(buffer); | ||
}, | ||
(screenshot: Screenshot, png: PNG) => { | ||
if ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is new too, this is to detect earlier the bug in #19563. I simulated an error to verify it is exposed correctly: (Note after that screenshot I made slight adjustments to the message). |
||
png.width !== screenshot.dimensions.width || | ||
png.height !== screenshot.dimensions.height | ||
) { | ||
const errorMessage = `Screenshot captured with width:${ | ||
png.width | ||
} and height: ${png.height}) is not of expected width: ${ | ||
screenshot.dimensions.width | ||
} and height: ${screenshot.dimensions.height}`; | ||
|
||
logger.error(errorMessage); | ||
throw new Error(errorMessage); | ||
} | ||
return { screenshot, png }; | ||
} | ||
) | ||
); | ||
|
||
const output$ = pngs$.pipe( | ||
reduce((output: PNG, input: { screenshot: Screenshot; png: PNG }) => { | ||
const { png, screenshot } = input; | ||
// Spitting out a lot of output to help debug https://github.com/elastic/kibana/issues/19563. Once that is | ||
// fixed, this should probably get pared down. | ||
logger.debug(`Output dimensions is ${JSON.stringify(outputSize)}`); | ||
logger.debug(`Input png w: ${png.width} and h: ${png.height}`); | ||
logger.debug( | ||
`Creating output png with ${JSON.stringify(screenshot.dimensions)}` | ||
); | ||
const { dimensions } = screenshot; | ||
png.bitblt( | ||
output, | ||
0, | ||
0, | ||
dimensions.width, | ||
dimensions.height, | ||
dimensions.x, | ||
dimensions.y | ||
); | ||
return output; | ||
}, new PNG({ width: outputSize.width, height: outputSize.height })) | ||
); | ||
|
||
return output$.pipe( | ||
tap(png => png.pack()), | ||
switchMap<PNG, Buffer>($streamToObservable), | ||
toArray(), | ||
map((chunks: Buffer[]) => Buffer.concat(chunks).toString('base64')) | ||
); | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,18 +6,26 @@ | |
|
||
import { toArray } from 'rxjs/operators'; | ||
import { $getClips } from './get_clips'; | ||
import { Dimension } from './types'; | ||
|
||
function getClipsTest(description, { dimensions, max }, { clips: expectedClips }) { | ||
function getClipsTest( | ||
description: string, | ||
input: { dimensions: Dimension; max: number }, | ||
expectedClips: { clips: Dimension[] } | ||
) { | ||
test(description, async () => { | ||
const clips = await $getClips(dimensions, max).pipe(toArray()).toPromise(); | ||
expect(clips.length).toBe(expectedClips.length); | ||
const clips = await $getClips(input.dimensions, input.max) | ||
.pipe(toArray()) | ||
.toPromise(); | ||
expect(clips.length).toBe(expectedClips.clips.length); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: can't we just replace this: expect(clips.length).toBe(expectedClips.clips.length);
for (let i = 0; i < clips.length; ++i) {
expect(clips[i]).toEqual(expectedClips.clips[i]);
} with that? expect(clips).toEqual(expectedClips.clips); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those two should be identical. |
||
for (let i = 0; i < clips.length; ++i) { | ||
expect(clips[i]).toEqual(expectedClips[i]); | ||
expect(clips[i]).toEqual(expectedClips.clips[i]); | ||
} | ||
}); | ||
} | ||
|
||
getClipsTest(`creates one rect if 0, 0`, | ||
getClipsTest( | ||
`creates one rect if 0, 0`, | ||
{ | ||
dimensions: { x: 0, y: 0, height: 0, width: 0 }, | ||
max: 100, | ||
|
@@ -27,7 +35,8 @@ getClipsTest(`creates one rect if 0, 0`, | |
} | ||
); | ||
|
||
getClipsTest(`creates one rect if smaller than max`, | ||
getClipsTest( | ||
`creates one rect if smaller than max`, | ||
{ | ||
dimensions: { x: 0, y: 0, height: 99, width: 99 }, | ||
max: 100, | ||
|
@@ -37,7 +46,8 @@ getClipsTest(`creates one rect if smaller than max`, | |
} | ||
); | ||
|
||
getClipsTest(`create one rect if equal to max`, | ||
getClipsTest( | ||
`create one rect if equal to max`, | ||
{ | ||
dimensions: { x: 0, y: 0, height: 100, width: 100 }, | ||
max: 100, | ||
|
@@ -47,33 +57,36 @@ getClipsTest(`create one rect if equal to max`, | |
} | ||
); | ||
|
||
getClipsTest(`creates two rects if width is 1.5 * max`, | ||
getClipsTest( | ||
`creates two rects if width is 1.5 * max`, | ||
{ | ||
dimensions: { x: 0, y: 0, height: 100, width: 150 }, | ||
max: 100, | ||
}, | ||
{ | ||
clips: [ | ||
{ x: 0, y: 0, height: 100, width: 100 }, | ||
{ x: 100, y: 0, height: 100, width: 50 } | ||
{ x: 100, y: 0, height: 100, width: 50 }, | ||
], | ||
} | ||
); | ||
|
||
getClipsTest(`creates two rects if height is 1.5 * max`, | ||
getClipsTest( | ||
`creates two rects if height is 1.5 * max`, | ||
{ | ||
dimensions: { x: 0, y: 0, height: 150, width: 100 }, | ||
max: 100, | ||
}, | ||
{ | ||
clips: [ | ||
{ x: 0, y: 0, height: 100, width: 100 }, | ||
{ x: 0, y: 100, height: 50, width: 100 } | ||
{ x: 0, y: 100, height: 50, width: 100 }, | ||
], | ||
} | ||
); | ||
|
||
getClipsTest(`created four rects if height and width is 1.5 * max`, | ||
getClipsTest( | ||
`created four rects if height and width is 1.5 * max`, | ||
{ | ||
dimensions: { x: 0, y: 0, height: 150, width: 150 }, | ||
max: 100, | ||
|
@@ -88,14 +101,13 @@ getClipsTest(`created four rects if height and width is 1.5 * max`, | |
} | ||
); | ||
|
||
getClipsTest(`creates one rect if height and width is equal to max and theres a y equal to the max`, | ||
getClipsTest( | ||
`creates one rect if height and width is equal to max and theres a y equal to the max`, | ||
{ | ||
dimensions: { x: 0, y: 100, height: 100, width: 100 }, | ||
max: 100, | ||
}, | ||
{ | ||
clips: [ | ||
{ x: 0, y: 100, height: 100, width: 100 }, | ||
], | ||
clips: [{ x: 0, y: 100, height: 100, width: 100 }], | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: uggggh, we just should probably write our own analog of https://github.com/Jason3S/rx-stream/blob/master/src/streamToRx.ts at some point.