Implement blend modes for Direct2D context

RESOLVED FIXED in mozilla20

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Rik Cabanier, Assigned: Rik Cabanier)

Tracking

unspecified
mozilla20
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 26 obsolete attachments)

632.82 KB, patch
Rik Cabanier
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
bug 748433 implements Canvas blending for Core Graphics and Cairo.
This patch implements it for Direct2D as well
(Assignee)

Updated

5 years ago
Depends on: 748433
(Assignee)

Comment 1

5 years ago
Created attachment 679741 [details] [diff] [review]
This is the shader program that implements blending
(Assignee)

Comment 2

5 years ago
Created attachment 679823 [details] [diff] [review]
This patch enables support for blending in the D2D backend
Attachment #679823 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #679741 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #679823 - Flags: review? → review?(bas)
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?
(Assignee)

Comment 4

5 years ago
Created attachment 682651 [details] [diff] [review]
Addressed Bas' comments
(Assignee)

Updated

5 years ago
Attachment #679823 - Attachment is obsolete: true
Attachment #679823 - Flags: review?(bas)
(Assignee)

Updated

5 years ago
Attachment #682651 - Attachment description: Addressed concerns → Addressed Bas' comments
Attachment #682651 - Flags: review?(bas)
(Assignee)

Comment 5

5 years ago
(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?
(Assignee)

Comment 6

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

I removed this code. I think it's obsolete.
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.
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
Created attachment 682843 [details] [diff] [review]
Fixed spacing issues. Added effect routine.
Attachment #682651 - Attachment is obsolete: true
Attachment #682651 - Flags: review?(bas)
Attachment #682843 - Flags: review?(bas)
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.
(Assignee)

Comment 11

5 years ago
Created attachment 682898 [details] [diff] [review]
more whitespace fixes
Attachment #682843 - Attachment is obsolete: true
Attachment #682843 - Flags: review?(bas)
Attachment #682898 - Flags: review?(bas)
(Assignee)

Updated

5 years ago
Attachment #682898 - Attachment is patch: true
(Assignee)

Updated

5 years ago
Attachment #682898 - Attachment is obsolete: true
Attachment #682898 - Flags: review?(bas)
(Assignee)

Comment 12

5 years ago
Created attachment 682910 [details] [diff] [review]
Fixed more whitespace issues
Attachment #682910 - Flags: review?(bas)
(Assignee)

Comment 13

5 years ago
(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
(Assignee)

Updated

5 years ago
Attachment #682910 - Attachment is obsolete: true
Attachment #682910 - Flags: review?(bas)
(Assignee)

Comment 14

5 years ago
Created attachment 682912 [details] [diff] [review]
more whitespace fixes
(Assignee)

Comment 15

5 years ago
Created attachment 682913 [details] [diff] [review]
removed ';'
Attachment #682913 - Flags: review?(bas)
(Assignee)

Updated

5 years ago
Attachment #682912 - Attachment is obsolete: true
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
Attachment #682913 - Flags: review?(bas) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 682933 [details] [diff] [review]
Added last (?) whitespace issues
Attachment #682933 - Flags: review+
(Assignee)

Updated

5 years ago
Blocks: 748433
No longer depends on: 748433
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Attachment #682913 - Attachment is obsolete: true
Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=ee59dfe33091
(Assignee)

Comment 19

5 years ago
Created attachment 683707 [details] [diff] [review]
Fixed issue with premultiplied alpha
Attachment #682933 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 683718 [details] [diff] [review]
Adds blending to D2D backend
Attachment #683707 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #683718 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #683718 - Attachment description: Fixed issue with premultiplied alpha → Adds blending to D2D backend
Does this need to be reviewed again?
Keywords: checkin-needed
(Assignee)

Comment 22

5 years ago
Created attachment 683841 [details] [diff] [review]
Added blending to D2D backend
Attachment #683718 - Attachment is obsolete: true
Attachment #683841 - Flags: review+
(Assignee)

Comment 23

5 years ago
Created attachment 683843 [details] [diff] [review]
Adds blending to D2D backend
Attachment #683841 - Attachment is obsolete: true
Attachment #683843 - Flags: review?
https://tbpl.mozilla.org/?tree=Try&rev=b3fb0a2665ab
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
(Assignee)

Comment 26

5 years ago
Created attachment 684235 [details] [diff] [review]
Adds blending to D2D backend
Attachment #683843 - Attachment is obsolete: true
Attachment #683843 - Flags: review?
(Assignee)

Comment 27

5 years ago
(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
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
New Try run: https://tbpl.mozilla.org/?tree=Try&rev=c50a7872fa9d
Flags: needinfo?(ryanvm)
(Assignee)

Comment 29

5 years ago
Created attachment 684585 [details] [diff] [review]
Adds blending to D2D backend
Attachment #684235 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
https://tbpl.mozilla.org/?tree=Try&rev=7f6e86970acc
Flags: needinfo?(ryanvm)
(Assignee)

Comment 31

5 years ago
Created attachment 684592 [details] [diff] [review]
Adds blending to D2D backend
Attachment #684585 - Attachment is obsolete: true
(Assignee)

Comment 32

5 years ago
Created attachment 684593 [details] [diff] [review]
Adds blending to D2D backend
Attachment #684592 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
https://tbpl.mozilla.org/?tree=Try&rev=15cfd0ca95ec

(BTW, the last two revisions haven't applied cleanly to mozilla-central. Please rebase)
Flags: needinfo?(ryanvm)
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.
(Assignee)

Comment 35

5 years ago
Created attachment 684807 [details] [diff] [review]
Adds blending to D2D backend
Attachment #684593 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
This patch just got 360k smaller. Is that expected?
Flags: needinfo?(ryanvm)
https://tbpl.mozilla.org/?tree=Try&rev=245d38c91824
Now with the right tests being run...
https://tbpl.mozilla.org/?tree=Try&rev=4726504f45b4
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
What info do you need? Comment 38 was from the last patch posted here.
Flags: needinfo?(ryanvm)
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
More runs triggered (I'm assuming that's what you wanted - in the future, a comment accompanying the needinfo request would be handy :)...)
Flags: needinfo?(ryanvm)
(Assignee)

Comment 41

5 years ago
(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
Same link as before.
(Assignee)

Comment 43

5 years ago
Created attachment 687269 [details] [diff] [review]
Debug patch for GPU failures
Attachment #684807 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
(Assignee)

Comment 44

5 years ago
Added debug statements to determine failures in GPU code.
Please rebuild and test win7 debug and optimized
https://tbpl.mozilla.org/?tree=Try&rev=7809598e392b
Flags: needinfo?(ryanvm)
(Assignee)

Comment 46

5 years ago
Created attachment 687471 [details] [diff] [review]
More debugging for DirectX isue
Attachment #687269 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
(Assignee)

Comment 47

5 years ago
(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
https://tbpl.mozilla.org/?tree=Try&rev=83b9bf20d434
Flags: needinfo?(ryanvm)
(Assignee)

Comment 49

5 years ago
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.
(Assignee)

Comment 50

5 years ago
Created attachment 687981 [details] [diff] [review]
Simplified shader to see if the problem still happens
Attachment #687471 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
(Assignee)

Comment 51

5 years ago
(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?
https://tbpl.mozilla.org/?tree=Try&rev=fd4a5a813d4e
Flags: needinfo?(ryanvm)
(Assignee)

Comment 53

5 years ago
Created attachment 688392 [details] [diff] [review]
Adds blending to D2D backend
Attachment #687981 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
(Assignee)

Comment 54

5 years ago
(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.
https://tbpl.mozilla.org/?tree=Try&rev=b19932b82aed
Flags: needinfo?(ryanvm)
(Assignee)

Comment 56

5 years ago
Created attachment 689300 [details] [diff] [review]
Adds blending to D2D backend
Attachment #688392 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
(Assignee)

Comment 57

5 years ago
(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?
(Assignee)

Comment 58

5 years ago
Created attachment 690200 [details] [diff] [review]
Adds blending to D2D backend
Attachment #689300 - Attachment is obsolete: true
(Assignee)

Comment 59

5 years ago
Created attachment 690209 [details] [diff] [review]
Adds blending to D2D backend
Attachment #690200 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
(Assignee)

Comment 60

5 years ago
successful try run: https://tbpl.mozilla.org/?tree=Try&rev=0347f8e7512f
Please review latest patch
(Assignee)

Updated

5 years ago
Attachment #690209 - Flags: review?(bas)
(Assignee)

Updated

5 years ago
Attachment #690209 - Flags: checkin?(bas)
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 :)
Attachment #690209 - Flags: review?(bas) → review+
(Assignee)

Comment 62

5 years ago
Created attachment 690583 [details] [diff] [review]
Adds blending to D2D backend
Attachment #690209 - Attachment is obsolete: true
Attachment #690209 - Flags: checkin?(bas)
Attachment #690583 - Flags: checkin?(bas)
(Assignee)

Comment 63

5 years ago
(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
(Assignee)

Updated

5 years ago
Attachment #690583 - Flags: checkin?(bas)
(Assignee)

Comment 64

5 years ago
Created attachment 691409 [details] [diff] [review]
Adds blending to D2D backend
Attachment #690583 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #691409 - Flags: checkin?
(Assignee)

Updated

5 years ago
Attachment #691409 - Flags: checkin? → checkin+
http://hg.mozilla.org/integration/mozilla-inbound/rev/1eedc9864f2d
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.
https://hg.mozilla.org/mozilla-central/rev/1eedc9864f2d
Assignee: nobody → cabanier
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 836892
You need to log in before you can comment on or make changes to this bug.