Last Comment Bug 809927 - Implement blend modes for Direct2D context
: Implement blend modes for Direct2D context
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla20
Assigned To: Rik Cabanier
:
Mentors:
Depends on: 836892
Blocks: 748433
  Show dependency treegraph
 
Reported: 2012-11-08 09:58 PST by Rik Cabanier
Modified: 2013-01-31 13:09 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
This is the shader program that implements blending (607.73 KB, patch)
2012-11-08 10:50 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
This patch enables support for blending in the D2D backend (616.61 KB, patch)
2012-11-08 13:59 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Addressed Bas' comments (503.18 KB, patch)
2012-11-16 15:15 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Fixed spacing issues. Added effect routine. (582.74 KB, patch)
2012-11-17 22:00 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
more whitespace fixes (582.69 KB, patch)
2012-11-18 09:02 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Fixed more whitespace issues (583.21 KB, patch)
2012-11-18 11:29 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
more whitespace fixes (583.19 KB, patch)
2012-11-18 11:36 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
removed ';' (582.28 KB, patch)
2012-11-18 11:41 PST, Rik Cabanier
bas: review+
Details | Diff | Splinter Review
Added last (?) whitespace issues (583.54 KB, patch)
2012-11-18 14:08 PST, Rik Cabanier
cabanier: review+
Details | Diff | Splinter Review
Fixed issue with premultiplied alpha (616.60 KB, patch)
2012-11-20 12:08 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Adds blending to D2D backend (587.36 KB, patch)
2012-11-20 12:37 PST, Rik Cabanier
cabanier: review+
Details | Diff | Splinter Review
Added blending to D2D backend (587.36 KB, patch)
2012-11-20 18:38 PST, Rik Cabanier
cabanier: review+
Details | Diff | Splinter Review
Adds blending to D2D backend (587.79 KB, patch)
2012-11-20 18:42 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Adds blending to D2D backend (584.00 KB, patch)
2012-11-21 16:51 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Adds blending to D2D backend (581.10 KB, patch)
2012-11-22 20:57 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Adds blending to D2D backend (581.10 KB, patch)
2012-11-22 21:25 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Adds blending to D2D backend (581.13 KB, patch)
2012-11-22 21:29 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Adds blending to D2D backend (220.39 KB, patch)
2012-11-23 14:29 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Debug patch for GPU failures (222.55 KB, patch)
2012-11-30 13:41 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
More debugging for DirectX isue (222.62 KB, patch)
2012-12-01 16:23 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Simplified shader to see if the problem still happens (490.62 KB, patch)
2012-12-03 14:50 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Adds blending to D2D backend (248.72 KB, patch)
2012-12-04 11:57 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Adds blending to D2D backend (286.78 KB, patch)
2012-12-06 11:04 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Adds blending to D2D backend (644.48 KB, patch)
2012-12-09 08:52 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Adds blending to D2D backend (644.48 KB, patch)
2012-12-09 12:06 PST, Rik Cabanier
bas: review+
Details | Diff | Splinter Review
Adds blending to D2D backend (644.50 KB, patch)
2012-12-10 14:47 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Adds blending to D2D backend (632.82 KB, patch)
2012-12-12 09:59 PST, Rik Cabanier
cabanier: checkin+
Details | Diff | Splinter Review

Description Rik Cabanier 2012-11-08 09:58:41 PST
bug 748433 implements Canvas blending for Core Graphics and Cairo.
This patch implements it for Direct2D as well
Comment 1 Rik Cabanier 2012-11-08 10:50:12 PST
Created attachment 679741 [details] [diff] [review]
This is the shader program that implements blending
Comment 2 Rik Cabanier 2012-11-08 13:59:04 PST
Created attachment 679823 [details] [diff] [review]
This patch enables support for blending in the D2D backend
Comment 3 Bas Schouten (:bas.schouten) 2012-11-16 13:16:54 PST
Comment on attachment 679823 [details] [diff] [review]
This patch enables support for blending in the D2D backend

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

In general there's a lot of stuff in here that is not adhering to our coding style guide, see https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style.

One thing that isn't in there but I also noticed is that operators should usually have spaces around there in our code, i.e. a > b and not a>b.

