Fix template scale in find element by image #307

KazuCocoa · 2019-03-01T13:02:43Z

Fix remaining issue in https://github.com/appium/appium-base-driver/pull/306/files#r261480026 .
In the PR, I fixed a template image had different width/height with device screen, but it can be fixed scaling them up/down upto match to device width/height.

But the ratio issue has also below case.

example:

A user has a template image which is scaled down from 750 x 1314 pixels image to 187 x 328 pixels in order to decrease their storage when he store the template image.
The user uses the scale downed image as the next test case
- Then, the scaled image does not match in current find by image logic since Appium cannot determin what ratio should be scaled the template up/down
- Users must scale them up the image in the user side every time.

To resolve the case, I introduced defaultImageTemplateScale. If the user sets defaultImageTemplateScale: 4 for example, Appium will scale the template image up 4 times by default. Then, users do not need to scale in user side.

It can use with fixImageTemplateScale for example. It can help to handle some cases such as I described as #306

cc @jlipps

…default

mykola-mokhnach · 2019-03-01T14:00:25Z

lib/basedriver/commands/find.js

@@ -183,6 +183,9 @@ commands.findByCustom = async function (selector, multiple) {
 * image is merely to check staleness. If so we can bypass a lot of logic
 * @property {boolean} [multiple=false] - Whether we are finding one element or
 * multiple
+ * @property {boolean} [ignoreDefaultImageTemplateScale=false] - Whether we
+ * ignore defaultImageTemplateScale. It can be used when you would noe like to


lib/basedriver/commands/find.js

mykola-mokhnach · 2019-03-01T14:04:00Z

lib/basedriver/commands/find.js

 * @param {string} b64Template - base64-encoded image used as a template to be
 * matched in the screenshot
- * @param {ScreenshotScale} scale - scale of screen. Default to `{}`
+ * @param {ScreenshotScale, ImageTemplateSettings} opts - Image template scale related options


I'd rather keep a single ImageTemplateSettings type here.

mykola-mokhnach · 2019-03-01T14:06:09Z

lib/basedriver/commands/find.js

+
+  const defaultScale = 1.0; // To initialise xScale and yScale
+  const defaultOpts = {
+    fixImageTemplateScale: false,


You can set defaults without _.defaults call:

const { defaultImageTemplateScale: DEFAULT_TEMPLATE_IMAGE_SCALE, ... } = opts;

mykola-mokhnach · 2019-03-01T14:06:31Z

lib/basedriver/commands/find.js

+    return b64Template;
+  }
+
+  const defaultScale = 1.0; // To initialise xScale and yScale


constants should be in uppercase

also it's better to move it out of the func

mykola-mokhnach · 2019-03-01T14:09:18Z

lib/basedriver/commands/find.js

+    xScale = xScale * defaultImageTemplateScale;
+    yScale = yScale * defaultImageTemplateScale;
+  } else {
+    // `1.0 *` makes NaN if defaultImageTemplateScale is no number


is not a number

mykola-mokhnach · 2019-03-01T14:10:00Z

lib/basedriver/commands/find.js

-  const {xScale, yScale} = scale;
+  // Calculate xScale and yScale Appium should scale
+  if (opts.fixImageTemplateScale) {
+    xScale = xScale * defaultImageTemplateScale;


use xScale *= defaultImageTemplateScale

mykola-mokhnach · 2019-03-01T14:10:50Z

lib/basedriver/commands/find.js

  if (!xScale || !yScale) {
    return b64Template;
  }

+  // Return if the scale is deatul (1.0) value


mykola-mokhnach · 2019-03-01T14:11:25Z

lib/basedriver/commands/find.js

  if (!xScale || !yScale) {
    return b64Template;
  }

+  // Return if the scale is deatul (1.0) value
+  if (xScale === defaultScale && yScale === defaultScale) {


in general it is not a good idea to compare float values for equality

mykola-mokhnach · 2019-03-01T14:12:02Z

lib/basedriver/device-settings.js

@@ -25,6 +25,14 @@ const GLOBAL_DEFAULT_SETTINGS = {
  //      if a user use `width=750 × height=1334` pixels's base template image.
  fixImageTemplateScale: false,

+  // Users might have scaled template image to reduce their storage size.


I would be nice to have this also described in some doc

Yes. I'm thinking to add this in https://github.com/appium/appium/blob/master/docs/en/advanced-concepts/image-elements.md

KazuCocoa · 2019-03-02T03:31:09Z

lib/basedriver/commands/find.js

+
+  const screenAR = screenWidth / screenHeight;
+  const shotAR = shotWidth / shotHeight;
+  if (screenAR === shotAR) {


https://github.com/appium/appium-base-driver/pull/306/files#diff-a7d1893e663367145d766d1aaab4aad4L347 case.
I discussed with @jlipps and figure out the issue.
I left an example of this ratio issue as a comment below.

float comparison?

mykola-mokhnach · 2019-03-02T06:39:26Z

lib/basedriver/commands/find.js

+    // Because if Appium selects the both ratio, the screenshot will be distorted.
+    // If Appium selects the xScale, height will be bigger than real screenshot size
+    // which is used to image comparison by OpenCV as a base image.
+    const _xScale = (1.0 * shotWidth) / screenWidth;


do we need underscore prefixes?

mykola-mokhnach · 2019-03-02T06:40:33Z

lib/basedriver/commands/find.js

    return b64Template;
  }

-  const {xScale, yScale} = scale;
+  let {


mykola-mokhnach · 2019-03-02T06:42:06Z

lib/basedriver/commands/find.js

+    xScale *= defaultImageTemplateScale;
+    yScale *= defaultImageTemplateScale;
+  } else {
+    // `1 *` makes NaN if defaultImageTemplateScale is not a number


I think parseFloat would be more readable for such purpose

> parseFloat('a') NaN

ah... 👍

mykola-mokhnach · 2019-03-02T06:43:45Z

lib/basedriver/commands/find.js

  if (!xScale || !yScale) {
    return b64Template;
  }

+  // Return if the scale is default (1.0) value
+  if ((xScale - DEFAULT_FIX_IMAGE_TEMPLATE_SCALE) <= 0 && (yScale - DEFAULT_FIX_IMAGE_TEMPLATE_SCALE) === 0) {


please avoid comparing float values for equality.
It is mandatory to have some delta epsilon value or use toFixed transformation and compare these as strings

https://stackoverflow.com/questions/3343623/javascript-comparing-two-float-values

mykola-mokhnach · 2019-03-02T07:22:05Z

lib/basedriver/commands/find.js

@@ -12,6 +12,10 @@ const commands = {}, helpers = {}, extensions = {};
 const IMAGE_STRATEGY = '-image';
 const CUSTOM_STRATEGY = '-custom';

+// Used to compar ratio and screen width
+// Pixel is basically under 1080 for example. 100K is probably enough fo a while.
+const DEFAULT_ROUND_FLOAT = 100000;


I'd rather rename it to FLOAT_PRECISION

mykola-mokhnach · 2019-03-02T07:45:17Z

lib/basedriver/commands/find.js



 const commands = {}, helpers = {}, extensions = {};

 const IMAGE_STRATEGY = '-image';
 const CUSTOM_STRATEGY = '-custom';

+// Used to compar ratio and screen width
+// Pixel is basically under 1080 for example. 100K is probably enough fo a while.
+const FLOAT_PERCISION = 100000;


mykola-mokhnach · 2019-03-02T07:46:18Z

lib/basedriver/commands/find.js

@@ -357,42 +367,119 @@ helpers.getScreenshotForImageFind = async function (screenWidth, screenHeight) {
  // of coordinates returned by the image match algorithm, since we match based
  // on the screenshot coordinates not the device coordinates themselves. There
  // are two potential types of mismatch: aspect ratio mismatch and scale
-  // mismatch.
+  // mismatch. we need to detect and fix both


KazuCocoa · 2019-03-02T07:53:06Z

Will wait for @jlipps to double check the comparison logic

KazuCocoa · 2019-03-11T06:07:12Z

@jlipps Do you have a chance to have a look?
(Or can I merge and publish it as beta?)

jlipps

got some final questions for you, looks mostly good though

jlipps · 2019-03-11T16:46:38Z

lib/basedriver/commands/find.js



 const commands = {}, helpers = {}, extensions = {};

 const IMAGE_STRATEGY = '-image';
 const CUSTOM_STRATEGY = '-custom';

+// Used to compar ratio and screen width


jlipps · 2019-03-11T16:48:03Z

lib/basedriver/commands/find.js

+  const screenAR = screenWidth / screenHeight;
+  const shotAR = shotWidth / shotHeight;
+  if (Math.round(screenAR * FLOAT_PRECISION) === Math.round(shotAR * FLOAT_PRECISION)) {
+    log.info('Screenshot aspect ratio matched screen aspect ratio');


would be nice to do something like 'Screenshot aspect ratio (${shotAR}) matched screen aspect ratio (${screenAR})'

jlipps · 2019-03-11T16:50:49Z

lib/basedriver/commands/find.js

+             `is ${screenWidth}x${screenHeight} whereas screenshot is ` +
+             `${shotWidth}x${shotHeight}.`);
+
+    // Select smaller ratio


// In the case where the x-scale and y-scale are different, we need to decide // which one to respect, otherwise the screenshot and template will end up // being resized in a way that changes its aspect ratio (distorts it). For example, let's say: // this.getScreenshot(shotWidth, shotHeight) is 540x397, // this.getDeviceSize(screenWidth, screenHeight) is 1080x1920. // The ratio would then be {xScale: 0.5, yScale: 0.2}. // In this case, we must should `yScale: 0.2` as scaleFactor, because // If we select the xScale, the height will be bigger than real screenshot size // which is used to image comparison by OpenCV as a base image. // All of this is primarily useful when the screenshot is a horizontal slice taken out of the // screen (for example not including top/bottom nav bars)

jlipps · 2019-03-11T16:53:06Z

lib/basedriver/commands/find.js


  // Resize based on the screen dimensions only if both width and height are mismatched
  // since except for that, it might be a situation which is different window rect and
  // screenshot size like `@driver.window_rect #=>x=0, y=0, width=1080, height=1794` and
  // `"deviceScreenSize"=>"1080x1920"`
-  let scale;
  if (screenWidth !== shotWidth && screenHeight !== shotHeight) {


shouldn't we be comparing the new imgObj width and height here? it looks like we're comparing against the old shotWidth/shotHeight, but those could have changed in the previous block, right?

Right. I added b536b10 to update shotWidth and shotHeight if they should have been updated in the ratio comparison

@jlipps Do you have a chance to take a look ^?

jlipps · 2019-03-11T16:53:50Z

lib/basedriver/device-settings.js

+  // Users might have scaled template image to reduce their storage size.
+  // This setting allows users to scale a template image they send to Appium server
+  // so that Appium server compare actual scale users orginaly have.
+  // e.g. A user have `270 × 32 pixels` image oridinally `1080 × 126 pixels`


If a user has an image of 270 x 32 pixels which was originally 1080 x 126 pixels

jlipps · 2019-03-11T16:53:55Z

lib/basedriver/device-settings.js

@@ -25,6 +25,14 @@ const GLOBAL_DEFAULT_SETTINGS = {
  //      if a user use `width=750 × height=1334` pixels's base template image.
  fixImageTemplateScale: false,

+  // Users might have scaled template image to reduce their storage size.
+  // This setting allows users to scale a template image they send to Appium server
+  // so that Appium server compare actual scale users orginaly have.


so that the Appium server compares the actual scale users originally had

jlipps · 2019-03-11T16:54:34Z

lib/basedriver/device-settings.js

+  // This setting allows users to scale a template image they send to Appium server
+  // so that Appium server compare actual scale users orginaly have.
+  // e.g. A user have `270 × 32 pixels` image oridinally `1080 × 126 pixels`
+  //      The user set {defaultImageTemplateScale: 4.0} to scale the small image


The user can set

KazuCocoa added 6 commits March 1, 2019 16:25

add imageTemplateDefaultScale to scale arbitrary scale every time by …

8e733b7

…default

put all of template scaling into one method

d0e713e

increase test cases to cover all situations

ea57050

define gnoreDefaultImageTemplateScale for click

69d0b84

add a comment

acf991f

tweak comments

281457d

KazuCocoa requested a review from jlipps March 1, 2019 13:02

KazuCocoa changed the title ~~Fix template scale more~~ Fix template scale in find element by image Mar 1, 2019

mykola-mokhnach reviewed Mar 1, 2019

View reviewed changes

lib/basedriver/commands/find.js Show resolved Hide resolved

mykola-mokhnach reviewed Mar 1, 2019

View reviewed changes

KazuCocoa added 3 commits March 2, 2019 01:17

fix typo, default value, docstring

4ae5e24

fix review

7c09a98

fix ratio potential issue

17cb7da

KazuCocoa force-pushed the fix-template-scale-more branch from 6b1c2c1 to 17cb7da Compare March 2, 2019 03:28

KazuCocoa commented Mar 2, 2019

View reviewed changes

KazuCocoa added 2 commits March 2, 2019 12:32

use const instead of let

57ed6ab

tweak a comment about scale factor

952f83d

mykola-mokhnach reviewed Mar 2, 2019

View reviewed changes

lib/basedriver/commands/find.js

return b64Template;

}

const {xScale, yScale} = scale;

let {

Copy link

Contributor

mykola-mokhnach Mar 2, 2019

Choose a reason for hiding this comment

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

👍

mykola-mokhnach reviewed Mar 2, 2019

View reviewed changes

remove _, tweak NaN comparison

de88d3e

use math.round to compare two float

ce71874

mykola-mokhnach reviewed Mar 2, 2019

View reviewed changes

rename float round constant

6dde6f0

mykola-mokhnach reviewed Mar 2, 2019

View reviewed changes

mykola-mokhnach approved these changes Mar 2, 2019

View reviewed changes

fix typo

1df9443

KazuCocoa mentioned this pull request Mar 4, 2019

add fixImageTemplateScale appium/appium#12245

Merged

9 tasks

jlipps suggested changes Mar 11, 2019

View reviewed changes

KazuCocoa added 2 commits March 12, 2019 10:37

update imgObj

b536b10

fix comments

07c246a

KazuCocoa requested a review from jlipps March 19, 2019 16:55

jlipps approved these changes Mar 19, 2019

View reviewed changes

KazuCocoa merged commit b38ef75 into appium:master Mar 20, 2019

KazuCocoa deleted the fix-template-scale-more branch March 20, 2019 00:00

KazuCocoa mentioned this pull request Mar 20, 2019

CHANGELOG 1.12 appium/appium#12348

Merged

KazuCocoa mentioned this pull request Apr 23, 2019

docs: Prepare v1.13.0 CHANGELOG appium/appium#12526

Merged

Fix template scale in find element by image #307

Fix template scale in find element by image #307

Conversation

KazuCocoa commented Mar 1, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Mar 2, 2019 • 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

Choose a reason for hiding this comment

KazuCocoa commented Mar 2, 2019

KazuCocoa commented Mar 11, 2019

jlipps left a 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

KazuCocoa Mar 12, 2019 • 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

Choose a reason for hiding this comment

mykola-mokhnach Mar 2, 2019 •

edited

Loading

KazuCocoa Mar 12, 2019 •

edited

Loading