[d2d][grafxbot] compareCanvases fails on reftest == clipPath-advanced-01.svg[accelerated] pass.svg[accelerated], Win7, ATI Mobility Radeon HD 4650

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jgriffin, Assigned: bas.schouten)

Tracking

Trunk
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

9 years ago
When the reftest '== clipPath-advanced-01.svg[accelerated] pass.svg[accelerated]' is run in hardware accelerated mode on a Win 7 machine with the ATI Mobility Radeon HD 4650, nsIDOMWindowUtils.compareCanvases fails with the following exception:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.compareCanvases]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://grafxbot/content/grafxbot.js :: DocumentLoaded :: line 978"  data: no]

The relevant line in script is:

differences = gWindowUtils.compareCanvases(gCanvas1, gCanvas2, {});

No other SVG reftest causes this failure.  This failure is not observed on a WinXP VM.
Assignee

Comment 1

9 years ago
This is the result of the thebes context going into error when CreateSharedBitmap fails with a D2DERR_UNSUPPORTED_PIXEL_FORMAT for an A8_UNORM DXGI surface. I have no theory as to why it fails, the documentation suggests this should be fine. And we need it to be fine. Looking into this.
Assignee

Comment 2

9 years ago
(In reply to comment #0)
> No other SVG reftest causes this failure.  This failure is not observed on a
> WinXP VM.

Under WinXP Direct2D isn't available, so this makes sense :-).
I think the point is that why do the other SVG reftests work under D2D since filters, masks and clipPaths all create bitmaps of one kind or another :-)
Assignee

Comment 4

9 years ago
With this patch we create a bitmap at brush creation and copy the contents of the render target to it in case we failed to create a surfaceBitmap. These should be rare cases and this gives us a consistent code path for all of them. Performance impact should be minimal compared to the old approach for HWND sources.
Assignee: nobody → bas.schouten
Status: NEW → ASSIGNED
Attachment #437547 - Flags: review?(jmuizelaar)
Can we find out a little more thoroughly what's going on here? If CreateSharedBitmap doesn't work with A8_UNORM I don't really see the point in trying it and having it fail.
Assignee

Comment 6

9 years ago
(In reply to comment #5)
> Can we find out a little more thoroughly what's going on here? If
> CreateSharedBitmap doesn't work with A8_UNORM I don't really see the point in
> trying it and having it fail.

True, it fails on all my hardware, but I was thinking it may work with some hardware or drivers so I figured there was no harm in just trying it. The D2D documentation is not clear about the specifics, there is certainly the possibility of alpha only bitmaps (evidently because CreateBitmap just succeeds), but CreateSharedBitmap seems to consistently fail on A8_UNORM textures.
Posted patch Rebased patch (obsolete) — Splinter Review
Rebased patch to latest tip (41057). Includes RefPtr changes.
Comment on attachment 437547 [details] [diff] [review]
Create bitmap at brush creation

I'd rather not have the code do something different if CreateSharedBitmap() succeeds. Let's just assume that it doesn't and keep the code paths unified.
Attachment #437547 - Flags: review?(jmuizelaar) → review-
Assignee

Comment 9

9 years ago
(In reply to comment #8)
> (From update of attachment 437547 [details] [diff] [review])
> I'd rather not have the code do something different if CreateSharedBitmap()
> succeeds. Let's just assume that it doesn't and keep the code paths unified.

That would regress performance and increase memory usage though.
Assignee

Comment 10

9 years ago
Attachment #437547 - Attachment is obsolete: true
Attachment #440889 - Attachment is obsolete: true
Attachment #444497 - Flags: review?(jmuizelaar)
Reporter

Comment 11

9 years ago
This problem was just observed by a testday tester ('dark_skeleton') on the following:

GeForce 9600M GT, 512MB RAM
win7 x64, intel c2d P7350 2.0GHz 1066 FSB, 3GB RAM
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100521 Minefield/3.7a5pre ID:20100521035955
Reporter

Comment 12

9 years ago
Re: comment #11, the video driver version in use is:

nVidia Forceware 197.75
Reporter

Comment 13

9 years ago
More details for the problem in comment #11:

Display Vendor: NVIDIA (0x10DE)
Display Adapter: NVIDIA GeForce 9600M GT / GeForce GT 220M
Display Chipset: 0x0649
Display RAM: 512 MB
Display Drivers: nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um
Driver Version: 8.17.11.9775, 4-28-2010
Display Settings: 1280 x 800 x 32
Reporter

Comment 14

9 years ago
Also noted on nVidia 240m, Windows 7
Reporter

Comment 15

9 years ago
more details about the failure noted in comment #14:

      Display Vendor: NVIDIA (0x10DE)
      Display Adapter: NVIDIA GeForce GT 240M
      Display Chipset: 0x0A34
      Display RAM: 1024 MB
      Display Drivers: nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um
      Driver Version: 8.17.11.9716, 3-16-2010
      Display Settings: 1366 x 768 x 32
Reporter

Comment 16

9 years ago
Also seen on:

one laptop with vista x32 and nvidia 8400m gs 
and win7 x64 on nvidia 8800gts

both with most recent drivers
Reporter

Comment 17

9 years ago
one of the testday testers made a build with this patch and verified that it resolved the problem on his machine

Comment 18

9 years ago
Yes, if anyone wants to take a look at it http://dl.dropbox.com/u/686313/firefox-3.7a5pre.en-US.win32.zip

mk_add_options MOZ_CO_PROJECT=browser

ac_add_options --enable-application=browser
ac_add_options --disable-optimize
ac_add_options --disable-debug
ac_add_options --disable-tests
ac_add_options --disable-jemalloc
ac_add_options --disable-update-packaging
Assignee

Updated

9 years ago
Blocks: 569166
Comment on attachment 444497 [details] [diff] [review]
Use unified fallback for windows and all A8_UNORM surfaces


>     bitProps = D2D1::BitmapProperties(D2D1::PixelFormat(DXGI_FORMAT_UNKNOWN, 
>-									       alpha));
>-    hr = newSurf->rt->CreateSharedBitmap(IID_IDXGISurface,
>-					 dxgiSurface,
>-					 &bitProps,
>-					 &newSurf->surfaceBitmap);
>+				      alpha));
> 
>-    if (FAILED(hr)) {
>+    if (dxgiformat != DXGI_FORMAT_A8_UNORM) {

This should have a comment about why A8_UNORM is special. Perhaps something like CreateSharedBitmap doesn't support A8 surfaces despite what the documentation seems to claim. For what it's worth CreateSharedBitmap seems to specifically check for A8 and return an error.
Attachment #444497 - Flags: review?(jmuizelaar) → review-
Assignee

Comment 20

9 years ago
Updated to apply to m-c and address review comments.
Attachment #444497 - Attachment is obsolete: true
Attachment #451396 - Flags: review?(jmuizelaar)
Attachment #451396 - Flags: review?(jmuizelaar) → review+
Assignee

Comment 21

9 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/db3e8b1ecb26.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.