::: gfx/2d/DrawTargetD2D.cpp
@@ +1588,5 @@
> +    desc.BindFlags = D3D10_BIND_RENDER_TARGET | D3D10_BIND_SHADER_RESOURCE;
> +	
> +    HRESULT hr = this->mDevice->CreateTexture2D(&desc, nullptr, byRef(tmpTexture));
> +    if (FAILED(hr)) {
> +      gfxWarning() << "Failed to create temporary texture to hold surface data.";

You don't return here and so this will still use an invalid texture.

::: gfx/2d/ShadersD2D.fx
@@ +181,5 @@
> +}
> +
> +float Lum(float3 C)
> +{
> +	return 0.3 * C.r + 0.59 * C.g + 0.11 * C.b;

Please make these numbers static constants.

@@ +211,5 @@
> +{
> +	return max(C.r, max(C.g, C.b)) - min(C.r, min(C.g, C.b));
> +}
> +
> +void setsatcomponents(inout float minComp, inout float midComp, inout float maxComp, in float satVal)

nit: CamelCase like other functions

@@ +218,5 @@
> +	maxComp -= minComp;
> +	minComp = 0.0;
> +	if (maxComp > 0.0)
> +	{
> +		// max(..., 0.0000001) is an fx 1300 workaround

Please explain a little more about the why and how of this workaround, or add a reference to some bug/article.

@@ +266,1 @@
>  float4 SampleTexturePS( VS_OUTPUT In) : SV_Target

Please make this into a -separate- technique so we don't have conditional branches or complex branchless shaders in our standard rendering paths. Also, will this compile on D3D10 9Level3?
Comment 4 Rik Cabanier 2012-11-16 15:15:05 PST
Created attachment 682651 [details] [diff] [review]
Addressed Bas' comments
Comment 5 Rik Cabanier 2012-11-16 15:18:01 PST
(In reply to Bas Schouten (:bas.schouten) from comment #3)
> Comment on attachment 679823 [details] [diff] [review]
> This patch enables support for blending in the D2D backend
> 
> Review of attachment 679823 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general there's a lot of stuff in here that is not adhering to our coding
> style guide, see
> https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style.
> 
> One thing that isn't in there but I also noticed is that operators should
> usually have spaces around there in our code, i.e. a > b and not a>b.

I fixed the shader program so it adheres to this. (The cpp seemed fine)

> 
> ::: gfx/2d/DrawTargetD2D.cpp
> @@ +1588,5 @@
> > +    desc.BindFlags = D3D10_BIND_RENDER_TARGET | D3D10_BIND_SHADER_RESOURCE;
> > +	
> > +    HRESULT hr = this->mDevice->CreateTexture2D(&desc, nullptr, byRef(tmpTexture));
> > +    if (FAILED(hr)) {
> > +      gfxWarning() << "Failed to create temporary texture to hold surface data.";
> 
> You don't return here and so this will still use an invalid texture.

Fixed

> 
> ::: gfx/2d/ShadersD2D.fx
> @@ +181,5 @@
> > +}
> > +
> > +float Lum(float3 C)
> > +{
> > +	return 0.3 * C.r + 0.59 * C.g + 0.11 * C.b;
> 
> Please make these numbers static constants.

fixed

> 
> @@ +211,5 @@
> > +{
> > +	return max(C.r, max(C.g, C.b)) - min(C.r, min(C.g, C.b));
> > +}
> > +
> > +void setsatcomponents(inout float minComp, inout float midComp, inout float maxComp, in float satVal)
> 
> nit: CamelCase like other functions

Fixed

> 
> @@ +218,5 @@
> > +	maxComp -= minComp;
> > +	minComp = 0.0;
> > +	if (maxComp > 0.0)
> > +	{
> > +		// max(..., 0.0000001) is an fx 1300 workaround
> 
> Please explain a little more about the why and how of this workaround, or
> add a reference to some bug/article.
> 
> @@ +266,1 @@
> >  float4 SampleTexturePS( VS_OUTPUT In) : SV_Target
> 
> Please make this into a -separate- technique so we don't have conditional
> branches or complex branchless shaders in our standard rendering paths.

Fixed

> Also, will this compile on D3D10 9Level3?

I believe so. How can I verify this?
Comment 6 Rik Cabanier 2012-11-16 15:19:13 PST
> // max(..., 0.0000001) is an fx 1300 workaround

I removed this code. I think it's obsolete.
Comment 7 Bas Schouten (:bas.schouten) 2012-11-17 08:52:06 PST
Comment on attachment 682651 [details] [diff] [review]
Addressed Bas' comments

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

::: gfx/2d/DrawTargetD2D.cpp
@@ +1562,2 @@
>  
>    D3D10_VIEWPORT viewport;

Indentation of the new code is not as per file modeline. 2-space indent, no tabs.

@@ +1566,5 @@
>    viewport.Height = mSize.height;
>    viewport.Width = mSize.width;
>    viewport.TopLeftX = 0;
>    viewport.TopLeftY = 0;
> +  

nit: whitespace

@@ +1577,5 @@
>        SetFloatVector(ShaderConstantRectD3D10(0, 0, 1.0f, 1.0f));
>      mPrivateData->mEffect->GetVariableByName("tex")->AsShaderResource()->SetResource(mSRView);
> +
> +	// Handle the case where we blend with the backdrop
> +	if(aOperator>OP_XOR) {

This operator still has no spaces.

@@ +1585,5 @@
> +
> +		CD3D10_TEXTURE2D_DESC desc(DXGIFormat(format), size.width, size.height,
> +								   1, 1);
> +		desc.BindFlags = D3D10_BIND_RENDER_TARGET | D3D10_BIND_SHADER_RESOURCE;
> +	

nit: whitespace

@@ +1605,5 @@
> +			gfxWarning() << *this << "Failed to create shader resource view for temp texture. Code: " << hr;
> +			return;
> +		}
> +
> +		unsigned int compop  = (unsigned int)aOperator-(unsigned int)OP_XOR;

still an operator without spaces.

::: gfx/2d/ShadersD2D.fx
@@ +57,5 @@
>      float4 alpha;
>  };
>  
>  Texture2D tex;
> +Texture2D bcktex;

This is all still not done in a separate effect.
Comment 8 Rik Cabanier 2012-11-17 21:58:15 PST
(In reply to Bas Schouten (:bas.schouten) from comment #7)
> Comment on attachment 682651 [details] [diff] [review]
> Addressed Bas' comments
> 
> Review of attachment 682651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetD2D.cpp
> @@ +1562,2 @@
> >  
> >    D3D10_VIEWPORT viewport;
> 
> Indentation of the new code is not as per file modeline. 2-space indent, no
> tabs.
> 
> @@ +1566,5 @@
> >    viewport.Height = mSize.height;
> >    viewport.Width = mSize.width;
> >    viewport.TopLeftX = 0;
> >    viewport.TopLeftY = 0;
> > +  
> 
> nit: whitespace
> 
> @@ +1577,5 @@
> >        SetFloatVector(ShaderConstantRectD3D10(0, 0, 1.0f, 1.0f));
> >      mPrivateData->mEffect->GetVariableByName("tex")->AsShaderResource()->SetResource(mSRView);
> > +
> > +	// Handle the case where we blend with the backdrop
> > +	if(aOperator>OP_XOR) {
> 
> This operator still has no spaces.
> 
> @@ +1585,5 @@
> > +
> > +		CD3D10_TEXTURE2D_DESC desc(DXGIFormat(format), size.width, size.height,
> > +								   1, 1);
> > +		desc.BindFlags = D3D10_BIND_RENDER_TARGET | D3D10_BIND_SHADER_RESOURCE;
> > +	
> 
> nit: whitespace
> 
> @@ +1605,5 @@
> > +			gfxWarning() << *this << "Failed to create shader resource view for temp texture. Code: " << hr;
> > +			return;
> > +		}
> > +
> > +		unsigned int compop  = (unsigned int)aOperator-(unsigned int)OP_XOR;
> 
> still an operator without spaces.

I fixed all the whitespace and operator spacing issues

> 
> ::: gfx/2d/ShadersD2D.fx
> @@ +57,5 @@
> >      float4 alpha;
> >  };
> >  
> >  Texture2D tex;
> > +Texture2D bcktex;
> 
> This is all still not done in a separate effect.

strange! the new effect was wiped out for some reason. I re-added it.
Comment 9 Rik Cabanier 2012-11-17 22:00:58 PST
Created attachment 682843 [details] [diff] [review]
Fixed spacing issues. Added effect routine.
Comment 10 Bas Schouten (:bas.schouten) 2012-11-18 08:26:52 PST
Comment on attachment 682843 [details] [diff] [review]
Fixed spacing issues. Added effect routine.

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

Looks like we're almost there!

::: gfx/2d/DrawTargetD2D.cpp
@@ +1579,5 @@
> +
> +  // Handle the case where we blend with the backdrop
> +  if(aOperator > OP_XOR) {
> +    RefPtr<ID3D10Texture2D> tmpTexture;
> +    IntSize size = this->mSize;

Please remove this-> everywhere, sorry for not spotting this earlier.

@@ +1589,5 @@
> +    HRESULT hr = this->mDevice->CreateTexture2D(&desc, nullptr, byRef(tmpTexture));
> +    if (FAILED(hr)) {
> +      gfxWarning() << "Failed to create temporary texture to hold surface data.";
> +      return;
> +    }

This could use an extra new line

@@ +1604,5 @@
> +      gfxWarning() << *this << "Failed to create shader resource view for temp texture. Code: " << hr;
> +      return;
> +    }
> +
> +    unsigned int compop  = (unsigned int)aOperator - (unsigned int)OP_XOR;

nit: space before =

@@ +1611,5 @@
> +
> +    mPrivateData->mEffect->GetTechniqueByName("SampleBlendTexture")->GetPassByIndex(0)->Apply(0);
> +    }
> +  else
> +     mPrivateData->mEffect->GetTechniqueByName("SampleTexture")->GetPassByIndex(0)->Apply(0);

nit: space

::: gfx/2d/ShadersD2D.fx
@@ +53,2 @@
>  
>  struct PS_TEXT_OUTPUT

Please fix all the spacing issues in this file too! :)

There's some more trailing whitespace I didn't mark too, the review view should point it out quite obviously.

@@ +196,5 @@
> +	float x = max(max(C.r, C.g), C.b);
> +
> +	if(n < 0)
> +		C = L + (((C - L) * L) / (L - n));
> +                      

nit: whitespace

@@ +199,5 @@
> +		C = L + (((C - L) * L) / (L - n));
> +                      
> +	if(x > 1)
> +		C = L + ((C - L) * (1 - L) / (x - L));
> +        

nit: whitespace

@@ +565,5 @@
>          SetPixelShader(CompileShader(ps_4_0_level_9_3, SampleTexturePS()));
>      }
>  }
>  
> +technique10 SampleBlendTexture

