Closed Bug 1396026 Opened 7 years ago Closed 7 years ago

Graphite automatic collision fixing no longer works.

Categories

(Core :: Graphics: Text, defect, P3)

x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: mark, Assigned: kevin.hsieh)

References

()

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(1 file)

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.
I'll look into this.
Assignee: nobody → kevin.hsieh
Status: NEW → ASSIGNED
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 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)
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)
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/35dcbab0bdbd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1406833
You need to log in before you can comment on or make changes to this bug.