1348584 - Windows ClearType Enhanced Contrast: 300 causes fatal error: "assert(0 <= c && c <= SK_Scalar1)"

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jorg K (CEST = GMT+2)]

I'm running a Thunderbird debug build, pretty much current at time of writing.

That build worked earlier today, but now I'm getting a crash:

c:\mozilla-source\comm-central\mozilla\gfx\skia\skia\src\core\SkScalerContext.h:77: fatal error: "assert(0 <= c && c <= SK_Scalar1)"
Abort from sk_abort
Hit MOZ_CRASH() at c:/mozilla-source/comm-central/mozilla/memory/mozalloc/mozalloc_abort.cpp:33

So I printed out the value of c and SK_Scalar1:
    void setContrast(SkScalar c) {
        printf("Skia: %f %f\n", c, SK_Scalar1);
        SkASSERT(0 <= c && c <= SK_Scalar1);
        fContrast = SkScalarRoundToInt(c * ((1 << 8) - 1));
    }

Result on two calls, the second crashes:
Skia: 0.500000 1.000000
Skia: 3.000000 1.000000

I have no idea what's going on here since I'm not familiar with graphics.

This is a Windows 10 x64 PC with a x64 build of Thunderbird.

I can get you a better stack if you want, this is called from
void DWriteFontTypeface::onFilterRec(SkScalerContext::Rec* rec)
    IDWriteRenderingParams* params = GetDwriteRenderingParams(ForceGDI());
    SkASSERT(params);
    rec->setContrast(params->GetEnhancedContrast());

I will only have access to this machine for one week, so if you want me to experiment, you need to ask now.

Comment 1

[Jorg K (CEST = GMT+2)]

OK, looking at the troubleshooting information, I see:
ClearType Parameters	Gamma: 2.2 Pixel Structure: RGB ClearType Level: 50 Enhanced Contrast: 300

Please don't ask how I managed to get those settings ;-)

Comment 2

[Jorg K (CEST = GMT+2)]

HKEY_CURRENT_USER\SOFTWARE\Microsoft\Avalon.Graphics\DISPLAY1 is set to 300. No idea how that happened, but it shouldn't crash, not even in debug mode.

Comment 3

[Michael Leu[:Lenzak](UTC+8)]

I encountered exactly same startup crash in Firefox nightly.

And my EnhancedContrastLevel was set to 300, too.

The crash can be fixed by setting EnhancedContrastLevel to 0, but it should not happen.

I think it is a regression introduced quite recently because I came across this issue yesterday after a clobber build.

Comment 4

[Jorg K (CEST = GMT+2)]

Lee and Mason: Any chance of getting this fixed, it's affecting developers.

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Limit contrast to 0-1 if we have a skia backend

This will revert us to the behavior pre- bug 1345222. Essentially, if the contrast isn't between 0-1 and we have skia, default back to 1.0.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8850095 [details] [diff] [review]
Limit contrast to 0-1 if we have a skia backend

This works for me, thanks. No one asked me for feedback, but since I filed the bug, I might was well show my appreciation. The warning shows up once, which is fine.

Comment 8

[Mason Chang [Inactive] [:mchang]]

(In reply to Jorg K (GMT+1) from comment #7)
> Comment on attachment 8850095 [details] [diff] [review]
> Limit contrast to 0-1 if we have a skia backend
> 
> This works for me, thanks. No one asked me for feedback, but since I filed
> the bug, I might was well show my appreciation. The warning shows up once,
> which is fine.

Thanks for filing and testing!

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 8850095 [details] [diff] [review]
Limit contrast to 0-1 if we have a skia backend

I found a document:
https://msdn.microsoft.com/en-us/library/windows/desktop/dd371290(v=vs.85).aspx
> Remarks
> 
> Enhanced contrast is the amount to increase the darkness of text, and
> typically ranges from 0 to 1. Zero means no contrast enhancement.

although it is not a hard requirement for DWrite backend.

Comment 10

[Masatoshi Kimura [:emk]]

We will need uplift to whatever branches that bug 1345222 uplifted to.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b95938854605
Default to 1.0 contrast for Skia backends if custom contrast dwrite param isn't supported by skia. r=emk

Comment 12

[Mason Chang [Inactive] [:mchang]]

Try looks good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d8039e6dab01c9a85b86f928e08069e77fc98e9

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b95938854605

Comment 14

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8850095 [details] [diff] [review]
Limit contrast to 0-1 if we have a skia backend

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1345222
[User impact if declined]: Debug builds will assert crash. Users with custom cleartype contrast parameters will see no text or very colorful pixels shaped somewhat like text.
[Is this code covered by automated tests?]: No, manually tested and verified.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No. 
[Why is the change risky/not risky?]: this reverts us to the behavior for skia pre bug 1345222.
[String changes made/needed]: None

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8850095 [details] [diff] [review]
Limit contrast to 0-1 if we have a skia backend

Fix an assert crash. Aurora54+ & Beta53+.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9e9bd62d163a

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/62c0bc2f6c9c

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8850095 [details] [diff] [review]
Limit contrast to 0-1 if we have a skia backend

we need this in esr52 along with bug 1345222

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/7cee9ad555af

Comment 20

[Alfred Peters [:infofrommozilla]]

I am experiencing the Bug just in a current trunk (TB & FF 56.0a1) build.

| s:\Projects\mozi\comm-central\mozilla\gfx\skia\skia\src\core\SkScalerContext.h:76: fatal error: "assert(0 <= c && c <= SK_Scalar1)"
| Abort from sk_abort
| Hit MOZ_CRASH() at s:/Projects/mozi/comm-central/mozilla/memory/mozalloc/mozalloc_abort.cpp:33
| [GPU 8164] WARNING: Shutting down GPU process early due to a crash!: file s:/Projects/mozi/comm-central/mozilla/gfx/ipc/GPUParent.cpp, line 399

Is this fix incomplete? Or is it a regression?

I traced it back to this call to: "params->GetEnhancedContrast()"
<https://dxr.mozilla.org/comm-central/source/mozilla/gfx/thebes/gfxDWriteFonts.cpp#718>

This seems to bypass your fix.

Comment 21

[Alfred Peters [:infofrommozilla]]

(In reply to Alfred Peters from comment #20)
> Is this fix incomplete? Or is it a regression?

A regression of Bug 1376026