900244 - Do new TextureHost/Client stuff for d3d9 compositor

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

The d3d9 compositor currently only uses the old model textures, it should use the new ones.

Comment 1

[Nicolas Silva [:nical]]

I am at point where I have Both ImageLayers and Canvas2D working with new D3D9 textures. It's rather simple because at the moment those two only use Shmem/Memory to transfer textures so I only had to properly implement DataTextureSourceD3D9 to get things working, and all the rest is cross platform code.

I still have the TextureHostD3D9 and TextureHostDIB to convert to new textures but those are only used by ContentClient/ContentHost, the new textures version of which has not landed yet.
Once the new ContentClient/Host lands, it will be able fallback to Shmem/Memory so I think we can split the D3D9 textures work 3:
* 1) Getting the general shmem/memory backed textures to work (which makes new textures available on Windows D3D9)
* 2) and getting the the D3D9 texture and DIB texture to work, which will provide faster paths for the ContentClient/Host stuff.
* 3) enable the new textures pref by default

The first one is almost ready (needs a try run), while the second one would be best implemented when the new ContentClient/Host of 893301 is landed.
Enabling the pref can happen before 2) happens, and will not make any difference as long as OMTC on windows is not enabled all together.

Comment 2

[Nicolas Silva [:nical]]

Attachment: Implement DataTextureSource for D3D9

Comment 3

[Nicolas Silva [:nical]]

Attachment: Change how the compositor handles YCbCr textures

This patch makes the D3D9 compositor handle Y, Cb, and Cr planes as separate TextureSources just like the GL compositor does.
It's sort of required in order to be compatible with the new textures model.
So I had to change the deprecated D3D9 YCbCr texture host a bit.
Basically it acts like ShmemTextureHost, so it's mostly a duplicate of what the new stuff does, and reuses DataTextureSourceD3D9 instead of doing its own thing for texture uploads.

Comment 4

[Nicolas Silva [:nical]]

Attachment: Add the missing glue to create new textures with D3D9.

Right now this just relies on the Backend-independent texture hosts (shmem and memory) and the DataTextureSourceD3D9 add in another patch of this bug.
With that and the other two patches we can enable new textures on D3D9 and it works well.

Comment 5

[Nick Cameron [:nrc]]

