1376026 - Blob images don't work on Windows

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

The dwrite fonts depend on some stuff from gfxPlatform which we intentionally don't have in the GPU process.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Lee, any thoughts or feelings about what we should do?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make things hobble along

Comment 3

[Lee Salzman [:lsalzman]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> Lee, any thoughts or feelings about what we should do?

Would possibly make more sense to pass in the gamma and contrast as parameters to SkCreateTypefaceFromDWriteFont, and then do the resolution of those values higher up, probably in thebes, which would mean plumbing to pass them along in ScaledFontDWrite as well.

Comment 4

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #3)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> > Lee, any thoughts or feelings about what we should do?
> 
> Would possibly make more sense to pass in the gamma and contrast as
> parameters to SkCreateTypefaceFromDWriteFont, and then do the resolution of
> those values higher up, probably in thebes, which would mean plumbing to
> pass them along in ScaledFontDWrite as well.

I'll take a look into doing this during the all hands. Should be something that's up my alley.

Comment 5

[Lee Salzman [:lsalzman]]

Discussed with Jeff. I'll work on fixing this.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: fix plumbing of DWrite parameters for Skia fonts to not depend on gfxPlatform

This patch ranges a bit far through the code, but it is mostly all in the service of one theme: sanitizing the DWrite plumbing.

Firstly, we had dwrite.dll loading happening in multiple places, so Factory::EnsureDWriteFactory was added to centralize this. Since it needs to be called from multiple threads, it required locking to be safe.

Secondly, we need to ensure that Skia isn't calling back into gfxWindowsPlatform to get the dwrite parameters (gamma and contrast). Since these are associated with the font, I added plumbing in ScaledFontDWrite to just store these and pass these in during Skia typeface creation. I used the InstanceData mechanism I had added for ScaledFontFonconfig for this so that we can serialize the gamma/contrast and send it through the recording stream without having to query this from the DWrite on the other side.

This way, Skia has everything it needs and does not need to call back to Gecko to get it. We also no longer need GlyphRenderingOptionsDWrite for this purpose, which made little sense anyway given that these parameters were strictly associated with the font, and not the text run being rendered.

Thirdly, CreateNativeFontResource was cleaned up so that it now always has a specific defined type of font resource to create that gets recorded. This was necessary since on the playback side we can't always guarantee we'll reproduce the correct decision-making that selected whether to use DWrite or GDI, so better to just write in the stream if we started with DWrite or GDI so the correct decision is always made.

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: assume DWrite is available on Windows 7 even without the platform update

After discussion with Mason, who added a check for D2D support before using DWrite to gfxWindowsPlatform, it turns out the MSDN documentation, in its own hard to understand way, was not actually meaning that we would need D2D support for our current usage of DWrite. So we can just remove this check to simplify our code.

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: fix plumbing of DWrite parameters for Skia fonts to not depend on gfxPlatform

V2 that now just assumes if a DWrite factory can be created, then we should use DWrite, without requiring D2D support.

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: fix plumbing of DWrite parameters for Skia fonts to not depend on gfxPlatform

V3, add a comment describing why we need to store both the IDWriteRenderingParams and gamma/contrast values for use in DrawTargetD2D1 and DrawTargetSkia.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a93c71a7ef8b
assume DWrite is available on Windows 7 even without the platform update. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d09fb3a5cedf
fix plumbing of DWrite parameters for Skia fonts to not depend on gfxPlatform. r=jrmuizel

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/581652145f1b
fix DWriteEnabled usage. r=me

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af8c0e8089fc
fix ScaledFontDWrite::InstanceData constructor to be explicit. r=me

Comment 13

[Carsten Book [:Tomcat]]

backed this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e0f5d38b04fcda5f2b286eef5e437989ba4cb227&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception because it seems this or the other bug caused https://treeherder.mozilla.org/logviewer.html#?job_id=112166204&repo=mozilla-inbound

Comment 14

[Carsten Book [:Tomcat]]

erm s/the other bug/ the followups :)

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db63eab6880e
Backed out changeset af8c0e8089fc 
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd3d0f4eacd
Backed out changeset 581652145f1b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/47ad4ca0f02f
Backed out changeset d09fb3a5cedf

Comment 16

[Lee Salzman [:lsalzman]]

Attachment: fix plumbing of DWrite parameters for Skia fonts to not depend on gfxPlatform

Fixed some build failures.

Comment 17

[Lee Salzman [:lsalzman]]

Attachment: fix tests to assume that Windows 7 always uses DWrite

Our Win7 VM test machines were not using DWrite previously, so enabling DWrite with it (on any Win7 machine) caused some diverging test results. This fixes some fuzz and unexpected passes.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be7d6aa4aef3
assume DWrite is available on Windows 7 even without the platform update. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/24421d72ba08
fix plumbing of DWrite parameters for Skia fonts to not depend on gfxPlatform. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/387e4a353e65
fix tests to assume that Windows 7 always uses DWrite. r=jrmuizel

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser_bug970746.js on Windows 7 VM:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b64554d7181e07f1d8dd7718c211acbf68d2a761
https://hg.mozilla.org/integration/mozilla-inbound/rev/39cb5de383f45c85f3938ebe6cd12e6631bf4773
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5912642f421bdd6061be45621fc0d94e675be6f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b64554d7181e07f1d8dd7718c211acbf68d2a761&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=112645434&repo=mozilla-inbound

18:42:16     INFO -  394 INFO TEST-PASS | browser/base/content/test/general/browser_bug970746.js | Menu item text 'Search Google for “I'm some text, …”' contains the correct search terms 'I'm some text, …' -
18:42:16     INFO -  395 INFO TEST-PASS | browser/base/content/test/general/browser_bug970746.js | search context menu item is shown for  '#mixedContent' and selected is 'false' -
18:42:16     INFO -  396 INFO TEST-PASS | browser/base/content/test/general/browser_bug970746.js | search context menu item is shown for  '#partialLink' and selected is 'true' -
18:42:16     INFO -  397 INFO TEST-PASS | browser/base/content/test/general/browser_bug970746.js | Menu item text 'Search Google for “link selection”' contains the correct search terms 'link selection' -
18:42:16     INFO -  Buffered messages finished
18:42:16    ERROR -  398 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug970746.js | search context menu item is shown for  '#partialLink' and selected is 'false' - Got true, expected false
18:42:16     INFO -  Stack trace:
18:42:16     INFO -  chrome://mochikit/content/browser-test.js:test_is:967
18:42:16     INFO -  chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug970746.js:null:106

Comment 20

[Lee Salzman [:lsalzman]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #19)
> Backed out for failing browser_bug970746.js on Windows 7 VM:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> b64554d7181e07f1d8dd7718c211acbf68d2a761
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 39cb5de383f45c85f3938ebe6cd12e6631bf4773
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> e5912642f421bdd6061be45621fc0d94e675be6f
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=b64554d7181e07f1d8dd7718c211acbf68d2a761&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=retry&filter-
> resultStatus=usercancel&filter-resultStatus=runnable
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=112645434&repo=mozilla-
> inbound
> 
> 18:42:16     INFO -  394 INFO TEST-PASS |
> browser/base/content/test/general/browser_bug970746.js | Menu item text
> 'Search Google for “I'm some text, …”' contains the correct search terms
> 'I'm some text, …' -
> 18:42:16     INFO -  395 INFO TEST-PASS |
> browser/base/content/test/general/browser_bug970746.js | search context menu
> item is shown for  '#mixedContent' and selected is 'false' -
> 18:42:16     INFO -  396 INFO TEST-PASS |
> browser/base/content/test/general/browser_bug970746.js | search context menu
> item is shown for  '#partialLink' and selected is 'true' -
> 18:42:16     INFO -  397 INFO TEST-PASS |
> browser/base/content/test/general/browser_bug970746.js | Menu item text
> 'Search Google for “link selection”' contains the correct search terms 'link
> selection' -
> 18:42:16     INFO -  Buffered messages finished
> 18:42:16    ERROR -  398 INFO TEST-UNEXPECTED-FAIL |
> browser/base/content/test/general/browser_bug970746.js | search context menu
> item is shown for  '#partialLink' and selected is 'false' - Got true,
> expected false
> 18:42:16     INFO -  Stack trace:
> 18:42:16     INFO -  chrome://mochikit/content/browser-test.js:test_is:967
> 18:42:16     INFO - 
> chrome://mochitests/content/browser/browser/base/content/test/general/
> browser_bug970746.js:null:106

Jared, it would appear that the test introduced in bug 970746 is failing here for a reason that I can't really discern, but only on the Windows 7 VM test machine. What is special about this category of test machine is it does not support Direct2D, and as of this bug, we would be enabling DirectWrite on it. Why this would cause the search-select item in the context menu to not show up is beyond me, as on a Windows 7 machine with Direct2D enabled, I am not able to reproduce this scenario locally, and there is nothing superficially wrong with the test.

I can't see any obvious reason why changing from GDI fonts to DirectWrite fonts would cause an unrelated context menu item to not show up, and only in that particular scenario without Direct2D. The non-VM Windows 7 test machines handle the test case fine. No other browser chrome tests with the context menu seem affected, so this is all just very strange and indecipherable.

I would like permission to simply disable this browser chrome test on Windows 7 so that we can get the fixes in this bug unblocked and proceed on Quantum Render work. The test would remain enabled for all other Windows test machines. I could file a bug against the partially disabled test so someone with more insight could look into it.

Comment 21

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #20)
> (In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on
> intermittent or backout) from comment #19)
> > Backed out for failing browser_bug970746.js on Windows 7 VM:
> > 
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > b64554d7181e07f1d8dd7718c211acbf68d2a761
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > 39cb5de383f45c85f3938ebe6cd12e6631bf4773
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > e5912642f421bdd6061be45621fc0d94e675be6f
> > 
> > Push with failures:
> > https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> > inbound&revision=b64554d7181e07f1d8dd7718c211acbf68d2a761&filter-
> > resultStatus=testfailed&filter-resultStatus=busted&filter-
> > resultStatus=exception&filter-resultStatus=retry&filter-
> > resultStatus=usercancel&filter-resultStatus=runnable
> > Failure log:
> > https://treeherder.mozilla.org/logviewer.html#?job_id=112645434&repo=mozilla-
> > inbound
> > 
> > 18:42:16     INFO -  394 INFO TEST-PASS |
> > browser/base/content/test/general/browser_bug970746.js | Menu item text
> > 'Search Google for “I'm some text, …”' contains the correct search terms
> > 'I'm some text, …' -
> > 18:42:16     INFO -  395 INFO TEST-PASS |
> > browser/base/content/test/general/browser_bug970746.js | search context menu
> > item is shown for  '#mixedContent' and selected is 'false' -
> > 18:42:16     INFO -  396 INFO TEST-PASS |
> > browser/base/content/test/general/browser_bug970746.js | search context menu
> > item is shown for  '#partialLink' and selected is 'true' -
> > 18:42:16     INFO -  397 INFO TEST-PASS |
> > browser/base/content/test/general/browser_bug970746.js | Menu item text
> > 'Search Google for “link selection”' contains the correct search terms 'link
> > selection' -
> > 18:42:16     INFO -  Buffered messages finished
> > 18:42:16    ERROR -  398 INFO TEST-UNEXPECTED-FAIL |
> > browser/base/content/test/general/browser_bug970746.js | search context menu
> > item is shown for  '#partialLink' and selected is 'false' - Got true,
> > expected false
> > 18:42:16     INFO -  Stack trace:
> > 18:42:16     INFO -  chrome://mochikit/content/browser-test.js:test_is:967
> > 18:42:16     INFO - 
> > chrome://mochitests/content/browser/browser/base/content/test/general/
> > browser_bug970746.js:null:106
> 
> Jared, it would appear that the test introduced in bug 970746 is failing
> here for a reason that I can't really discern, but only on the Windows 7 VM
> test machine. What is special about this category of test machine is it does
> not support Direct2D, and as of this bug, we would be enabling DirectWrite
> on it. Why this would cause the search-select item in the context menu to
> not show up is beyond me, as on a Windows 7 machine with Direct2D enabled, I
> am not able to reproduce this scenario locally, and there is nothing
> superficially wrong with the test.
> 
> I can't see any obvious reason why changing from GDI fonts to DirectWrite
> fonts would cause an unrelated context menu item to not show up, and only in
> that particular scenario without Direct2D. The non-VM Windows 7 test
> machines handle the test case fine. No other browser chrome tests with the
> context menu seem affected, so this is all just very strange and
> indecipherable.
> 
> I would like permission to simply disable this browser chrome test on
> Windows 7 so that we can get the fixes in this bug unblocked and proceed on
> Quantum Render work. The test would remain enabled for all other Windows
> test machines. I could file a bug against the partially disabled test so
> someone with more insight could look into it.

Rather, I meant to say above "as on a Windows 7 mcahine with Direct2D forcefully disabled"

Comment 22

[Lee Salzman [:lsalzman]]

Attachment: disable test for bug 970746 on Windows 7

Separated out this particular change so it could be independently reviewed. See comment 20 for reasoning.

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8884919 [details] [diff] [review]
disable test for bug 970746 on Windows 7

Review of attachment 8884919 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser.ini
@@ +309,5 @@
>  # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
>  [browser_bug882977.js]
>  # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
>  [browser_bug970746.js]
> +skip-if = (os == "win" && bits == 32) # Disabled on Windows 7 (No D2D but supports DWrite) due to unexplained consequence of bug 1376026

I'm pretty sure we would still want to run this test on other 32-bit Windows platforms.

Instead of making the change here, please add the following at the beginning of the test:

> add_task(function() {
>   if (AppConstants.isPlatformAndVersionAtMost("win", "6.1")) {
>     ok(true, "This test is disabled on Windows 7 (no D2D but supports DWrite) due to unexplained consequence of bug 1376026");
>     return;
>   }
> 
>   ... continue with the rest of the test ...
> }

We don't support anything before Windows 7 now so this check is fine.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e738e018fe57
assume DWrite is available on Windows 7 even without the platform update. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5b7263f383d
fix plumbing of DWrite parameters for Skia fonts to not depend on gfxPlatform. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d22cfe83584a
fix tests to assume that Windows 7 always uses DWrite. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a96e12230d
disable test for bug 970746 on Windows 7. r=jaws

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e738e018fe57
https://hg.mozilla.org/mozilla-central/rev/d5b7263f383d
https://hg.mozilla.org/mozilla-central/rev/d22cfe83584a
https://hg.mozilla.org/mozilla-central/rev/86a96e12230d

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf5603f8b181
follow-up - fix ScaledFontDWrite construct parameter order. r=me

Comment 27

[Carsten Book [:Tomcat]]

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