Thinking about https://wiki.mozilla.org/Gecko:TransparentLayersAndSubpixelAA and looking at some of the performance issues we've been hitting... I think with BasicLayers using Quartz, X and GDI, the first three points on the wiki page will be adequate, and they've landed. Some tweaking is still required. I'll assume that everyone (we care about) who has D2D will end up using D3D9 layers instead of BasicLayers. So let's focus on D3D9 here. I think we should go for component alpha. Making layers nonretained sucks. Losing subpixel AA sucks. We can do various workarounds but they all have limitations or lose on performance. D3D9 gives us access to shaders. We could use that to support component alpha for transparent ThebesLayers so they don't have to lose subpixel AA. We can do the old black-buffer/white-buffer trick: keep two retained buffers for the ThebesLayer, one on a black background and one on white. At compositing time, copy the current contents of the destination buffer into a separate copy-of-destination buffer, then use a shader to fetch from the black-buffer, the white-buffer, and the copy-of-destination buffer, do the math, and write the results back to the destination. That may be expensive, but not retaining is also expensive. Would that approach be ridiculously expensive/impossible? I presume that writing the shader is not hard, and the hard bit (if it is hard) is making a copy of part of the destination buffer without destroying performance.

(In reply to comment #0) > I presume that writing the shader is not hard, and the hard bit (if it is hard) > is making a copy of part of the destination buffer without destroying > performance. Yeah, that and keeping the additional memory usage under control. Dealing with gamma may also make this a bit trickier. D2D currently doesn't seem to handle this issue very well. For reference, here's a description of how cleartype blending is done: http://msdn.microsoft.com/en-us/library/ff561082%28v=VS.85%29.aspx It may also be possible remove the need for a copy of the destination surface when using D3D10 by using SRGB buffers and two color blending a. la ARB_blend_func_extended.

See http://msdn.microsoft.com/en-us/library/bb172508%28VS.85%29.aspx. This looks easy on Vista/7.

Mmm, ARB_blend_func_extended sounds like exactly what we want on GL too. I wonder how widely that is supported. So I have the following proposal: 1) Whenever D3D9Ex is available, make all transparent ThebesLayers render to retained on-white and on-black buffers, and composite them using an appropriate shader and D3DBLEND_SRCCOLOR2. 2) When D3D9Ex is not available, let transparent ThebesLayers be retained RGBA buffers. If there is text over transparent pixels, disable subpixel AA, otherwise render any text normally. My main concern is whether part 1 actually works. MSDN says "These new blending modes are only used for text rendering on the first render target." I'm not sure what that means. This may not be well tested in drivers, or it may be slow. Would be nice to have someone hack it up experimentally so we can see.

As for gamma, is it not enough to just composite in linear-RGB space, which I think we can get for free?

"ARB_blend_func_extended is called dual source blending in DX10, but got dropped in DX11." http://oscarbg.blogspot.com/2010/03/whats-left-in-opengl-40-and-more-raw.html Is that bad?

Another possible approach would be to use glColorMask or its D3D equivalent D3DRS_COLORWRITEENABLE to update one color channel at a time. That would take 3 or 4 passes though, not so good.

I have ARB_blend_func_extended on my nvidia 330M, which is a generation old. Pretty sure it's supported on AMD as well, though it doesn't exist on mobile (but we also don't care about it there, I don't think).

Yeah, on mobile I think it's fine to disable subpixel AA for transparent surfaces. We have limited shader bandwidth there anyway. Anyway, that's a different bug. Filed bug 593733 for GL component alpha.

From discussions with Bas on IRC, it sounds like we can persuade D2D to draw subpixel AA into transparent buffers when we know the text is over opaque pixels. So we should absolutely do that, and then we only need to use component alpha when there's text over transparent pixels.

BTW the shader would look like this: whitebuf = <read from white buffer> blackbuf = <read from black buffer> PSOutColor[0] = blackbuf; PSOutColor[1] = (1.0, 1.0, 1.0, 1.0) - whitebuf + blackbuf; So I guess it's pretty cheap as shaders go.

