809927 - Implement blend modes for Direct2D context

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Rik Cabanier]

bug 748433 implements Canvas blending for Core Graphics and Cairo.
This patch implements it for Direct2D as well

Comment 1

[Rik Cabanier]

Attachment: This is the shader program that implements blending

Comment 2

[Rik Cabanier]

Attachment: This patch enables support for blending in the D2D backend

Comment 3

[Bas Schouten (:bas.schouten)]

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]

Attachment: Addressed Bas' comments

Comment 5

[Rik Cabanier]

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

> // max(..., 0.0000001) is an fx 1300 workaround

I removed this code. I think it's obsolete.

Comment 7

[Bas Schouten (:bas.schouten)]

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]

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

Attachment: Fixed spacing issues. Added effect routine.

Comment 10

[Bas Schouten (:bas.schouten)]

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]

Attachment: more whitespace fixes

Comment 12

[Rik Cabanier]

Attachment: Fixed more whitespace issues

Comment 13

[Rik Cabanier]

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

Attachment: more whitespace fixes

Comment 15

[Rik Cabanier]

Attachment: removed ';'

Comment 16

[Bas Schouten (:bas.schouten)]

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]

Attachment: Added last (?) whitespace issues

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=ee59dfe33091

Comment 19

[Rik Cabanier]

Attachment: Fixed issue with premultiplied alpha

Comment 20

[Rik Cabanier]

Attachment: Adds blending to D2D backend

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Does this need to be reviewed again?

Comment 22

[Rik Cabanier]

Attachment: Added blending to D2D backend

Comment 23

[Rik Cabanier]

Attachment: Adds blending to D2D backend

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=b3fb0a2665ab

Comment 25

[Bas Schouten (:bas.schouten)]

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]

Attachment: Adds blending to D2D backend

Comment 27

[Rik Cabanier]

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

New Try run: https://tbpl.mozilla.org/?tree=Try&rev=c50a7872fa9d

Comment 29

[Rik Cabanier]

Attachment: Adds blending to D2D backend

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=7f6e86970acc

Comment 31

[Rik Cabanier]

Attachment: Adds blending to D2D backend

Comment 32

[Rik Cabanier]

Attachment: Adds blending to D2D backend

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

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]

Attachment: Adds blending to D2D backend

Comment 36

[Ryan VanderMeulen [:RyanVM]]

This patch just got 360k smaller. Is that expected?

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=245d38c91824

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Now with the right tests being run...
https://tbpl.mozilla.org/?tree=Try&rev=4726504f45b4

Comment 39

[Ryan VanderMeulen [:RyanVM]]

What info do you need? Comment 38 was from the last patch posted here.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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]

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

Same link as before.

Comment 43

[Rik Cabanier]

Attachment: Debug patch for GPU failures

Comment 44

[Rik Cabanier]

Added debug statements to determine failures in GPU code.
Please rebuild and test win7 debug and optimized

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=7809598e392b

Comment 46

[Rik Cabanier]

Attachment: More debugging for DirectX isue

Comment 47

[Rik Cabanier]

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

https://tbpl.mozilla.org/?tree=Try&rev=83b9bf20d434

Comment 49

[Rik Cabanier]

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]

Attachment: Simplified shader to see if the problem still happens

Comment 51

[Rik Cabanier]

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

https://tbpl.mozilla.org/?tree=Try&rev=fd4a5a813d4e

Comment 53

[Rik Cabanier]

Attachment: Adds blending to D2D backend

Comment 54

[Rik Cabanier]

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

https://tbpl.mozilla.org/?tree=Try&rev=b19932b82aed

Comment 56

[Rik Cabanier]

Attachment: Adds blending to D2D backend

Comment 57

[Rik Cabanier]

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

Attachment: Adds blending to D2D backend

Comment 59

[Rik Cabanier]

Attachment: Adds blending to D2D backend

Comment 60

[Rik Cabanier]

successful try run: https://tbpl.mozilla.org/?tree=Try&rev=0347f8e7512f
Please review latest patch

Comment 61

[Bas Schouten (:bas.schouten)]

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]

Attachment: Adds blending to D2D backend

Comment 63

[Rik Cabanier]

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

Attachment: Adds blending to D2D backend

Comment 65

[Bas Schouten (:bas.schouten)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1eedc9864f2d

Comment 66

[Ryan VanderMeulen [:RyanVM]]

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]]

https://hg.mozilla.org/mozilla-central/rev/1eedc9864f2d