Maybe this could use a more descriptive name, I'm not quite sure what it would be though, sadly.
Comment 11 Rik Cabanier 2012-11-18 09:02:32 PST
Created attachment 682898 [details] [diff] [review]
more whitespace fixes
Comment 12 Rik Cabanier 2012-11-18 11:29:42 PST
Created attachment 682910 [details] [diff] [review]
Fixed more whitespace issues
Comment 13 Rik Cabanier 2012-11-18 11:31:12 PST
(In reply to Bas Schouten (:bas.schouten) from comment #10)
> Comment on attachment 682843 [details] [diff] [review]
> Fixed spacing issues. Added effect routine.
> 
> Review of attachment 682843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like we're almost there!
> 
> ::: gfx/2d/DrawTargetD2D.cpp
> @@ +1579,5 @@
> > +
> > +  // Handle the case where we blend with the backdrop
> > +  if(aOperator > OP_XOR) {
> > +    RefPtr<ID3D10Texture2D> tmpTexture;
> > +    IntSize size = this->mSize;
> 
> Please remove this-> everywhere, sorry for not spotting this earlier.

Done. I wonder why I did this...

> 
> @@ +1589,5 @@
> > +    HRESULT hr = this->mDevice->CreateTexture2D(&desc, nullptr, byRef(tmpTexture));
> > +    if (FAILED(hr)) {
> > +      gfxWarning() << "Failed to create temporary texture to hold surface data.";
> > +      return;
> > +    }
> 
> This could use an extra new line

Done

> 
> @@ +1604,5 @@
> > +      gfxWarning() << *this << "Failed to create shader resource view for temp texture. Code: " << hr;
> > +      return;
> > +    }
> > +
> > +    unsigned int compop  = (unsigned int)aOperator - (unsigned int)OP_XOR;
> 
> nit: space before =

Done

> 
> @@ +1611,5 @@
> > +
> > +    mPrivateData->mEffect->GetTechniqueByName("SampleBlendTexture")->GetPassByIndex(0)->Apply(0);
> > +    }
> > +  else
> > +     mPrivateData->mEffect->GetTechniqueByName("SampleTexture")->GetPassByIndex(0)->Apply(0);
> 
> nit: space

