940959 - Use luminance textures instead of alpaha textures in D3D9 and D3D11 shaders.

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

Our D3D shaders are using luminance textures for the Y Cb and Cr planes, while using alpha textures for masks. This makes it impractical to refer to 8bits one-channel textures in a generic way, and does not play well with most of Gecko not making a difference between A8 and L8 (Moz2D for instance only has FORMAT_A8).

Our GL shaders, on the other hand always use GL_LUMINANCE textures, so no distinction between masks and YUV planes, which is good.

Let's do with D3D the same thing we do with GL.

Comment 1

[Nicolas Silva [:nical]]

Actually I am doing it the otherway around: YCbCr planes to use alpha textures. This is a much smaller change than switching masks to L8 because the latter requires to modify Moz2D, and the YCbCr path is more contained and easier to test so I'd rather modify it than the mask stuff.

Comment 2

[Nicolas Silva [:nical]]

Attachment: D3D9: Use A8 textures for YCbCr planes

Comment 3

[Nicolas Silva [:nical]]

Attachment: D3D10: Use A8 textures for YCbCr planes

Comment 4

[Nicolas Silva [:nical]]

Attachment: D3D11: Use A8 textures for YCbCr planes

Comment 5

[Nicolas Silva [:nical]]

Attachment: Compiled shaders for D3D9, D3D10 and D3D11

This is just generated code so it actually contains nothing for review.

Comment 6

[Nick Cameron [:nrc]]

Comment on attachment 8335370 [details] [diff] [review]
D3D9: Use A8 textures for YCbCr planes

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

Please use eight lines of context for your patches, it makes it much easier to review

::: gfx/layers/d3d9/LayerManagerD3D9Shaders.hlsl
@@ +13,4 @@
>  sampler s2DY;
>  sampler s2DCb;
>  sampler s2DCr;
> +sampler s2DMask;

this seems to be a noop, please remove it

Comment 7

[Nick Cameron [:nrc]]

Comment on attachment 8335373 [details] [diff] [review]
D3D10: Use A8 textures for YCbCr planes

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

I don't think we need to do this do we? We are not going to use new textures here and we'll remove it soon in any case. r=me if you really want it though

Comment 8

[Nicolas Silva [:nical]]

try push https://tbpl.mozilla.org/?tree=Try&rev=08b3c1bd9647

Comment 9

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #7)
> Comment on attachment 8335373 [details] [diff] [review]
> D3D10: Use A8 textures for YCbCr planes
> 
> Review of attachment 8335373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think we need to do this do we? We are not going to use new textures
> here and we'll remove it soon in any case. r=me if you really want it though

We don't really need to, but I think it's dangerous to leave subtle differences like this between backends. The fact that D3D was doing something different than GL where it didn't need to already lead me to spend almost a day looking in the wrong directions. The patch is simple enough, I think, that the risk of taking it is smaller than the risk of later causing bugs because there is this subtle difference that may make things fail silently.

Comment 10

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f48eebd7b4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0fa18d121c
https://hg.mozilla.org/integration/mozilla-inbound/rev/36433cc86d07
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0544dfcc75c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f48eebd7b4a
https://hg.mozilla.org/mozilla-central/rev/6a0fa18d121c
https://hg.mozilla.org/mozilla-central/rev/36433cc86d07
https://hg.mozilla.org/mozilla-central/rev/f0544dfcc75c