Graphite automatic collision fixing no longer works.

RESOLVED FIXED in Firefox 57

Status

()

Core
Graphics: Text
P3
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: Mark Straver, Assigned: Kevin Hsieh)

Tracking

(Depends on: 1 bug, {regression})

Trunk
mozilla57
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed)

Details

(Whiteboard: [gfx-noted], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
Graphite issue: Awami Nastaliq automatic collision fixing no longer works in Nightly. This worked properly before (ESR52, Fx 55).
See the URL for a demo.
Looks like the OTS Graphite support is rejecting this font: it shows console messages

downloadable font: Glat: Decompression failed (font-family: "AwamiT" style:normal weight:normal stretch:normal src index:0) source: http://scripts.sil.org/pub/woff/fonts/AwamiNastaliq-Regular.woff unknown:80:11
downloadable font: Dropping all Graphite tables (font-family: "AwamiT" style:normal weight:normal stretch:normal src index:0) source: http://scripts.sil.org/pub/woff/fonts/AwamiNastaliq-Regular.woff

So we need to look into that, and figure out why it's failing.
(Assignee)

Comment 2

4 months ago
I'll look into this.
(Assignee)

Updated

3 months ago
Assignee: nobody → kevin.hsieh
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 months ago
The Awami Nastaliq font has padding after its compressed data, so LZ4 complains about unconsumed input. I've submitted an upstream PR that accounts for this. The font also has fields that are calculated in a way that is inconsistent with older Graphite fonts; jfkthame has submitted relevant upstream PR's.
Comment hidden (mozreview-request)

Comment 5

3 months ago
mozreview-review
Comment on attachment 8905613 [details]
Bug 1396026 - Update OTS to accept Awami Nastaliq.

https://reviewboard.mozilla.org/r/177410/#review182428

r=me for the OTS update here, but I think we should also ask one of the mfbt owners about the API change in Compression.h ... are they OK with adding the optional `partial` parameter, or would an alternative approach be preferred? (Another option would be to add a separate LZ4::decompressPartial wrapper to expose the LZ4 function we need.)
Attachment #8905613 - Flags: review?(jfkthame) → review+
Attachment #8905613 - Flags: review?(nfroyd)
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1395053

Comment 7

3 months ago
mozreview-review
Comment on attachment 8905613 [details]
Bug 1396026 - Update OTS to accept Awami Nastaliq.

https://reviewboard.mozilla.org/r/177410/#review182526

::: mfbt/Compression.h:93
(Diff revision 1)
> +   * @param partial is set to true when performing a partial decompression
> +   *   (i.e., to ignore unconsumed input upon reaching aMaxOutputSize)
>     * @return true on success, false on failure
>     */
>    static MFBT_API MOZ_MUST_USE bool
>    decompress(const char* aSource, size_t aInputSize, char* aDest,
> -             size_t aMaxOutputSize, size_t* aOutputSize);
> +             size_t aMaxOutputSize, size_t* aOutputSize, bool partial = false);

Drive-by stylistic nit: s/partial/aPartial/

(In the function signature &  documentation in the .h file, and in the impl in the .cpp file)
Comment hidden (mozreview-request)
Priority: -- → P3
Whiteboard: [gfx-noted]
Comment on attachment 8905613 [details]
Bug 1396026 - Update OTS to accept Awami Nastaliq.

https://reviewboard.mozilla.org/r/177410/#review183800

Sorry for the delay in looking at this.  r=me with the changes below; please remember to add a documentation comment for the new function.  Thanks!

::: mfbt/Compression.cpp:72
(Diff revision 2)
> -  int ret = LZ4_decompress_safe(aSource, aDest, inputSizeChecked.value(),
> +  int ret = aPartial ? LZ4_decompress_safe_partial(aSource, aDest,
> +                                                  inputSizeChecked.value(),
> +                                                  maxOutputSizeChecked.value(),
> +                                                  maxOutputSizeChecked.value())
> +                     : LZ4_decompress_safe(aSource, aDest,
> +                                           inputSizeChecked.value(),
> -                                maxOutputSizeChecked.value());
> +                                           maxOutputSizeChecked.value());

I think I have a small preference for exposing this as a separate function, `LZ4::decompressPartial`, for consistency with the exposed LZ4 API.
Attachment #8905613 - Flags: review?(nfroyd) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 11

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/35dcbab0bdbd
Update OTS to accept Awami Nastaliq. r=froydnj,jfkthame
Keywords: checkin-needed

Comment 12

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/35dcbab0bdbd
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1369672
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox-esr52: --- → unaffected
Depends on: 1406833
You need to log in before you can comment on or make changes to this bug.