Done

> 
> ::: gfx/2d/ShadersD2D.fx
> @@ +53,2 @@
> >  
> >  struct PS_TEXT_OUTPUT
> 
> Please fix all the spacing issues in this file too! :)

Done

> 
> There's some more trailing whitespace I didn't mark too, the review view
> should point it out quite obviously.
> 
> @@ +196,5 @@
> > +	float x = max(max(C.r, C.g), C.b);
> > +
> > +	if(n < 0)
> > +		C = L + (((C - L) * L) / (L - n));
> > +                      
> 
> nit: whitespace
> 
> @@ +199,5 @@
> > +		C = L + (((C - L) * L) / (L - n));
> > +                      
> > +	if(x > 1)
> > +		C = L + ((C - L) * (1 - L) / (x - L));
> > +        
> 
> nit: whitespace
> 
> @@ +565,5 @@
> >          SetPixelShader(CompileShader(ps_4_0_level_9_3, SampleTexturePS()));
> >      }
> >  }
> >  
> > +technique10 SampleBlendTexture
> 
> Maybe this could use a more descriptive name, I'm not quite sure what it
> would be though, sadly.

I gave it a longer name.
Let me know what you think
Comment 14 Rik Cabanier 2012-11-18 11:36:37 PST
Created attachment 682912 [details] [diff] [review]
more whitespace fixes
Comment 15 Rik Cabanier 2012-11-18 11:41:31 PST
Created attachment 682913 [details] [diff] [review]
removed ';'
Comment 16 Bas Schouten (:bas.schouten) 2012-11-18 13:44:09 PST
Comment on attachment 682913 [details] [diff] [review]
removed ';'

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

Alright, so this basically looks good with a couple of comments.

Please note when we land this along with the canvas work we'll need to have tests for all the blend modes as well!