BTW, some motivation for this bug: In bug 593389 is an example of a page where we have blog text with a text-shadow over a background-attachment:fixed background. Repainting the whole page on every paint is going to be relatively slow no matter what we do (although we can and will make it faster, of course). But using a transparent layer and losing subpixel AA on the text is also not going to be acceptable to a large chunk of users --- see bug 593361 comment #26 and following. It seems to me that component alpha is the best way out of this dilemma, because even if it's expensive it shifts the load from the CPU (which we know is saturated for cases like bug 593389) to the GPU.

And BTW Bas discovered that D3DBLEND_SRCCOLOR2 isn't supported, so my suggestions here don't work, but he has a clever alternative approach that does work, which I'll let him explain :-).

Is there a D3D10 version of this bug?

Currently, cairo_tee_surface doesn't interact very well with cairo_push_group etc. cairo_push_group creates a similar surface, which is another tee surface so drawing to the pushed group draws into all the subsurfaces as you'd expect. But when we pop_group_to_source and paint, the inner tee surface's master surface is used as the source for painting into all the surfaces in the outer tee surface. I don't think this is really what you'd want in any usage scenario. It's particularly bad when you're teeing into two surfaces which aren't meant to have equivalent contents, which we will later in this patch series. This patch fixes the problem. When we composite a tee surface into another tee surface, pick a source subsurface that's at the same index as the destination subsurface, if there is one and it has the same backend type.

Later it'll be helpful for container layers to know whether they contain text over transparent pixels. We can just extend CONTENT_NO_TEXT_OVER_TRANSPARENT and CONTENT_NO_TEXT to be applicable to all types of layers.

We can use those flags and CanUseOpaqueSurface which already exists to create a new helper GetSurfaceMode() returning an enum telling us what kind of alpha a layer wants.

I was investigating why our chrome layer thinks it needs text over transparent pixels, and part of the problem was that we're treating all CSS gradients as transparent. This patch fixes that. This doesn't actually get us all the way to knowing our chrome's text is over opaque pixels, but the patch is trivial and I think worth having.

This lets FrameLayerBuilder set CONTENT_NO_TEXT_OVER_TRANSPARENT on containers --- if all their children have the flag. It's not optimal as noted in the comments, but it's cheap and it'll do for now.

When gfxContext::PushGroupAndCopyBackground copies up from the current target to the pushed group, and those surfaces are tee surfaces, we need to copy the subsurfaces pointwise. We definitely don't want to copy the contents of our "render onto black" surface to the temporary pushed surface for "render onto white"!

If our on-black/on-white tee surface is ever used as a source, we're pretty doomed for the same reason I mentioned in the previous patch. Add API so we can assert if that happens (in simple cases anyway).

This is the real thing. I had to refactor the existing code quite a bit. In particular I moved all the texture allocation and copying from SetVisibleRegion into RenderLayer. In the shaders we have to pick a value of the alpha channel in case we draw into a transparent surface. Picking the green channel was somewhat arbitrary but I can't think of a better approach.

We need to fix containers so that component alpha isn't mashed away when drawing into an intermediate surface. One way to do this would be to support component alpha for containers directly but that's a lot of work, potentially slow, and not really worth it. Instead we can use the old "copy the background into the container so we have opaque pixels in the container" trick. This will work great for Web content (since we know the container for the Web page is opaque), as long as the transform is a translation by pixels (if any resampling were to happen in the copy, the results would be terrible). So I restrict it to that case. This actually isn't a problem in practice because if we are doing any scaling or rotation of the temporary surface to the target, any "subpixel AA" we draw to the temporary surface is going to be completely wrong anyway. We may need a followup patch to disable Cleartype in such cases (I think this is probably already a problem on trunk).

With these patches, this testcase gets subpixel AA everywhere with D3D9+GDI. (As we do with the patches in bug 363861 for BasicLayers+GDI.) With these patches, our chrome ThebesLayer uses component alpha, because the "Firefox" button has rounded borders so its background is treated as not opaque, so the "Firefox" text is treated as possibly being over transparent pixels. On the plus side, these patches restore subpixel AA to the chrome (e.g., URL bar). On the minus side, that could be a performance hit since painting the chrome ThebesLayer will be more expensive --- we'll effectively have to paint a lot of it twice. I may need to investigate adding more optimizations so we can know that the chrome ThebesLayer does not need component alpha. I imagine this approach will port over to D3D10+D2D and OGL+Quartz fairly easily. Only parts 9 and 10 would need to be ported. I think we should wait until this D3D9 code is landed, to shake out any issues.

