From f4d22e7ec59f0e631c72ffcf72ee313e3f34f2df Mon Sep 17 00:00:00 2001 From: Olivier Terral Date: Wed, 4 Mar 2020 15:35:30 +0100 Subject: [PATCH 1/9] Ignore missing glyph when font range is already loaded --- src/render/glyph_manager.js | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/render/glyph_manager.js b/src/render/glyph_manager.js index 556d3aa0597..578be3dec04 100644 --- a/src/render/glyph_manager.js +++ b/src/render/glyph_manager.js @@ -52,26 +52,26 @@ class GlyphManager { if (!entry) { entry = this.entries[stack] = { glyphs: {}, - requests: {} + requests: {}, + ranges: {}, }; } - let glyph = entry.glyphs[id]; - if (glyph !== undefined) { - callback(null, {stack, id, glyph}); + const range = Math.floor(id / 256); + if (range * 256 > 65535) { + callback(new Error('glyphs > 65535 not supported')); return; } - glyph = this._tinySDF(entry, stack, id); - if (glyph) { - entry.glyphs[id] = glyph; - callback(null, {stack, id, glyph}); + if (entry.ranges[range]) { + callback(null, {stack, id, glyph: entry.glyphs[id]}); return; } - const range = Math.floor(id / 256); - if (range * 256 > 65535) { - callback(new Error('glyphs > 65535 not supported')); + const glyph = this._tinySDF(entry, stack, id); + if (glyph) { + entry.glyphs[id] = glyph; + callback(null, {stack, id, glyph}); return; } @@ -86,6 +86,7 @@ class GlyphManager { entry.glyphs[+id] = response[+id]; } } + entry.ranges[range] = true; } for (const cb of requests) { cb(err, response); From b078ecac3d6ae363263b2891f539fe2ac08327f7 Mon Sep 17 00:00:00 2001 From: Olivier Terral Date: Wed, 4 Mar 2020 16:32:56 +0100 Subject: [PATCH 2/9] Add test --- test/unit/render/glyph_manager.test.js | 32 +++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/test/unit/render/glyph_manager.test.js b/test/unit/render/glyph_manager.test.js index b03b9036b2f..ee1f846dc21 100644 --- a/test/unit/render/glyph_manager.test.js +++ b/test/unit/render/glyph_manager.test.js @@ -1,4 +1,4 @@ -import {test} from '../../util/test'; +import {test, only} from '../../util/test'; import parseGlyphPBF from '../../../src/style/parse_glyph_pbf'; import GlyphManager from '../../../src/render/glyph_manager'; import fs from 'fs'; @@ -28,6 +28,36 @@ test('GlyphManager requests 0-255 PBF', (t) => { }); }); +test('GlyphManager doesn\'t request twice 0-255 PBF if a glyph is missing', (t) => { + const identityTransform = (url) => ({url}); + const stub = t.stub(GlyphManager, 'loadGlyphRange').callsFake((stack, range, urlTemplate, transform, callback) => { + t.equal(stack, 'Arial Unicode MS'); + t.equal(range, 0); + t.equal(urlTemplate, 'https://localhost/fonts/v1/{fontstack}/{range}.pbf'); + t.equal(transform, identityTransform); + setImmediate(() => callback(null, glyphs)); + }); + + const manager = new GlyphManager(identityTransform); + manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf'); + + manager.getGlyphs({'Arial Unicode MS': [0.5]}, (err, glyphs) => { + t.ifError(err); + t.equal(manager.entries['Arial Unicode MS'].ranges[0], true); + t.equal(stub.calledOnce, true); + + // We remove all requests as in getGlyphs code. + delete manager.entries['Arial Unicode MS'].requests; + + manager.getGlyphs({'Arial Unicode MS': [0.5]}, (err, glyphs) => { + t.ifError(err); + t.equal(manager.entries['Arial Unicode MS'].ranges[0], true); + t.equal(stub.calledOnce, true); + t.end(); + }); + }); +}); + test('GlyphManager requests remote CJK PBF', (t) => { t.stub(GlyphManager, 'loadGlyphRange').callsFake((stack, range, urlTemplate, transform, callback) => { setImmediate(() => callback(null, glyphs)); From e21bbecced165ed92407b87047a425ef996eecab Mon Sep 17 00:00:00 2001 From: Olivier Terral Date: Wed, 4 Mar 2020 16:44:44 +0100 Subject: [PATCH 3/9] lint --- src/render/glyph_manager.js | 2 +- test/unit/render/glyph_manager.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/render/glyph_manager.js b/src/render/glyph_manager.js index 578be3dec04..743d15d94e6 100644 --- a/src/render/glyph_manager.js +++ b/src/render/glyph_manager.js @@ -53,7 +53,7 @@ class GlyphManager { entry = this.entries[stack] = { glyphs: {}, requests: {}, - ranges: {}, + ranges: {} }; } diff --git a/test/unit/render/glyph_manager.test.js b/test/unit/render/glyph_manager.test.js index ee1f846dc21..7c3c76288f9 100644 --- a/test/unit/render/glyph_manager.test.js +++ b/test/unit/render/glyph_manager.test.js @@ -1,4 +1,4 @@ -import {test, only} from '../../util/test'; +import {test} from '../../util/test'; import parseGlyphPBF from '../../../src/style/parse_glyph_pbf'; import GlyphManager from '../../../src/render/glyph_manager'; import fs from 'fs'; From 0fc9d6672333d9223ac18737cb2ce6801b7517b5 Mon Sep 17 00:00:00 2001 From: Olivier Terral Date: Wed, 4 Mar 2020 17:16:09 +0100 Subject: [PATCH 4/9] Fix tests --- src/render/glyph_manager.js | 23 +++++++++++++++-------- test/unit/render/glyph_manager.test.js | 2 +- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/render/glyph_manager.js b/src/render/glyph_manager.js index 743d15d94e6..c83725a6b85 100644 --- a/src/render/glyph_manager.js +++ b/src/render/glyph_manager.js @@ -57,24 +57,31 @@ class GlyphManager { }; } - const range = Math.floor(id / 256); - if (range * 256 > 65535) { - callback(new Error('glyphs > 65535 not supported')); - return; - } - if (entry.ranges[range]) { - callback(null, {stack, id, glyph: entry.glyphs[id]}); + let glyph = entry.glyphs[id]; + if (glyph !== undefined) { + callback(null, {stack, id, glyph}); return; } - const glyph = this._tinySDF(entry, stack, id); + glyph = this._tinySDF(entry, stack, id); if (glyph) { entry.glyphs[id] = glyph; callback(null, {stack, id, glyph}); return; } + const range = Math.floor(id / 256); + if (range * 256 > 65535) { + callback(new Error('glyphs > 65535 not supported')); + return; + } + + if (entry.ranges[range]) { + callback(null, {stack, id, glyph: null}); + return; + } + let requests = entry.requests[range]; if (!requests) { requests = entry.requests[range] = []; diff --git a/test/unit/render/glyph_manager.test.js b/test/unit/render/glyph_manager.test.js index 7c3c76288f9..453acd2114b 100644 --- a/test/unit/render/glyph_manager.test.js +++ b/test/unit/render/glyph_manager.test.js @@ -47,7 +47,7 @@ test('GlyphManager doesn\'t request twice 0-255 PBF if a glyph is missing', (t) t.equal(stub.calledOnce, true); // We remove all requests as in getGlyphs code. - delete manager.entries['Arial Unicode MS'].requests; + delete manager.entries['Arial Unicode MS'].requests[0]; manager.getGlyphs({'Arial Unicode MS': [0.5]}, (err, glyphs) => { t.ifError(err); From 640824ae0cfd1db68c3d5150421b2b1bd34ad961 Mon Sep 17 00:00:00 2001 From: Olivier Terral Date: Wed, 4 Mar 2020 17:18:24 +0100 Subject: [PATCH 5/9] Remove line --- src/render/glyph_manager.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/render/glyph_manager.js b/src/render/glyph_manager.js index c83725a6b85..dd5699ab117 100644 --- a/src/render/glyph_manager.js +++ b/src/render/glyph_manager.js @@ -57,7 +57,6 @@ class GlyphManager { }; } - let glyph = entry.glyphs[id]; if (glyph !== undefined) { callback(null, {stack, id, glyph}); From 41f017c459bcbda13507e31d098b3cdea8d526b3 Mon Sep 17 00:00:00 2001 From: Olivier Terral Date: Wed, 4 Mar 2020 17:18:53 +0100 Subject: [PATCH 6/9] Remove line --- src/render/glyph_manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/render/glyph_manager.js b/src/render/glyph_manager.js index dd5699ab117..d872890b688 100644 --- a/src/render/glyph_manager.js +++ b/src/render/glyph_manager.js @@ -77,7 +77,7 @@ class GlyphManager { } if (entry.ranges[range]) { - callback(null, {stack, id, glyph: null}); + callback(null, {stack, id, glyph}); return; } From f08da011903a878d40448892d37550bf213d1661 Mon Sep 17 00:00:00 2001 From: Olivier Terral Date: Thu, 5 Mar 2020 09:05:33 +0100 Subject: [PATCH 7/9] Lint --- test/unit/render/glyph_manager.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/render/glyph_manager.test.js b/test/unit/render/glyph_manager.test.js index 453acd2114b..70ba92023cf 100644 --- a/test/unit/render/glyph_manager.test.js +++ b/test/unit/render/glyph_manager.test.js @@ -41,7 +41,7 @@ test('GlyphManager doesn\'t request twice 0-255 PBF if a glyph is missing', (t) const manager = new GlyphManager(identityTransform); manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf'); - manager.getGlyphs({'Arial Unicode MS': [0.5]}, (err, glyphs) => { + manager.getGlyphs({'Arial Unicode MS': [0.5]}, (err) => { t.ifError(err); t.equal(manager.entries['Arial Unicode MS'].ranges[0], true); t.equal(stub.calledOnce, true); @@ -49,7 +49,7 @@ test('GlyphManager doesn\'t request twice 0-255 PBF if a glyph is missing', (t) // We remove all requests as in getGlyphs code. delete manager.entries['Arial Unicode MS'].requests[0]; - manager.getGlyphs({'Arial Unicode MS': [0.5]}, (err, glyphs) => { + manager.getGlyphs({'Arial Unicode MS': [0.5]}, (err) => { t.ifError(err); t.equal(manager.entries['Arial Unicode MS'].ranges[0], true); t.equal(stub.calledOnce, true); From 9568daccba38075972de4aff6668da4e2272f668 Mon Sep 17 00:00:00 2001 From: Olivier Terral Date: Thu, 5 Mar 2020 09:09:12 +0100 Subject: [PATCH 8/9] Add ranges flow definition --- src/render/glyph_manager.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/render/glyph_manager.js b/src/render/glyph_manager.js index d872890b688..66b8dcd74f0 100644 --- a/src/render/glyph_manager.js +++ b/src/render/glyph_manager.js @@ -15,6 +15,7 @@ type Entry = { // null means we've requested the range, but the glyph wasn't included in the result. glyphs: {[id: number]: StyleGlyph | null}, requests: {[range: number]: Array>}, + ranges: {[range: number]: boolean | null}, tinySDF?: TinySDF }; From 6621b7154ea08a6e352bf598bc5871830fce59a3 Mon Sep 17 00:00:00 2001 From: Olivier Terral Date: Thu, 19 Mar 2020 09:15:02 +0100 Subject: [PATCH 9/9] Use helpers function --- test/unit/render/glyph_manager.test.js | 48 +++++++++++--------------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/test/unit/render/glyph_manager.test.js b/test/unit/render/glyph_manager.test.js index 70ba92023cf..73f55531a14 100644 --- a/test/unit/render/glyph_manager.test.js +++ b/test/unit/render/glyph_manager.test.js @@ -8,18 +8,27 @@ for (const glyph of parseGlyphPBF(fs.readFileSync('./test/fixtures/0-255.pbf'))) glyphs[glyph.id] = glyph; } -test('GlyphManager requests 0-255 PBF', (t) => { - const identityTransform = (url) => ({url}); - t.stub(GlyphManager, 'loadGlyphRange').callsFake((stack, range, urlTemplate, transform, callback) => { +const identityTransform = (url) => ({url}); + +const createLoadGlyphRangeStub = (t) => { + return t.stub(GlyphManager, 'loadGlyphRange').callsFake((stack, range, urlTemplate, transform, callback) => { t.equal(stack, 'Arial Unicode MS'); t.equal(range, 0); t.equal(urlTemplate, 'https://localhost/fonts/v1/{fontstack}/{range}.pbf'); t.equal(transform, identityTransform); setImmediate(() => callback(null, glyphs)); }); +}; - const manager = new GlyphManager(identityTransform); +const createGlyphManager = (font) => { + const manager = new GlyphManager(identityTransform, font); manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf'); + return manager; +}; + +test('GlyphManager requests 0-255 PBF', (t) => { + createLoadGlyphRangeStub(t); + const manager = createGlyphManager(); manager.getGlyphs({'Arial Unicode MS': [55]}, (err, glyphs) => { t.ifError(err); @@ -29,17 +38,8 @@ test('GlyphManager requests 0-255 PBF', (t) => { }); test('GlyphManager doesn\'t request twice 0-255 PBF if a glyph is missing', (t) => { - const identityTransform = (url) => ({url}); - const stub = t.stub(GlyphManager, 'loadGlyphRange').callsFake((stack, range, urlTemplate, transform, callback) => { - t.equal(stack, 'Arial Unicode MS'); - t.equal(range, 0); - t.equal(urlTemplate, 'https://localhost/fonts/v1/{fontstack}/{range}.pbf'); - t.equal(transform, identityTransform); - setImmediate(() => callback(null, glyphs)); - }); - - const manager = new GlyphManager(identityTransform); - manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf'); + const stub = createLoadGlyphRangeStub(t); + const manager = createGlyphManager(); manager.getGlyphs({'Arial Unicode MS': [0.5]}, (err) => { t.ifError(err); @@ -63,8 +63,7 @@ test('GlyphManager requests remote CJK PBF', (t) => { setImmediate(() => callback(null, glyphs)); }); - const manager = new GlyphManager((url) => ({url})); - manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf'); + const manager = createGlyphManager(); manager.getGlyphs({'Arial Unicode MS': [0x5e73]}, (err, glyphs) => { t.ifError(err); @@ -89,8 +88,7 @@ test('GlyphManager does not cache CJK chars that should be rendered locally', (t return new Uint8ClampedArray(900); } }); - const manager = new GlyphManager((url) => ({url}), 'sans-serif'); - manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf'); + const manager = createGlyphManager('sans-serif'); //Request char that overlaps Katakana range manager.getGlyphs({'Arial Unicode MS': [0x3005]}, (err, glyphs) => { @@ -116,8 +114,7 @@ test('GlyphManager generates CJK PBF locally', (t) => { } }); - const manager = new GlyphManager((url) => ({url}), 'sans-serif'); - manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf'); + const manager = createGlyphManager('sans-serif'); manager.getGlyphs({'Arial Unicode MS': [0x5e73]}, (err, glyphs) => { t.ifError(err); @@ -134,8 +131,7 @@ test('GlyphManager generates Katakana PBF locally', (t) => { } }); - const manager = new GlyphManager((url) => ({url}), 'sans-serif'); - manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf'); + const manager = createGlyphManager('sans-serif'); // Katakana letter te manager.getGlyphs({'Arial Unicode MS': [0x30c6]}, (err, glyphs) => { @@ -153,8 +149,7 @@ test('GlyphManager generates Hiragana PBF locally', (t) => { } }); - const manager = new GlyphManager((url) => ({url}), 'sans-serif'); - manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf'); + const manager = createGlyphManager('sans-serif'); //Hiragana letter te manager.getGlyphs({'Arial Unicode MS': [0x3066]}, (err, glyphs) => { @@ -174,8 +169,7 @@ test('GlyphManager caches locally generated glyphs', (t) => { } }); - const manager = new GlyphManager((url) => ({url}), 'sans-serif'); - manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf'); + const manager = createGlyphManager('sans-serif'); // Katakana letter te manager.getGlyphs({'Arial Unicode MS': [0x30c6]}, (err, glyphs) => {