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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Griffin (:jgriffin)]

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.

Comment 1

[Bas Schouten (:bas.schouten)]

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.

Comment 2

[Bas Schouten (:bas.schouten)]

(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 :-).

Comment 3

[Robert Longson [:longsonr]]

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 :-)

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Create bitmap at brush creation

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.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 6

[Bas Schouten (:bas.schouten)]

(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.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Rebased patch

Rebased patch to latest tip (41057). Includes RefPtr changes.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 9

[Bas Schouten (:bas.schouten)]

(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.

Comment 10

[Bas Schouten (:bas.schouten)]

Attachment: Use unified fallback for windows and all A8_UNORM surfaces

Comment 11

[Jonathan Griffin (:jgriffin)]

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

Comment 12

[Jonathan Griffin (:jgriffin)]

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

nVidia Forceware 197.75

Comment 13

[Jonathan Griffin (:jgriffin)]

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

Comment 14

[Jonathan Griffin (:jgriffin)]

Also noted on nVidia 240m, Windows 7

Comment 15

[Jonathan Griffin (:jgriffin)]

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

Comment 16

[Jonathan Griffin (:jgriffin)]

Also seen on:

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

both with most recent drivers

Comment 17

[Jonathan Griffin (:jgriffin)]

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

Comment 18

[Kevin Brosnan]

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

Comment 19

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 20

[Bas Schouten (:bas.schouten)]

Attachment: Use unified fallback for windows and all A8_UNORM surfaces v2

Updated to apply to m-c and address review comments.

Comment 21

[Bas Schouten (:bas.schouten)]

Pushed http://hg.mozilla.org/mozilla-central/rev/db3e8b1ecb26.