Comment on attachment 488416 [details] [diff] [review] Part 5: Mark CSS gradient images as opaque when all their stops are opaque r=dbaron, but it's probably worth adding an assertion to nsStyleGradient::IsOpaque that mStops.Length() >= 1, and adding a comment that we assume that the first and last stops always extend their colors off to infinity.

OK. The parser actually rejects gradients with less than two stops.

(In reply to comment #22) > In the shaders we have to pick a value of the alpha channel in case we draw > into a transparent surface. Picking the green channel was somewhat arbitrary > but I can't think of a better approach. Would the arithmetic mean ("(alphas.r + alphas.g + alphas.b) / 3.0" or "dot(alphas.rgb, thirds)" with float3 thirds = {1/3, 1/3, 1/3}) slow things down too much? Or are you choosing green because it's the 'center' color channel? [/uninformed comment]

Do these patches handle gamma correction at all?

(In reply to comment #27) > Would the arithmetic mean ("(alphas.r + alphas.g + alphas.b) / 3.0" or > "dot(alphas.rgb, thirds)" with float3 thirds = {1/3, 1/3, 1/3}) slow things > down too much? Or are you choosing green because it's the 'center' color > channel? [/uninformed comment] I mainly chose it because it's what cairo-win32-font.c currently does. I have no reason to believe it's better or worse than other possible choices. And it doesn't really matter because with these patches we hardly ever composite text over transparent pixels. (In reply to comment #28) > Do these patches handle gamma correction at all? Not specifically, but my understanding is that the shader calculations are done in linear color space, so things might just work out. I'm not really sure though.

(In reply to comment #29) > (In reply to comment #27) > > Would the arithmetic mean ("(alphas.r + alphas.g + alphas.b) / 3.0" or > > "dot(alphas.rgb, thirds)" with float3 thirds = {1/3, 1/3, 1/3}) slow things > > down too much? Or are you choosing green because it's the 'center' color > > channel? [/uninformed comment] > > I mainly chose it because it's what cairo-win32-font.c currently does. I have > no reason to believe it's better or worse than other possible choices. And it > doesn't really matter because with these patches we hardly ever composite text > over transparent pixels I doubt it will matter much, since there is 'some' performance impact I'd suggest we try this first and then try different solutions is there are problems.

I don't know why this isn't blocking already

Comment on attachment 488414 [details] [diff] [review] Part 3: Make Layer::CONTENT_NO_TEXT/CONTENT_NO_TEXT_OVER_TRANSPARENT applicable to all layer types Moved to part 3 of bug 612840

Comment on attachment 488417 [details] [diff] [review] Part 6: Set CONTENT_NO_TEXT/CONTENT_NO_TEXT_OVER_TRANSPARENT on container layers Moved to part 3 of bug 612840

Refreshed due to rebasing onto bug 612840. Also, adds SupportsComponentAlphaChildren() to ContainerLayerD3D9, which returns false if the container isn't a good target for component-alpha blending. A ThebesLayerD3D9 that wants component alpha but finds no support in its parent will rerender with grayscale AA. This actually changes behavior significantly from the previous patch for the browser chrome in Aero. The previous patch would go ahead and do component alpha blending for the chrome Thebeslayer, but with this patch, we will never use component alpha blending for the chrome ThebesLayer. Instead if there is text over the transparent part of the window, we'll make the whole layer grayscale AA. I will restore subpixel-AA to the browser chrome in bug 602757. Oh, also there was a bug in the SURFACE_SINGLE_CHANNEL_ALPHA --- we need to enable Cleartype for the transparent surface when we can.

Also refreshed. This version sets mSupportsComponentAlphaChildren appropriately. For the mUseIntermediateSurface case, we could use the Clear path instead of the StretchRect path in some cases where we know the children don't need component alpha. I thought probably we should avoid the extra complexity unless there's a significant performance difference.

(In reply to comment #13) > Is there a D3D10 version of this bug? I just filed bug 612846 for that.

I may not be able to review this stuff in the next couple of days, as I'm really busy with fennec stuff right now.

Updated to use gfxASurface::SetSubpixelAntialiasingEnabled from revised patches in bug 363861.

(In reply to comment #17) > Created attachment 488415 [details] [diff] [review] > Part 4: Create Layer::GetSurfaceMode to help determine what kind of alpha > support the layer needs > > We can use those flags and CanUseOpaqueSurface which already exists to create a > new helper GetSurfaceMode() returning an enum telling us what kind of alpha a > layer wants. This needs to use CONTENT_COMPONENT_ALPHA instead of CONTENT_NO_TEXT_OVER_TRANSPARENT if bug 612840 is a dependency.

Refreshed patches are here: http://hg.mozilla.org/users/rocallahan_mozilla.com/main/

Comment on attachment 488419 [details] [diff] [review] Part 8: Add SetAllowUseAsSource API for better diagnostics This seems like a good thing to have.

Comment on attachment 488413 [details] [diff] [review] Part 2: When compositing a tee surface into another tee surface, try to compose the subsurfaces pointwise I agree that this is probably the right thing to do but it feels sort of ugly. i.e. the handling of the situations where the number of subsurfaces don't match is not really that intuitive. >+static const cairo_pattern_t * >+_cairo_tee_surface_match_source (cairo_tee_surface_t *surface, >+ const cairo_pattern_t *source, >+ int index, >+ cairo_surface_wrapper_t *dest, >+ cairo_surface_pattern_t *temp) >+{ >+ cairo_surface_t *s; >+ cairo_status_t status = cairo_pattern_get_surface ((cairo_pattern_t *)source, &s); cairo_pattern_get_surface calls _cairo_error if source is not a surface pattern. This will cause false positives when trying to break on _cairo_error which is sort of unfortunate. I think the right thing to do is probably for cairo_pattern_get_surface to not call _cairo_error because _cairo_error is meant more for debugging error states that are sticky (like on cairo_t*). The easiest thing to do is just check for surface pattern before hand, but this issue isn't really a big deal. > static cairo_int_status_t > _cairo_tee_surface_paint (void *abstract_surface, > cairo_operator_t op, > const cairo_pattern_t *source, > cairo_clip_t *clip) > { > cairo_tee_surface_t *surface = abstract_surface; > cairo_surface_wrapper_t *slaves; > int n, num_slaves; > cairo_status_t status; >+ const cairo_pattern_t *matched_source; >+ cairo_surface_pattern_t temp; > >- status = _cairo_surface_wrapper_paint (&surface->master, op, source, clip); >+ matched_source = _cairo_tee_surface_match_source (surface, source, 0, &surface->master, &temp); >+ status = _cairo_surface_wrapper_paint (&surface->master, op, matched_source, clip); >+ if (matched_source == &temp.base) { >+ _cairo_pattern_fini (&temp.base); >+ } > if (unlikely (status)) > return status; > > num_slaves = _cairo_array_num_elements (&surface->slaves); > slaves = _cairo_array_index (&surface->slaves, 0); > for (n = 0; n < num_slaves; n++) { >- status = _cairo_surface_wrapper_paint (&slaves[n], op, source, clip); >+ matched_source = _cairo_tee_surface_match_source (surface, source, n + 1, &slaves[n], &temp); >+ status = _cairo_surface_wrapper_paint (&slaves[n], op, matched_source, clip); Looks like some of the indentation is off here. I may have seen other places too.

Comment on attachment 488418 [details] [diff] [review] Part 7: When doing a PushGroupAndCopyBackground on a gfxTeeSurface, copy the backgrounds of the subsurfaces pointwise Making CopySurface virtual might make this cleaner.

(In reply to comment #42) > Looks like some of the indentation is off here. I may have seen other places > too. It's just because some lines use spaces and other use tabs and the ones with spaces indent one character more when + or - is at the start of the line in the patch.

I split up part 9 into a few patches to help me track down a performance regression. It should also make things easier to review.

There was a bug in the previous patch where mTextureRegion was only set if mTexture already existed. This led to us painting a lot of content twice, unnecessarily: the first time through mTexture was null so we wouldn't set mTextureRegion, the next time through mTextureRegion was empty so we'd paint the content again. This led to a small Tp4 regression, which is fixed with this version of the patch.

Sorry, v4 was not correctly updated. v5 fixes the performance regression I mentioned.

Comment on attachment 500321 [details] [diff] [review] Part 9 v5 In general (but this can be done in a follow-up), I think if transitions from component alpha to opaque are common we could consider retaining opaque regions by retaining mTexture contents in those areas and tossing out mTextureOnWhite. I'm somewhat concerned about performance on low-end devices and devices with shared memory architectures, but let's see. Also, I'm guessing gfxWindowsNativeDrawing doesn't know about gfxTeeSurfaces meaning it will go the long way around and do its own alpha recovery. With some additional code we could optimize this way by letting it grab both the opaque destinations and drawing there twice. But maybe that isn't worth it if we rarely do native theme stuff to component alpha surfaces. >diff --git a/gfx/layers/d3d9/ThebesLayerD3D9.cpp b/gfx/layers/d3d9/ThebesLayerD3D9.cpp >--- a/gfx/layers/d3d9/ThebesLayerD3D9.cpp >+++ b/gfx/layers/d3d9/ThebesLayerD3D9.cpp > ThebesLayerD3D9::RenderLayer() >+ if (mTexture) { >+ if (!mTextureRegion.IsEqual(mVisibleRegion)) { I'd prefer to keep the next bit in a separate function, it keeps RenderLayer cleaner. > >- nsIntRegionRectIterator iter(mVisibleRegion); >+ for (PRInt32 pass = 1; pass <= 2; ++pass) { Let's #define 1 and 2 for maintainability. i.e. PASS_INVSRCALPHA PASS_SRCCOLOR or something like that. >+ if (mode == SURFACE_COMPONENT_ALPHA) { We should unbind mTextureOnWhite from the second texture sampler. We don't do this with stage 0 since we practically always override it with some other texture soon after anyway. This could potentially not be the case for stage 1. >+ // XXX Do we actually need this copy? Why can't we do the LockRect earlier >+ // and paint directly into here? We would only be able to create an ImageSurface and not a gfxWindowsSurface. For our native theme drawing this creates an undesirable situation. That's the original reasoning. If we believe that native theme drawing to ALPHA textures is much rarer than the case without native theme drawing, we could consider changing it.

Comment on attachment 491140 [details] [diff] [review] Part 10 v2 >diff --git a/gfx/layers/d3d9/ContainerLayerD3D9.cpp b/gfx/layers/d3d9/ContainerLayerD3D9.cpp >--- a/gfx/layers/d3d9/ContainerLayerD3D9.cpp >+++ b/gfx/layers/d3d9/ContainerLayerD3D9.cpp >@@ -129,39 +129,79 @@ LayerD3D9* > void > ContainerLayerD3D9::RenderLayer() > { >+ RECT dest = { 0, 0, visibleRect.width, visibleRect.height }; >+ RECT src = dest; >+ ::OffsetRect(&src, >+ visibleRect.x + PRInt32(transform.x0), >+ visibleRect.y + PRInt32(transform.y0)); >+ hr = device()-> >+ StretchRect(previousRenderTarget, &src, renderSurface, &dest, D3DTEXF_NONE); In the past we discovered a bug on relatively recent NVidia drivers with StretchRect between equal sized surfaces. NVidia fixed this in more recent drivers after I reported it, but we should make sure we don't hit this here.

Comment on attachment 500321 [details] [diff] [review] Part 9 v5 >+ case SURFACE_COMPONENT_ALPHA: { >+ nsRefPtr<gfxWindowsSurface> onBlack = opaqueRenderer.Begin(this); >+ nsRefPtr<gfxWindowsSurface> onWhite = opaqueRendererOnWhite.Begin(this); >+ FillSurface(onBlack, aRegion, bounds.TopLeft(), gfxRGBA(0.0, 0.0, 0.0, 1.0)); >+ FillSurface(onWhite, aRegion, bounds.TopLeft(), gfxRGBA(1.0, 1.0, 1.0, 1.0)); >+ gfxASurface* surfaces[2] = { onBlack.get(), onWhite.get() }; >+ destinationSurface = new gfxTeeSurface(surfaces, NS_ARRAY_LENGTH(surfaces)); >+ // Using this surface as a source will likely go horribly wrong, since >+ // only the onBlack surface will really be used, so alpha information will >+ // be incorrect. >+ destinationSurface->SetAllowUseAsSource(PR_FALSE); >+ break; >+ } > } Weird indentation style btw .. is this really the one we use? Personally I prefer either: switch () { case SURFACE_COMPONENT_ALPHA: { foo(); } }

(In reply to comment #50) > >diff --git a/gfx/layers/d3d9/ThebesLayerD3D9.cpp b/gfx/layers/d3d9/ThebesLayerD3D9.cpp > >--- a/gfx/layers/d3d9/ThebesLayerD3D9.cpp > >+++ b/gfx/layers/d3d9/ThebesLayerD3D9.cpp > > ThebesLayerD3D9::RenderLayer() > >+ if (mTexture) { > >+ if (!mTextureRegion.IsEqual(mVisibleRegion)) { > > I'd prefer to keep the next bit in a separate function, it keeps RenderLayer > cleaner. OK > >- nsIntRegionRectIterator iter(mVisibleRegion); > >+ for (PRInt32 pass = 1; pass <= 2; ++pass) { > > Let's #define 1 and 2 for maintainability. i.e. PASS_INVSRCALPHA PASS_SRCCOLOR > or something like that. That isn't right when the first pass is for a non-component-alpha layer. I'll try factoring out the guts of the loop into a helper function so we can just call it in a few different ways. > >+ if (mode == SURFACE_COMPONENT_ALPHA) { > > We should unbind mTextureOnWhite from the second texture sampler. We don't do > this with stage 0 since we practically always override it with some other > texture soon after anyway. This could potentially not be the case for stage 1. OK. > >+ // XXX Do we actually need this copy? Why can't we do the LockRect earlier > >+ // and paint directly into here? > > We would only be able to create an ImageSurface and not a gfxWindowsSurface. > For our native theme drawing this creates an undesirable situation. That's the > original reasoning. If we believe that native theme drawing to ALPHA textures > is much rarer than the case without native theme drawing, we could consider > changing it. OK (In reply to comment #51) > Comment on attachment 491140 [details] [diff] [review] > Part 10 v2 > > >diff --git a/gfx/layers/d3d9/ContainerLayerD3D9.cpp b/gfx/layers/d3d9/ContainerLayerD3D9.cpp > >--- a/gfx/layers/d3d9/ContainerLayerD3D9.cpp > >+++ b/gfx/layers/d3d9/ContainerLayerD3D9.cpp > >@@ -129,39 +129,79 @@ LayerD3D9* > > void > > ContainerLayerD3D9::RenderLayer() > > { > >+ RECT dest = { 0, 0, visibleRect.width, visibleRect.height }; > >+ RECT src = dest; > >+ ::OffsetRect(&src, > >+ visibleRect.x + PRInt32(transform.x0), > >+ visibleRect.y + PRInt32(transform.y0)); > >+ hr = device()-> > >+ StretchRect(previousRenderTarget, &src, renderSurface, &dest, D3DTEXF_NONE); > > In the past we discovered a bug on relatively recent NVidia drivers with > StretchRect between equal sized surfaces. NVidia fixed this in more recent > drivers after I reported it, but we should make sure we don't hit this here. I'm not sure what you mean. How can I avoid calling StretchRect on equal-size surfaces?

(In reply to comment #50) > We should unbind mTextureOnWhite from the second texture sampler. We don't do > this with stage 0 since we practically always override it with some other > texture soon after anyway. This could potentially not be the case for stage 1. You mean by calling device()->SetTexture(1, NULL)? We don't do this for PlanarYCbCrImageD3D9, which uses three samplers; should we?

What do you think of this?

Comment on attachment 500505 [details] [diff] [review] Part 9 v6 Looks good! Indentation on the curly braces in the cases still hurts my eyes, but I'm happy.

(In reply to comment #54) > (In reply to comment #50) > > We should unbind mTextureOnWhite from the second texture sampler. We don't do > > this with stage 0 since we practically always override it with some other > > texture soon after anyway. This could potentially not be the case for stage 1. > > You mean by calling device()->SetTexture(1, NULL)? > > We don't do this for PlanarYCbCrImageD3D9, which uses three samplers; should > we? Hrm, we probably should do that for stage 1 and 2. It's not terribly important, but a good idea.

http://hg.mozilla.org/mozilla-central/rev/ccba8826be14 http://hg.mozilla.org/mozilla-central/rev/bacc54d452a9 http://hg.mozilla.org/mozilla-central/rev/e427b4ea7e2f http://hg.mozilla.org/mozilla-central/rev/10ea906a2791 http://hg.mozilla.org/mozilla-central/rev/77cb8bb83b12 http://hg.mozilla.org/mozilla-central/rev/9fd3423cae84 http://hg.mozilla.org/mozilla-central/rev/27debc14ea67 http://hg.mozilla.org/mozilla-central/rev/431218b437ab http://hg.mozilla.org/mozilla-central/rev/02c4dac4ae7d http://hg.mozilla.org/mozilla-central/rev/b4e0f47b628d http://hg.mozilla.org/mozilla-central/rev/58aa602af75c

Roc, There is a problem with the changeset http://hg.mozilla.org/mozilla-central/rev/c20f34eefa5d. It causes the orbitz the webpage to load but displays only when you scroll, otherwise it looks like a blank page.

I see the same problem with that patch backed out.

(In reply to comment #60) > I see the same problem with that patch backed out. yhe following changeset causes this. c20f34eefa5d Robert O'Callahan — Bug 593604. Re-add missing hunk of part 10. a=blocking

OK, I backed that part out. http://hg.mozilla.org/mozilla-central/rev/d641b5c7774e That part actually disables component alpha in most cases, so this bug is mostly unfixed again.

I was wrong in comment #60. Dale, Alice, are you both using D3D9+Direct2D?

I think the problem is D2D combined with D3D9. I see the regression with D2D+D3D9, but not with GDI+D3D9. This patch fixes it for me. Before this patch, HaveTextures() with SURFACE_COMPONENT_ALPHA and D2D would always return false even after a successful CreateNewTextures, because mTextureOnWhite would never be created. This patch makes D2D and SURFACE_COMPONENT_ALPHA just use a single RGBA surface, i.e., no copmonent alpha. I think that's OK because D3D9+D2D is not something we really want to support well. Having said that, are Dale and Alice being assigned D3D9+D2D by default for some reason? We should make sure that D3D9+D2D is never the default.

This was already reviewed above.

(In reply to comment #64) > Having said that, are Dale and Alice being assigned D3D9+D2D by default for > some reason? We should make sure that D3D9+D2D is never the default. Reason: for test.

OK. I really appreciate all your testing. It would be useful if, when you report graphics bugs, you mention whether you're using a default (or at least supported) configuration or a non-default configuration. I think our supported configurations are: -- No accelerated layers, no Direct2D, no DirectWrite -- D3D9 accelerated layers, no Direct2D, no DirectWrite -- D3D10 accelerated layers, Direct2D, DirectWrite -- GL accelerated layers (Mac and mobile Linux only)

I'd have to confirm when I get home today if you still need this info, but I think I'm running both D2D/DW+D3D9, mostly for testing as well.

How about we make a patch that removes all the D2D code form D3D9. It would certainly make life easier and clean up the D3D9 code a bunch?

Yes, we should remove the ability for D2D to work with D3D9, as it's not a configuration we have any interest in supporting in the future.

Yes, I suggested this a while ago! :-)

Probably more relevant to other bugs (e.g. bug 605808) but would it make sense to refer to Direct3D 10 instead of Direct2D? D3D10 only works with D2D anyway, and D3D9 vs D2D gives the impression (to me at least) that the former would be newer/better.

sorry if wrong place, is below issue caused by this bug ? http://forums.mozillazine.org/viewtopic.php?p=10270865#p10270865

Could be. Either way, a new bug should be filed on that.

thanks, filed a bug. Bug 622733

This patch removes all the D3D9/D2D interop code. This makes the prefer-d3d9 mode slow, as expected. But it will still work. I think that's okay since it can still be used for some testing, and we should keep it around.

Blocks regression bugs.

Checked in last part: http://hg.mozilla.org/mozilla-central/rev/6bae7865092a