::: gfx/2d/DrawTargetD2D.cpp
@@ +1576,5 @@
>      mPrivateData->mEffect->GetVariableByName("TexCoords")->AsVector()->
>        SetFloatVector(ShaderConstantRectD3D10(0, 0, 1.0f, 1.0f));
>      mPrivateData->mEffect->GetVariableByName("tex")->AsShaderResource()->SetResource(mSRView);
> +
> +  // Handle the case where we blend with the backdrop

Seems to be like this whole block should be indented 2 spaces more.

@@ +1582,5 @@
> +    RefPtr<ID3D10Texture2D> tmpTexture;
> +    IntSize size = mSize;
> +    SurfaceFormat format = mFormat;
> +
> +	CD3D10_TEXTURE2D_DESC desc(DXGIFormat(format), size.width, size.height, 1, 1);

nit: missed a whitespace fix

@@ +1611,5 @@
> +    mPrivateData->mEffect->GetVariableByName("blendop")->AsScalar()->SetInt(compop);
> +
> +    mPrivateData->mEffect->GetTechniqueByName("SampleTextureForBlending")->GetPassByIndex(0)->Apply(0);
> +    }
> +  else

single line clauses still get { }

::: gfx/2d/ShadersD2D.fx
@@ +330,5 @@
> +        retval.b = 1 - min(1, (1 -  background.b) / output.b);
> +      else
> +        retval.b = 0;
> +      break;
> +      }

nit: please make indentation on these case closing { consistent
Comment 17 Rik Cabanier 2012-11-18 14:08:35 PST
Created attachment 682933 [details] [diff] [review]
Added last (?) whitespace issues
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-11-19 18:57:07 PST
Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=ee59dfe33091
Comment 19 Rik Cabanier 2012-11-20 12:08:24 PST
Created attachment 683707 [details] [diff] [review]
Fixed issue with premultiplied alpha
Comment 20 Rik Cabanier 2012-11-20 12:37:14 PST
Created attachment 683718 [details] [diff] [review]
Adds blending to D2D backend
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-11-20 14:55:50 PST
Does this need to be reviewed again?
Comment 22 Rik Cabanier 2012-11-20 18:38:39 PST
Created attachment 683841 [details] [diff] [review]
Added blending to D2D backend
Comment 23 Rik Cabanier 2012-11-20 18:42:06 PST
Created attachment 683843 [details] [diff] [review]
Adds blending to D2D backend
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-11-20 19:09:40 PST
https://tbpl.mozilla.org/?tree=Try&rev=b3fb0a2665ab
Comment 25 Bas Schouten (:bas.schouten) 2012-11-21 10:32:41 PST
Comment on attachment 683843 [details] [diff] [review]
Adds blending to D2D backend

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

::: gfx/2d/DrawTargetD2D.cpp
@@ +1593,5 @@
> +      HRESULT hr = mDevice->CreateTexture2D(&desc, nullptr, byRef(tmpTexture));
> +      if (FAILED(hr)) {
> +        gfxWarning() << "Failed to create temporary texture to hold surface data.";
> +        return;
> +      }

You could try calling DrawTargetD2D::Flush() here to see if it solves your problems.

@@ +1598,5 @@
> +
> +      mDevice->CopyResource(tmpTexture, mTexture);
> +      if (FAILED(hr)) {
> +          gfxWarning() << *this << "Failed to create shader resource view for temp texture. Code: " << hr;
> +      return;

nit: indentation

@@ +1614,5 @@
> +      mPrivateData->mEffect->GetVariableByName("bcktex")->AsShaderResource()->SetResource(mBckSRView);
> +      mPrivateData->mEffect->GetVariableByName("blendop")->AsScalar()->SetInt(compop);
> +
> +      mPrivateData->mEffect->GetTechniqueByName("SampleTextureForBlending")->GetPassByIndex(0)->Apply(0);
> +      }

nit: else goes on same line

@@ +1617,5 @@
> +      mPrivateData->mEffect->GetTechniqueByName("SampleTextureForBlending")->GetPassByIndex(0)->Apply(0);
> +      }
> +    else {
> +       mPrivateData->mEffect->GetTechniqueByName("SampleTexture")->GetPassByIndex(0)->Apply(0);
> +	}

