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.

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

svg:MeasureTextCache: flow.html: The cached glyph width depends on the order of the tests. #1241

Open
h-sug1no opened this issue Nov 29, 2021 · 7 comments

Comments

@h-sug1no
Copy link
Contributor

  • build e3d0eb8 and load (Reproduce in chrome, safari, and FX):
    {TabSlide › Simple TabSlide › SVG + Bravura} (Assume that all tests that use the MeasureTextCache are affected)
  1. tests/flow.html#TabSlide.Simple_TabSlide.Bravura
  2. tests/flow.html?module=TabSlide#TabSlide.Simple_TabSlide.Bravura

image

  • some width or position attributes are different like follows:

image


with this dirty patch:

diff --git a/src/svgcontext.ts b/src/svgcontext.ts
index f600a6ea..45bf23a0 100644
--- a/src/svgcontext.ts
+++ b/src/svgcontext.ts
@@ -45,6 +45,11 @@ export interface State {
   lineWidth: number;
 }
 
+interface SVGTextMeasure extends TextMeasure {
+  svg: SVGSVGElement;
+  propsStr: string;
+}
+
 class MeasureTextCache {
   protected txt?: SVGTextElement;
 
@@ -70,6 +75,12 @@ class MeasureTextCache {
       entry = this.measureImpl(text, svg, attributes);
       entries[key] = entry;
     }
+
+    const entry1 = this.measureImpl(text, svg, attributes);
+
+    if (entry.width !== entry1.width) {
+      console.log(`entry: ${JSON.stringify(entry)}, entry1: ${JSON.stringify(entry1)},`);
+    }
     return entry;
   }
 
@@ -97,7 +108,15 @@ class MeasureTextCache {
     // CSS specifies dpi to be 96 and there are 72 points to an inch: 96/72 == 4/3.
     const height = Font.convertSizeToPixelValue(fontSizeInPt);
 
-    return { width: bbox.width, height: height };
+    const ret: SVGTextMeasure = {
+      width: bbox.width,
+      height: height,
+      svg: svg,
+      propsStr: JSON.stringify({
+        viewBox: svg.getAttributeNS(null, 'viewBox'),
+      }),
+    };
+    return ret;
   }
 }
 

multiple problems(entry.width and entry1.width should always be the same, right?) are detected, as follows:

entry: {"width":7.4166669845581055,"height":13.333333333333332,"svg":{},"propsStr":"{\"viewBox\":\"0 0 333.3333333333333 160\"}"},
entry1: {"width":7.415865421295166,"height":13.333333333333332,"svg":{},"propsStr":"{\"viewBox\":\"0 0 384.6153846153846 184.6153846153846\"}"},
@h-sug1no h-sug1no changed the title svg:MeasureTextCache cache: The cached glyph width depends on the order of the tests. svg:MeasureTextCache: flow.html: The cached glyph width depends on the order of the tests. Nov 29, 2021
@0xfe
Copy link
Owner

0xfe commented Nov 29, 2021

yikes, thanks for catching this @h-sug1no!

@0xfe 0xfe added the bug label Nov 29, 2021
@ronyeh
Copy link
Collaborator

ronyeh commented Nov 30, 2021

I'll take a look this week. I'm not familiar with the measure text cache but can investigate it.

How did you notice this bug, by the way?

Have you also been running the command line tests in parallel? We should get more of us testing the parallel visual diffs, and then promote it to be the default!

@h-sug1no
Copy link
Contributor Author

h-sug1no commented Nov 30, 2021

How did you notice this bug, by the way?

Ah...though my puppeteer visual-regression-test branch is still on going, but it can generate svg(and png (generated by UA from svg data)) files. I found this problem when I was testing my branch.

you can test my branch as follows:

clone and checkout:
https://github.com/h-sug1no/vexflow/tree/vrtest-puppeteer-backend-1

git checkout vrtest-puppeteer-backend-1
npm install
npm audit fix
npm install --save-dev puppeteer
npx grunt && npm run reference
VF_GENERATE_OPTIONS="--backend=pptr" npm run generate:current
VF_GENERATE_OPTIONS="--backend=pptr --parallel=1" npm run generate:reference
npm run diff:reference

then you can get the following errors:

# In the case of macOS 11.6, results may be different on the other platforms...
You have        7 fail(s):
pptr-Bend_-_Double_Bends_-_SVG_%2B_Petaluma.svg 1.37663
pptr-Vibrato_-_Vibrato_with_Bend_-_SVG_%2B_Bravura.svg 0.339537
pptr-Bend_-_Double_Bends_-_SVG_%2B_Bravura.svg 0.230638
pptr-Bend_-_Double_Bends_With_Release_-_SVG_%2B_Bravura.svg 0.221603
pptr-Bend_-_Double_Bends_With_Release_-_SVG_%2B_Gonville.svg 0.221384
pptr-Bend_-_Double_Bends_With_Release_-_SVG_%2B_Petaluma.svg 0.218442
pptr-TabTie_-_Pulloffs_-_SVG_%2B_Petaluma.svg 0.0415295

I have no time to fix my branch until the end of next month....

But if you need to fix, Please feel free to fork my branch.

@ronyeh
Copy link
Collaborator

ronyeh commented Dec 6, 2021

I investigated a bit. It might be because some of the test cases call ctx.scale(1.5, 1.5).

Currently, when we scale a SVG context by 1.5X, we modify the viewBox attribute. I think the reasoning is that it should operate similarly to CanvasContext. If the SVG is scaled up, its containing box will be the same size, but we "zoom into" the image.

A different approach would change the SVG element's transform:
transform="scale(1.5 1.5)"

But if we wanted to preserve the cropping behavior of the CanvasContext, we would need to wrap our SVG element in a parent div tag with overflow:hidden.

Here's an example where you can play around with the SVG's viewBox, transform, and parent DIV:

https://jsfiddle.net/om72h39k/

As you modify the viewBox or scale transform, you'll see that the reported width of the ✕ symbol changes slightly. I think it's just regular floating point error.

SVGRect {x: 11, y: 5, width: 7.622656345367432, height: 11.5}
SVGRect {x: 11, y: 5, width: 7.625, height: 11.5}
SVGRect {x: 11, y: 5, width: 7.622109413146973, height: 11.5}
SVGRect {x: 11, y: 5, width: 7.622265815734863, height: 11.5}

See the test cases in: tests/flow.html?module=Bend

Even when I round the widths to 1 decimal place, I get discrepancies like this:

OLD
{"text":"Monstrous","width":62.3,"height":13.3,"svg":{},"family":"Arial, sans-serif","size":"10pt","style":"normal","weight":"normal"}

NEW
{"text":"Monstrous","width":62.2,"height":13.3,"svg":{},"family":"Arial, sans-serif","size":"10pt","style":"normal","weight":"normal"}

This is because some test cases set the scale to 1.5x, and then others set the scale to 1.0x. When we ask the browser to measure the text, the floating point error shows up.

This means that if we change the ordering of test cases, or if we run tests in parallel, the exact widths won't match up.

What should we do?

  1. Nothing. The floating point error is small and doesn't noticeably affect visual output.
  2. Round widths to 1 decimal point. This fixes most, but not all of the discrepancies.
  3. Round widths to nearest integer. Maybe this will introduce small errors with layout? Or maybe it's fine?
  4. Drop the text measuring cache? I think we had a good reason to implement it, as @tommadams made some performance improvements several months ago.

Can you verify that the failing test cases are when:

  • two test cases use the same text (e.g., "Monstrous" in bend_tests.ts)
  • the first test case sets scale(x1, y1)
  • the second / failing test sets scale(x2, y2)
  • x1 != x2 and/or y1 != y2

@tommadams
Copy link
Contributor

Sorry I didn't see this issue earlier, I've not had much time for personal projects recently.

I added the glyph measurement cache because it speeded up the unit tests by about 20%. Without the cache, every single call to measureText on a CanvasContext will cause a document reflow.

I'm all for deleting the cache if its buggy. I actually have a branch that I'm working on that factors texture measurement out into a separate class. That way, we can use a hidden (or offscreen) canvas to measure text even when the actual rendering is done using SVG.

I can send a PR deleting the cache later in the week.

@ronyeh
Copy link
Collaborator

ronyeh commented Dec 7, 2021

I don't think we need to delete the cache just yet. The measurement numbers are basically the same. :-) It only affects the visual regression tests if we run them out of order (or in parallel).

On a given score in the real world, no one will notice if something is off by 0.1 pixels. Plus, this only happens if someone has two scores at different context scales rendering the same text.

I like your idea of using a offscreen canvas to measure text. That way, the helper canvas can always be the same scale, even if we scale the output canvas or SVG.

@sschmidTU
Copy link
Contributor

sschmidTU commented Dec 7, 2021

Just a quick idea: Can't we save the pixel value for 1.0 scale or something and restore it later?
Whenever you have an out of order testing bug, usually you can either reset the values to a default before the test or store and restore values that tests modify (like we do with context).
This may not work or be helpful, but I thought I'd mention it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants