Closed Bug 765845 Opened 11 years ago Closed 11 years ago

Hang in DrawTargetD2D::FillGlyphs


(Core :: Graphics, defect)

Windows 7
Not set



Tracking Status
firefox15 + fixed


(Reporter: bent.mozilla, Assigned: bas.schouten)



(1 file, 1 obsolete file)

Whenever I use my laptop on battery Firefox seems to slow to a crawl. I am becoming more and more convinced that the slowdown I see is due to our 3d acceleration, probably because NVIDIA switches to a very slow low-power mode. Recently I started collecting minidumps whenever Firefox enters a hang state (where the window gets greyed out and the title bar says "Not Responding"). I collected about ten different dumps and each one had the following stack:

Main thread:
  DWrite.dll!operator!=()  + 0x19 bytes
  DWrite.dll!GlyphDataBlock<GlyphBitmapLayout,GlyphBitmapRasterizationParameters,GlyphBitmapRasterizationState>::IsEqualKeyImpl()  + 0x3a bytes
  DWrite.dll!CacheReader::LookupElement()  + 0xe6 bytes
  DWrite.dll!GlyphDataBlock<GlyphBitmapLayout,GlyphBitmapRasterizationParameters,GlyphBitmapRasterizationState>::FillFromReadOnlyCache()  + 0x76 bytes
  DWrite.dll!GlyphDataBlock<GlyphBitmapLayout,GlyphBitmapRasterizationParameters,GlyphBitmapRasterizationState>::ServerLookup()  + 0xa8 bytes
  DWrite.dll!GlyphDataBlock<GlyphBitmapLayout,GlyphBitmapRasterizationParameters,GlyphBitmapRasterizationState>::GetGlyphData()  + 0x57 bytes
  DWrite.dll!GetGlyphBitmaps()  + 0x93 bytes
  DWrite.dll!GlyphRunAnalysis::GlyphRunAnalysis()  + 0x2fc bytes
  DWrite.dll!DWriteFactory::CreateGlyphRunAnalysis()  + 0xfb bytes
  d2d1.dll!GlyphRunRealizer::Init()  + 0xf8 bytes
  d2d1.dll!CHwSurfaceRenderTarget::DrawGlyphRunInternal()  + 0xbc bytes
  d2d1.dll!CCommand_DrawGlyphRun::Execute()  + 0x27 bytes
  d2d1.dll!CHwSurfaceRenderTarget::ProcessBatch()  + 0x4c bytes
  d2d1.dll!CBatchSerializer::FlushInternal()  + 0x2e bytes
  d2d1.dll!CBatchSerializer::FlushInternalAndTryIncreaseStorage()  + 0x36 bytes
  d2d1.dll!CBatchSerializer::StartCommand<CCommand_SetTextRenderingParams>()  + 0xc bytes
  d2d1.dll!DrawingContext::SetTextRenderingParams()  + 0x34 bytes
  d2d1.dll!D2DRenderTargetBase<ID2D1HwndRenderTarget>::SetTextRenderingParams()  + 0x4e bytes
> gkmedias.dll!mozilla::gfx::DrawTargetD2D::FillGlyphs(mozilla::gfx::ScaledFont * aFont, const mozilla::gfx::GlyphBuffer & aBuffer, const mozilla::gfx::Pattern & aPattern, const mozilla::gfx::DrawOptions & aOptions, const mozilla::gfx::GlyphRenderingOptions * aRenderOptions)  Line 886
  xul.dll!gfxContext::AzureState::AzureState(const gfxContext::AzureState & __that)  + 0x80 bytes

Is there anything that can be done about this?

Here are my graphics stats:

Adapter Description
  Intel(R) HD Graphics Family
Vendor ID
Device ID
Adapter RAM
Adapter Drivers
  igdumd64 igd10umd64 igd10umd64 igdumdx32 igd10umd32 igd10umd32