nit: indentation
Comment 26 Rik Cabanier 2012-11-21 16:51:54 PST
Created attachment 684235 [details] [diff] [review]
Adds blending to D2D backend
Comment 27 Rik Cabanier 2012-11-21 16:52:36 PST
(In reply to Bas Schouten (:bas.schouten) from comment #25)
> Comment on attachment 683843 [details] [diff] [review]
> Adds blending to D2D backend
> 
> Review of attachment 683843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetD2D.cpp
> @@ +1593,5 @@
> > +      HRESULT hr = mDevice->CreateTexture2D(&desc, nullptr, byRef(tmpTexture));
> > +      if (FAILED(hr)) {
> > +        gfxWarning() << "Failed to create temporary texture to hold surface data.";
> > +        return;
> > +      }
> 
> You could try calling DrawTargetD2D::Flush() here to see if it solves your
> problems.

Done

> 
> @@ +1598,5 @@
> > +
> > +      mDevice->CopyResource(tmpTexture, mTexture);
> > +      if (FAILED(hr)) {
> > +          gfxWarning() << *this << "Failed to create shader resource view for temp texture. Code: " << hr;
> > +      return;
> 
> nit: indentation

Done

> 
> @@ +1614,5 @@
> > +      mPrivateData->mEffect->GetVariableByName("bcktex")->AsShaderResource()->SetResource(mBckSRView);
> > +      mPrivateData->mEffect->GetVariableByName("blendop")->AsScalar()->SetInt(compop);
> > +
> > +      mPrivateData->mEffect->GetTechniqueByName("SampleTextureForBlending")->GetPassByIndex(0)->Apply(0);
> > +      }
> 
> nit: else goes on same line
> 
> @@ +1617,5 @@
> > +      mPrivateData->mEffect->GetTechniqueByName("SampleTextureForBlending")->GetPassByIndex(0)->Apply(0);
> > +      }
> > +    else {
> > +       mPrivateData->mEffect->GetTechniqueByName("SampleTexture")->GetPassByIndex(0)->Apply(0);
> > +	}
> 
> nit: indentation

Done
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-11-21 19:25:45 PST
New Try run: https://tbpl.mozilla.org/?tree=Try&rev=c50a7872fa9d
Comment 29 Rik Cabanier 2012-11-22 20:57:43 PST
Created attachment 684585 [details] [diff] [review]
Adds blending to D2D backend
Comment 30 Ryan VanderMeulen [:RyanVM] 2012-11-22 21:06:09 PST
https://tbpl.mozilla.org/?tree=Try&rev=7f6e86970acc
Comment 31 Rik Cabanier 2012-11-22 21:25:08 PST
Created attachment 684592 [details] [diff] [review]
Adds blending to D2D backend
Comment 32 Rik Cabanier 2012-11-22 21:29:36 PST
Created attachment 684593 [details] [diff] [review]
Adds blending to D2D backend
Comment 33 Ryan VanderMeulen [:RyanVM] 2012-11-23 05:25:10 PST
https://tbpl.mozilla.org/?tree=Try&rev=15cfd0ca95ec

(BTW, the last two revisions haven't applied cleanly to mozilla-central. Please rebase)
Comment 34 Bas Schouten (:bas.schouten) 2012-11-23 05:42:14 PST
Comment on attachment 684593 [details] [diff] [review]
Adds blending to D2D backend

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

::: gfx/2d/DrawTargetD2D.cpp
@@ +1490,5 @@
>  
>    PopAllClips();
>  
> +  if (aOperator > OP_XOR) {
> +    mRT->Flush();

If the other flush fixed it, this one probably isn't needed anymore.

@@ +1601,5 @@
> +        gfxWarning() << *this << "Failed to create shader resource view for temp texture. Code: " << hr;
> +        return;
> +      }
> +
> +	  DrawTargetD2D::Flush();

nit: Indentation, also, did this fix anything?

@@ +1616,5 @@
> +      mPrivateData->mEffect->GetVariableByName("bcktex")->AsShaderResource()->SetResource(mBckSRView);
> +      mPrivateData->mEffect->GetVariableByName("blendop")->AsScalar()->SetInt(compop);
> +
> +      mPrivateData->mEffect->GetTechniqueByName("SampleTextureForBlending")->GetPassByIndex(0)->Apply(0);
> +      }

nit: Indentation still off and else still on the wrong line.

@@ +1619,5 @@
> +      mPrivateData->mEffect->GetTechniqueByName("SampleTextureForBlending")->GetPassByIndex(0)->Apply(0);
> +      }
> +    else {
> +       mPrivateData->mEffect->GetTechniqueByName("SampleTexture")->GetPassByIndex(0)->Apply(0);
> +	}

nit: Indentation still off.

::: gfx/2d/Types.h
@@ +83,5 @@
>    FONT_STYLE_BOLD,
>    FONT_STYLE_BOLD_ITALIC
>  };
>  
> +enum CompositionOp { OP_OVER, OP_ADD, OP_ATOP, OP_OUT, OP_IN, OP_SOURCE, OP_DEST_IN, OP_DEST_OUT, OP_DEST_OVER, OP_DEST_ATOP, OP_XOR, OP_MULTIPLY, OP_SCREEN, OP_OVERLAY, OP_DARKEN, OP_LIGHTEN, OP_COLOR_DODGE, OP_COLOR_BURN, OP_HARD_LIGHT, OP_SOFT_LIGHT,  OP_DIFFERENCE, OP_EXCLUSION, OP_HUE, OP_SATURATION, OP_COLOR, OP_LUMINOSITY, OP_COUNT };