Comment on attachment 8336102 [details] [diff] [review]
Add the missing glue to create new  textures with D3D9.

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

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +54,5 @@
> +                                                   aDeallocator, aFlags);
> +      break;
> +    }
> +    case SurfaceDescriptor::TSurfaceDescriptorD3D9: {
> +      // TODO

change these TODOs to MOZ_CRASH, it would be good to really notice if we hit these paths. And add the bug number in a comment please.

Comment 6

[Nick Cameron [:nrc]]

Going to take a bit of time to do these reviews because there is a lot to check, I'll try and get them done today, but might be Monday. Please fix your hg setup to generate patches with 8 lines of context (its not actually so important for these patches because it is mostly big chunks of new code, but in general reviewing patches with 3 lines of context is painful).

Comment 7

[Nick Cameron [:nrc]]

I just landed bug 900248. Some of those patches change how texture clients and hosts work quite a lot, so it is probably taking another look at these patches to make sure they still do the right thing. Also please do a try push on top of 900248 and with OMTC pref'ed on to make sure nothing regresses. Please also test this manually - in particular test resizing the window and these things (https://bugzilla.mozilla.org/show_bug.cgi?id=900248#c33) both with and without d3d9Ex (by commenting this line http://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/DeviceManagerD3D9.cpp#22).

Comment 8

[Nick Cameron [:nrc]]

Comment on attachment 8336095 [details] [diff] [review]
Implement DataTextureSource for D3D9

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

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +854,5 @@
> +                              gfx::IntPoint* aSrcOffset)
> +{
> +  // Right now we only support null aDestRegion and aSrcOffset (which means)
> +  // full surface update. Incremental update is only used on Mac so it is
> +  // not clear that we ever will need to support it for D3D.

change this comment to an assert

@@ +855,5 @@
> +{
> +  // Right now we only support null aDestRegion and aSrcOffset (which means)
> +  // full surface update. Incremental update is only used on Mac so it is
> +  // not clear that we ever will need to support it for D3D.
> +  if (!mCompositor) {

assert mCompositor and check mCompositor->device()

@@ +864,5 @@
> +  uint32_t bpp = 0;
> +
> +  _D3DFORMAT format = D3DFMT_A8R8G8B8;
> +  mFormat = aSurface->GetFormat();
> +  // XXX - we ignore quite a few formats, is that intentional?

that is fine - we catch that in the default clause and we really don't expect any other formats. If the platform independent new texture client is likely to provide data in a different format you should add it here.

@@ +870,5 @@
> +  case FORMAT_B8G8R8X8:
> +    format = D3DFMT_X8R8G8B8;
> +    bpp = 4;
> +    break;
> +  case gfx::FORMAT_B8G8R8A8:

unnecessary prefix

@@ +888,5 @@
> +  if (mSize.width <= maxSize && mSize.height <= maxSize) {
> +    mTextures[0] = DataToTexture(mCompositor->device(),
> +                                 aSurface->GetData(), aSurface->Stride(),
> +                                 ThebesIntSize(mSize), format, bpp);
> +    NS_ASSERTION(mTextures[0], "Could not upload texture");

we should check this rather than asserting, also need to check in the tiled case too

::: gfx/layers/d3d9/TextureD3D9.h
@@ +99,5 @@
> +    mIterating = true;
> +    mCurrentTile = 0;
> +  }
> +
> +

nit remove the extra new line

Comment 9

[Nick Cameron [:nrc]]

Comment on attachment 8336099 [details] [diff] [review]
Change how the compositor handles YCbCr textures

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

Pretty much looks right, but there are a lot of small changes and I think it needs rebasing too, so I would like to see new version.

::: gfx/layers/d3d9/CompositorD3D9.cpp
@@ +311,5 @@
>                                             textureCoords.width,
>                                             textureCoords.height),
>                                           1);
> +
> +      const int Y = 0, Cb = 1, Cr = 2;

We should have these as proper constants somewhere, this is way too error prone

@@ +315,5 @@
> +      const int Y = 0, Cb = 1, Cr = 2;
> +      TextureSource *source = ycbcrEffect->mTexture;
> +      TextureSourceD3D9 *sourceY  = source->GetSubSource(Y)->AsSourceD3D9();
> +      TextureSourceD3D9 *sourceCb = source->GetSubSource(Cb)->AsSourceD3D9();
> +      TextureSourceD3D9 *sourceCr = source->GetSubSource(Cr)->AsSourceD3D9();

nit: asterisk on the type, not the variable for all vars.

@@ +326,5 @@
>         * Send 3d control data and metadata
>         */
>        if (mDeviceManager->GetNv3DVUtils()) {
>          Nv_Stereo_Mode mode;
> +        switch (source->AsSourceD3D9()->GetStereoMode()) {

pull the sourceD3D9 out as a variable rather than getting it multiple times. Maybe source can just be a TextureSourceD3D9

::: gfx/layers/d3d9/LayerManagerD3D9Shaders.hlsl
@@ +212,4 @@
>    float4 alphas = 1.0 - tex2D(s2DWhite, aVertex.vTexCoords) + src;
>    alphas.a = alphas.g;
>    float2 maskCoords = aVertex.vMaskCoords;
> +  float mask = tex2D(s2DMask, maskCoords).r;

Why do we need all these a -> r changes in the hlsl? Didn't you just land a patch changing everything from r -> a? And I don't see anything changing mask layers to using luminance rather than alpha. If you do want these changes, then you need the changes to the compiled file (.h) too

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +398,5 @@
> +  RefPtr<gfx::DataSourceSurface> tempCr =
> +    gfx::Factory::CreateWrappingDataSourceSurface(yuvDeserializer.GetCrData(),
> +                                                  yuvDeserializer.GetCbCrStride(),
> +                                                  gfx::ToIntSize(yuvDeserializer.GetCbCrSize()),
> +                                                  gfx::FORMAT_A8);

don't need the gfx:: namespaces

@@ +399,5 @@
> +    gfx::Factory::CreateWrappingDataSourceSurface(yuvDeserializer.GetCrData(),
> +                                                  yuvDeserializer.GetCbCrStride(),
> +                                                  gfx::ToIntSize(yuvDeserializer.GetCbCrSize()),
> +                                                  gfx::FORMAT_A8);
> +  // We don't support partial updates for Y U V textures

don't need the comment since it is just repeated in the assert

@@ +408,5 @@
> +    NS_WARNING("failed to update the DataTextureSource");
> +    return;
> +  }
> +
> +  mSize = IntSize(size.width, size.height);

repeated code, please remove

@@ +468,4 @@
>      mFormat = FORMAT_B8G8R8A8;
>      bpp = 4;
>      break;
> +  case D3DFMT_L8:

do you really want to change this?

@@ +583,4 @@
>      break;
>    case gfxImageFormatA8:
>      mFormat = FORMAT_A8;
> +    format = D3DFMT_L8;

and this?

@@ +663,4 @@
>      // fallback to DIB texture client
>      return false;
>    case GFX_CONTENT_ALPHA:
> +    format = D3DFMT_L8;

and this?

@@ +883,4 @@
>    , mIsTiled(false)
>    , mIterating(false)
>  {
> +  mStereoMode = aStereoMode;

This is a field in the superclass right? You should add initialise it there and add it to that constructor.

@@ +934,5 @@
> +    mTexture = DataToTexture(mCompositor->device(),
> +                             aSurface->GetData(), aSurface->Stride(),
> +                             ThebesIntSize(mSize), format, bpp);
> +    NS_ASSERTION(mTexture, "Could not upload texture");
> +    MOZ_ASSERT(mTexture);

I don't think you need to assert this twice. I mean better safe than sorry, but this seems over the top :-p

::: gfx/layers/d3d9/TextureD3D9.h
@@ +46,5 @@
>  {
>  public:
> +  DataTextureSourceD3D9(gfx::SurfaceFormat aFormat,
> +                        CompositorD3D9* aCompositor,
> +                        bool aAllowBiImage = true,

missing 'g'

@@ +97,4 @@
>    RefPtr<CompositorD3D9> mCompositor;
>    gfx::SurfaceFormat mFormat;
>    uint32_t mCurrentTile;
> +  bool mDisallowBigImage;

I find this ugly, I can't really explain why. Why do we need it in the first place? Is it worth keeping a TextureFlags member around rather than just one bool?

@@ +228,5 @@
>  {
>  public:
> +  DeprecatedTextureHostYCbCrD3D9();
> +
> +  ~DeprecatedTextureHostYCbCrD3D9();

nit: virtual please

Comment 10

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #9)
> Comment on attachment 8336099 [details] [diff] [review]
> Change how the compositor handles YCbCr textures
> 
> @@ +326,5 @@
> >         * Send 3d control data and metadata
> >         */
> >        if (mDeviceManager->GetNv3DVUtils()) {
> >          Nv_Stereo_Mode mode;
> > +        switch (source->AsSourceD3D9()->GetStereoMode()) {
> 
> pull the sourceD3D9 out as a variable rather than getting it multiple times.
> Maybe source can just be a TextureSourceD3D9

TextureSourceD3D9 does not inherit from TextureSource so I'd need to keep both around. Calling AsSourceD3D9 twice doesn't look that bad.

> 
> ::: gfx/layers/d3d9/LayerManagerD3D9Shaders.hlsl
> @@ +212,4 @@
> >    float4 alphas = 1.0 - tex2D(s2DWhite, aVertex.vTexCoords) + src;
> >    alphas.a = alphas.g;
> >    float2 maskCoords = aVertex.vMaskCoords;
> > +  float mask = tex2D(s2DMask, maskCoords).r;
> 
> Why do we need all these a -> r changes in the hlsl? Didn't you just land a
> patch changing everything from r -> a? And I don't see anything changing
> mask layers to using luminance rather than alpha. If you do want these
> changes, then you need the changes to the compiled file (.h) too

Oh darn, I badly messed up my rebase.

> @@ +97,4 @@
> >    RefPtr<CompositorD3D9> mCompositor;
> >    gfx::SurfaceFormat mFormat;
> >    uint32_t mCurrentTile;
> > +  bool mDisallowBigImage;
> 
> I find this ugly, I can't really explain why. Why do we need it in the first
> place? Is it worth keeping a TextureFlags member around rather than just one
> bool?

We need it because tiling the texture breaks video (since Y and Cb/Cr planes are not the same size), I'll keep the flag instead of a bool (I hesitated but the existing deprecated version used a bool).

Comment 11

[Nicolas Silva [:nical]]

New try push for the patches to come: https://tbpl.mozilla.org/?tree=Try&rev=89e0430921ff
D3D9 + D3D11 with OMTC and deprecated textures

Comment 12

[Nicolas Silva [:nical]]

Same thing with new textures: https://tbpl.mozilla.org/?tree=Try&rev=54b3d7dbe00e

Comment 13

[Nicolas Silva [:nical]]

Attachment: Make the deprecated D3D9 YCbCr texture host use DataTextureSource

Comment 14

[Nicolas Silva [:nical]]

Attachment: (followup) Make the DataTextureSource store the TextureFlags instead of a boolean

Followup patch addressing Nick's comment about passing a boolean to allow/disallow big images in DataTextureSource.

Comment 15

[Nick Cameron [:nrc]]

Comment on attachment 8337809 [details] [diff] [review]
(followup) Make the DataTextureSource store the TextureFlags instead of a boolean

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

When I saw the size of this patch, I felt bad for asking you do it for basically a nit, but actually it is a nice improvement to the code, so now I don't feel bad :-) Thanks for doing it!

Comment 16

[Nick Cameron [:nrc]]

Comment on attachment 8337807 [details] [diff] [review]
Make the deprecated D3D9 YCbCr texture host use DataTextureSource

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

::: gfx/layers/d3d9/CompositorD3D9.cpp
@@ +88,5 @@
>  
> +TemporaryRef<DataTextureSource>
> +CompositorD3D9::CreateDataTextureSource(TextureFlags aFlags)
> +{
> +  return new DataTextureSourceD3D9(gfx::FORMAT_UNKNOWN, this,

unnecessary gfx::

@@ +330,5 @@
>                                               textureCoords.width,
>                                               textureCoords.height),
>                                             1);
> +
> +      const int Y = 0, Cb = 1, Cr = 2;

these should be constants in CompsoitorTypes.h or somewhere so we are sure that the client and host are using the same values.

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +398,5 @@
> +    srcY->SetNextSibling(srcU);
> +    srcU->SetNextSibling(srcV);
> +  } else {
> +    MOZ_ASSERT(mFirstSource->GetNextSibling());
> +    MOZ_ASSERT(mFirstSource->GetNextSibling()->GetNextSibling());

should we check that the size and stereo mode are still the same? We should at least assert they haven't changed, I think

@@ +404,5 @@
> +    srcU = mFirstSource->GetNextSibling()->AsDataTextureSource();
> +    srcV = mFirstSource->GetNextSibling()->GetNextSibling()->AsDataTextureSource();
> +  }
> +
> +  RefPtr<gfx::DataSourceSurface> tempY =

I would prefer name like wrapper(Y/Cb/Cr) rather than temp(Y/Cb/Cr) because you are just wrapping the data from the serialiser, not creating a copy (right?)

@@ +414,5 @@
> +    gfx::Factory::CreateWrappingDataSourceSurface(yuvDeserializer.GetCbData(),
> +                                                  yuvDeserializer.GetCbCrStride(),
> +                                                  gfx::ToIntSize(yuvDeserializer.GetCbCrSize()),
> +                                                  gfx::FORMAT_A8);
> +  RefPtr<gfx::DataSourceSurface> tempCr =

you use src(Y/Cb/Cr) but temp(y/u/v) please be consistent (also the comment below)

@@ +425,5 @@
> +  if (!srcY->Update(tempY,  TEXTURE_FLAGS_DEFAULT) ||
> +      !srcU->Update(tempCb, TEXTURE_FLAGS_DEFAULT) ||
> +      !srcV->Update(tempCr, TEXTURE_FLAGS_DEFAULT)) {
> +    NS_WARNING("failed to update the DataTextureSource");
> +    return;

You should null out a bunch of stuff here - at least mSize and mFirstTexture, maybe some other stuff too

@@ +426,5 @@
> +      !srcU->Update(tempCb, TEXTURE_FLAGS_DEFAULT) ||
> +      !srcV->Update(tempCr, TEXTURE_FLAGS_DEFAULT)) {
> +    NS_WARNING("failed to update the DataTextureSource");
> +    return;
> +  }

Please remove all the gfx:: namespaces in this block

@@ +508,1 @@
>                                      texture, mSize, format);

nit: indent

@@ +621,1 @@
>                                      surf, mSize, format);

nit: indent

@@ +981,1 @@
>                                   aSurface->GetData(), aSurface->Stride(),

nit: indent

Comment 17

[Nicolas Silva [:nical]]

a new try push https://tbpl.mozilla.org/?tree=Try&rev=8b42ff9edd63

Comment 18

[Nicolas Silva [:nical]]

Attachment: Various fixes

At this point I lost the patience to fix nits and other issues in each patch individually so the latest fixes ended up here (sorry).

Comment 19

[Nicolas Silva [:nical]]

Attachment: Implement the remaining D3D9 TextureClients and Hosts

Comment 20

[Nick Cameron [:nrc]]

Comment on attachment 8350263 [details] [diff] [review]
Implement the remaining D3D9 TextureClients and Hosts

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

r=me with or without SharedTextureClientD3D9

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +1418,5 @@
> +{
> +  MOZ_ASSERT(mIsLocked && IsAllocated());
> +
> +  if (!mDrawTarget) {
> +    printf("DIBTextureClientD3D9::GetAsDrawTarget create DrawTarget\n");

remove

@@ +1481,5 @@
> +
> +void
> +SharedTextureClientD3D9::Unlock()
> +{
> +  MOZ_ASSERT(mIsLocked, "Unlocked called while the texture is not locked!");

s/Unlocked/Unlock

::: gfx/layers/d3d9/TextureD3D9.h
@@ +175,5 @@
>  };
>  
> +/**
> + * Can only be drawn into through Cairo, and only support opaque surfaces.
> + */

So, aiui CairoTextureClientD3D9 ->TextureHostD3D9
DIBTetxureCLientD3D9 -> DIBTextureHostD3D9
SharedTextureClientD3D9 -> ?

Could you put something to this effect in the comments somewhere please?

@@ +264,5 @@
> +  gfx::SurfaceFormat mFormat;
> +  bool mIsLocked;
> +};
> +
> +class SharedTextureClientD3D9 : public TextureClient,

what is this used for now?

@@ +350,5 @@
> +
> +/**
> + * Supports alpha, and uses a gfxWindowsSurface which lets us do native drawing
> + * directly into it.
> + * For opaque surfaces, prefer CairoTextureClientD3D9.

texture client/host confusion

Comment 21

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd1d484f6d22
https://hg.mozilla.org/integration/mozilla-inbound/rev/1562cd2c5a77
https://hg.mozilla.org/integration/mozilla-inbound/rev/620155e87cbb

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/620155e87cbb
https://hg.mozilla.org/mozilla-central/rev/1562cd2c5a77
https://hg.mozilla.org/mozilla-central/rev/dd1d484f6d22