Driver Version
Driver Date
Adapter Description (GPU #2)
  NVIDIA Quadro 2000M
Vendor ID (GPU #2)
Device ID (GPU #2)
Adapter RAM (GPU #2)
Adapter Drivers (GPU #2)
  nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um
Driver Version (GPU #2)
Driver Date (GPU #2)
Direct2D Enabled
DirectWrite Enabled
  true (6.1.7601.17789)
ClearType Parameters
  ClearType parameters not found
WebGL Renderer
  Blocked for your graphics card because of unresolved driver issues.
GPU Accelerated Windows1/1
  Direct3D 10
Hrm, we will be calling this a little more with Azure content than without. Has this problem started recently? The fix should be easy. But I should fix this on aurora as well.
This makes us call TextRenderingParams as often as we do with Cairo. I never really knew this could cause a flush of the command stream so could be expensive.
Assignee: nobody → bas.schouten
Attachment #634260 - Flags: review?(jmuizelaar)
(In reply to Bas Schouten (:bas) from comment #1)
> Has this problem started recently?

Not sure about this particular stack, but FF has been really slow on battery power for a while now. I try to be plugged in whenever I can because of it so I just don't know.
It's not clear that this is a recent regression, so no need to track for release. That being said, we would consider an uplift to Aurora 15 once fixed.
If this patch fixes the bug, then it's a regression from bug 715768, which landed for Firefox 15.

Ben, can you do a build with this patch in it?
Wait, if this reduces the number of times we call this function that clearly causes an expensive blocking operation why wouldn't we take it?

I can do a build sometime soon, I guess.
Oh, we'd take it, but I'd like to know if it isn't fixed :)
Oops, bugzilla changed the flags...
So... It seems that this changes rendering slightly. It's hard to see it, almost. I'll attach some screenshots.
Comment on attachment 634260 [details] [diff] [review]
Reduce calling frequency of SetTextRenderingParams

Review of attachment 634260 [details] [diff] [review]:

::: gfx/2d/DrawTargetD2D.h
@@ +199,5 @@
>    mutable RefPtr<ID2D1RenderTarget> mRT;
> +  // We store this to prevent excessive SetTextRenderingParams calls.
> +  RefPtr<IDWriteRenderingParams> mTextRenderingParams;
> +

This should say why we want to avoid excessive SetTextRenderingParams calls, but maybe that is better off at the SetTextRenderingParams call site.
Attachment #634260 - Flags: review?(jmuizelaar) → review+
Though it is also important to know why the rendering changed.
It looks to me like there are two differences in the font rendering: different hinting behavior (maybe the difference between Natural and GDI-like rendering, for example?), and different gamma correction being used for the colored subpixel-AA pixels.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> Though it is also important to know why the rendering changed.

I guess I could see this happening if it returned the TempRT for drawing somewhere... I'll add something for that to the patch.
The verdict is in, though: this patch fixes the hang I was seeing. Now I see some slowdown on battery but all my profiles point to the layout engine.
This is a regression then, let's fix it. I'll be uploading a new patch in a little bit.
The previous patch was very broken (note how it sets mParams before actually checking if it's equal to params). This patch fixes all correctness issues.
Attachment #634260 - Attachment is obsolete: true
Attachment #636098 - Flags: review?(jmuizelaar)
Attachment #636098 - Flags: review?(jmuizelaar) → review+
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Comment on attachment 636098 [details] [diff] [review]
Reduce calling frequency of SetTextRenderingParams v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 715768
User impact if declined: Severe performance slowdown in some cases
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Very low risk
String or UUID changes made by this patch: None
Attachment #636098 - Flags: approval-mozilla-aurora?
Comment on attachment 636098 [details] [diff] [review]
Reduce calling frequency of SetTextRenderingParams v2

[Triage Comment]
New regression in FF15, with a low risk fix. Approved for Aurora 15.
Attachment #636098 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
[Triage Comment]
This missed landing in when 15 was on Aurora so now needs to land to beta.  Please update the status flags once this has landed.
Comment on attachment 636098 [details] [diff] [review]
Reduce calling frequency of SetTextRenderingParams v2

[Triage Comment]
bumping up the approval nomination accordingly.
Attachment #636098 - Flags: approval-mozilla-aurora+ → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.