We should insert some newlines here ideally.
Comment 35 Rik Cabanier 2012-11-23 14:29:10 PST
Created attachment 684807 [details] [diff] [review]
Adds blending to D2D backend
Comment 36 Ryan VanderMeulen [:RyanVM] 2012-11-23 14:31:50 PST
This patch just got 360k smaller. Is that expected?
Comment 37 Ryan VanderMeulen [:RyanVM] 2012-11-23 14:50:01 PST
https://tbpl.mozilla.org/?tree=Try&rev=245d38c91824
Comment 38 Ryan VanderMeulen [:RyanVM] 2012-11-23 14:53:35 PST
Now with the right tests being run...
https://tbpl.mozilla.org/?tree=Try&rev=4726504f45b4
Comment 39 Ryan VanderMeulen [:RyanVM] 2012-11-26 17:29:58 PST
What info do you need? Comment 38 was from the last patch posted here.
Comment 40 Ryan VanderMeulen [:RyanVM] 2012-11-28 14:20:36 PST
More runs triggered (I'm assuming that's what you wanted - in the future, a comment accompanying the needinfo request would be handy :)...)
Comment 41 Rik Cabanier 2012-11-28 14:57:50 PST
(In reply to Ryan VanderMeulen from comment #40)
> More runs triggered (I'm assuming that's what you wanted - in the future, a
> comment accompanying the needinfo request would be handy :)...)

Will do so!
Where can I find the runs? They aren't popping up on https://tbpl.mozilla.org/?tree=Try
Comment 42 Ryan VanderMeulen [:RyanVM] 2012-11-28 15:13:36 PST
Same link as before.
Comment 43 Rik Cabanier 2012-11-30 13:41:29 PST
Created attachment 687269 [details] [diff] [review]
Debug patch for GPU failures
Comment 44 Rik Cabanier 2012-11-30 13:42:43 PST
Added debug statements to determine failures in GPU code.
Please rebuild and test win7 debug and optimized
Comment 45 Ryan VanderMeulen [:RyanVM] 2012-11-30 15:43:50 PST
https://tbpl.mozilla.org/?tree=Try&rev=7809598e392b
Comment 46 Rik Cabanier 2012-12-01 16:23:03 PST
Created attachment 687471 [details] [diff] [review]
More debugging for DirectX isue
Comment 47 Rik Cabanier 2012-12-01 16:24:40 PST
(In reply to Ryan VanderMeulen from comment #45)
> https://tbpl.mozilla.org/?tree=Try&rev=7809598e392b

Can you rebuild +test once more (on windows)?
I added some more output to track the problem
Comment 48 Ryan VanderMeulen [:RyanVM] 2012-12-01 18:07:46 PST
https://tbpl.mozilla.org/?tree=Try&rev=83b9bf20d434
Comment 49 Rik Cabanier 2012-12-03 09:57:49 PST
I sent an email to our NVidia rep. This is what he had to say:

It maybe worthwhile to try breaking up the shader into multiple stages.
Having larger ubershaders with switch statements means potentially that there is increased register requirements, potentially meaning that fewer threads can be launched at the same time, resulting in more CTA's being swapped in, meaning the shader will take longer.

It's worth a quick try, if it does not work, then we will probably need to investigate in more detail specifically with the shader being used.  Also the driver being used is quite old, have you tried a newer driver (version R310) is the latest that we have available on the NVIDIA website.
Comment 50 Rik Cabanier 2012-12-03 14:50:37 PST
Created attachment 687981 [details] [diff] [review]
Simplified shader to see if the problem still happens
Comment 51 Rik Cabanier 2012-12-03 14:53:42 PST
(In reply to Ryan VanderMeulen from comment #48)
> https://tbpl.mozilla.org/?tree=Try&rev=83b9bf20d434

Per NVidia's suggestion, I simplified the shader. (by simply removing the case, this is not a valid patch as the results will be wrong!)
Can you run another windows test?
Comment 52 Ryan VanderMeulen [:RyanVM] 2012-12-03 17:11:03 PST
https://tbpl.mozilla.org/?tree=Try&rev=fd4a5a813d4e
Comment 53 Rik Cabanier 2012-12-04 11:57:49 PST
Created attachment 688392 [details] [diff] [review]
Adds blending to D2D backend
Comment 54 Rik Cabanier 2012-12-04 11:59:16 PST
(In reply to Ryan VanderMeulen from comment #52)
> https://tbpl.mozilla.org/?tree=Try&rev=fd4a5a813d4e

Thanks! I didn't see any crashes so simplifying might fix the issue.
Can you rebuild with the latest patch that contains a shader that is simpler? Please  also include several test runs.
Comment 55 Ryan VanderMeulen [:RyanVM] 2012-12-04 15:46:22 PST
https://tbpl.mozilla.org/?tree=Try&rev=b19932b82aed
Comment 56 Rik Cabanier 2012-12-06 11:04:56 PST
Created attachment 689300 [details] [diff] [review]
Adds blending to D2D backend
Comment 57 Rik Cabanier 2012-12-06 11:06:13 PST
(In reply to Ryan VanderMeulen from comment #55)
> https://tbpl.mozilla.org/?tree=Try&rev=b19932b82aed

I reworked the patch yet again.
This version is not crashing on my local mac mini test server.

Can you rebuild (and retest multiple times) once again?
Comment 58 Rik Cabanier 2012-12-09 08:52:16 PST
Created attachment 690200 [details] [diff] [review]
Adds blending to D2D backend
Comment 59 Rik Cabanier 2012-12-09 12:06:07 PST
Created attachment 690209 [details] [diff] [review]
Adds blending to D2D backend
Comment 60 Rik Cabanier 2012-12-09 15:24:36 PST
successful try run: https://tbpl.mozilla.org/?tree=Try&rev=0347f8e7512f
Please review latest patch
Comment 61 Bas Schouten (:bas.schouten) 2012-12-10 14:37:59 PST
Comment on attachment 690209 [details] [diff] [review]
Adds blending to D2D backend

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

Some small changes then we should be able to Check-in :)

::: gfx/2d/DrawTargetD2D.cpp
@@ +1647,5 @@
> +      mPrivateData->mEffect->GetVariableByName("bcktex")->AsShaderResource()->SetResource(mBckSRView);
> +      mPrivateData->mEffect->GetVariableByName("blendop")->AsScalar()->SetInt(compop);
> +
> +      if (aOperator > OP_EXCLUSION)
> +        mPrivateData->mEffect->GetTechniqueByName("SampleTextureForNonSeparableBlending")->GetPassByIndex(0)->Apply(0);

nit: Put a newline in here somewhere.

::: gfx/2d/ShadersD2D.fx
@@ +274,5 @@
> +	return output;
> +
> +  output.rgb /= output.a;
> +  background.rgb /= background.a;
> + 

nit: stray whitespace

@@ +326,5 @@
> +	return output;
> +
> +  output.rgb /= output.a;
> +  background.rgb /= background.a;
> + 

nit: whitespace

@@ +403,5 @@
> +	return output;
> +
> +  output.rgb /= output.a;
> +  background.rgb /= background.a;
> + 

nit: see above :)
Comment 62 Rik Cabanier 2012-12-10 14:47:14 PST
Created attachment 690583 [details] [diff] [review]
Adds blending to D2D backend
Comment 63 Rik Cabanier 2012-12-10 15:31:18 PST
(In reply to Bas Schouten (:bas.schouten) from comment #61)
> Comment on attachment 690209 [details] [diff] [review]
> Adds blending to D2D backend
> 
> Review of attachment 690209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some small changes then we should be able to Check-in :)
> 
> ::: gfx/2d/DrawTargetD2D.cpp
> @@ +1647,5 @@
> > +      mPrivateData->mEffect->GetVariableByName("bcktex")->AsShaderResource()->SetResource(mBckSRView);
> > +      mPrivateData->mEffect->GetVariableByName("blendop")->AsScalar()->SetInt(compop);
> > +
> > +      if (aOperator > OP_EXCLUSION)
> > +        mPrivateData->mEffect->GetTechniqueByName("SampleTextureForNonSeparableBlending")->GetPassByIndex(0)->Apply(0);
> 
> nit: Put a newline in here somewhere.

Fixed

> 
> ::: gfx/2d/ShadersD2D.fx
> @@ +274,5 @@
> > +	return output;
> > +
> > +  output.rgb /= output.a;
> > +  background.rgb /= background.a;
> > + 
> 
> nit: stray whitespace

Fixed

> 
> @@ +326,5 @@
> > +	return output;
> > +
> > +  output.rgb /= output.a;
> > +  background.rgb /= background.a;
> > + 
> 
> nit: whitespace

Fixed

> 
> @@ +403,5 @@
> > +	return output;
> > +
> > +  output.rgb /= output.a;
> > +  background.rgb /= background.a;
> > + 
> 
> nit: see above :)

Fixed
Comment 64 Rik Cabanier 2012-12-12 09:59:41 PST
Created attachment 691409 [details] [diff] [review]
Adds blending to D2D backend
Comment 65 Bas Schouten (:bas.schouten) 2012-12-12 12:30:01 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/1eedc9864f2d
Comment 66 Ryan VanderMeulen [:RyanVM] 2012-12-12 13:53:02 PST
Rik, in the future, adding checkin-needed to the keywords at the top is usually the best way to get things checked in in a timely fashion.
Comment 67 Ed Morley [:emorley] 2012-12-13 08:07:21 PST
https://hg.mozilla.org/mozilla-central/rev/1eedc9864f2d

Note You need to log in before you can comment on or make changes to this bug.