938591 - Use the new texture clients and hosts with D3D11

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

Let's get rid of deprecated textures on windows.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Fallback to thebes if creating a DrawTarget fails in BufferTextureClient::UpdateSurface

gfx::Factory::CreateDrawTargetForData doesn't seem to have a code path for the D2D backend which makes BufferTextureClient crash.

This patch makes us fallback to thebes to copy the surface in shared memory if creating a DrawTarget failed.

The best solution should be to implement the D2D backend code path in Factory.cpp but I think it's outside the scope of this bug.

Comment 2

[Nicolas Silva [:nical]]

Attachment: Implement DataTextureSource for the D3D11 backend

This was initially implemented by Vlad in another bug but never landed, then I resurrected it in bug 858914 (d3d11 backend patch) and it didn't land either although it was r+ed by Bas.

Here it is once again. I separated it from the rest of the new textures D3D11 code to make review a tad easier. With this patch DataTextureSourceD3D11 is not used anywhere, but will be in the next patch.

I don't think it needs another review, as it hasn't changed since Bas r+ed it in bug 858914.

Comment 3

[Nicolas Silva [:nical]]

Attachment: D3D11 backend for the new textures

Here is the meat of the D3D11 new textures work.
It is mostly a rebase of what I did in bug 858914 which got r+ed by Bas. I had to change DeprecatedTextureHostYCbCrD3D11 a bit though, so it probably needs a quick re-review.

With this (and the rest of the patch queue), new textures work on windows, but are still preffed off by default.
Async video on windows will need a bit of extra work, as it hits an assertion because of some code being invoked in the wrong thread (after removing the assertion async-video works, but I didn't investigate further).

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 832247 [details] [diff] [review]
Fallback to thebes if creating a DrawTarget fails in BufferTextureClient::UpdateSurface

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

This has become pointless on trunk. Thebes has been completely deprecated.

Comment 5

[Nicolas Silva [:nical]]

Attachment: Fallback to cairo if creating a DrawTarget fails in BufferTextureClient::UpdateSurface

This version does not use thebes but falls back to a cairo draw target in ImageDataSerializer if the preferred content drawing backend does not support rendering to RAM.

Comment 6

[Nick Cameron [:nrc]]

Could we not pref them on before I pref on Windows OMTC please? I'd hate to have to put off doing that even further :-( That would mean this all got tested properly too.

Comment 7

[Bas Schouten (:bas.schouten)]

Comment on attachment 832367 [details] [diff] [review]
Fallback to cairo if creating a DrawTarget fails in BufferTextureClient::UpdateSurface

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

::: gfx/layers/ImageDataSerializer.cpp
@@ +138,5 @@
> +                                                        GetFormat());
> +  if (!dt) {
> +    // Some backends like D2D don't support rendering to RAM, it is OK to
> +    // fallback to Cairo since the result of the draw target is not going
> +    // to be drawn back into a D2D DrawTarget.

I feel we should hide this in gfxPlatform::CreateDrawTargetForData somehow..

Comment 8

[Nicolas Silva [:nical]]

(In reply to Bas Schouten (:bas.schouten) from comment #7)
> I feel we should hide this in gfxPlatform::CreateDrawTargetForData somehow..

Matt had a different opinion on the subject:

<nical> mattwoodrow: hm ok, and would it be a crime to fallback to cairo if mContentBackend (D2D) does not support CreateDrawTargetForData ?
<mattwoodrow> nical: I think it would be :)
<mattwoodrow> we don't support interop between DT types, so if someone tried to do it for a temp and then draw the results back to their main DT it wouldn't work
<nical> mattwoodrow: if I do the fallback to cairo, but in TextureClient, would it be ok?
<mattwoodrow> yep!

I don't really mind either way, although matt has a point about not making it easy for people to mix different backends without explicitly asking for it.

Comment 9

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #6)
> Could we not pref them on before I pref on Windows OMTC please? I'd hate to
> have to put off doing that even further :-( That would mean this all got
> tested properly too.

Sounds reasonable, approximately how long do you think we have before OMTC Windows land?

Comment 10

[Nicolas Silva [:nical]]

I am facing the following problem (with bith D3D11 and D3D9): We use luminance textures (L8) for YCbCr surfaces and alpha textures for masks (A8). So the shaders will sample the red channels for YCbCr planes while a mask will use the alpha channel. At the TextureSource level it's very annoying because the DataTextureSource does not need to know that it has an alpha or a luminance texture. Plus, Moz2D does not have the notion of luminance texture (should we then add gfx::FORMAT_L8?).

Is there a reason why our shaders don't just always use alpha textures hwn referring to 8bits-one-channel textures?

Making this distinction between A8 and L8 is error prone (sampling from the wrong channel will silently give you 0, so it took me a big part of yesterday figuring out that I was just not reading the right channel) and I don't see what it buys us.

Can we move to always using alpha (or luminance but stick to either of he two) textures? Or should we add a FORMAT_L8 in Moz2D (but then make it harder to generically refer to 8bits mono-channel textures)?

Our GL shaders only use GL_LUMINANCE textures.

Comment 11

[Nick Cameron [:nrc]]

Bas said on irc yesterday that it probably doesn't matter if we use A8 or L8 textures for d3d9. L8 might be better supported with older drivers (which is why it was used for d3d9).

I would just one or the other and not add an extra type to Moz2D (but do add a comment on the enum that it might get implemented as L8 rather than A8, if you do that).

I guess if we use L8 for OGL already, we should maybe use L8 everywhere?

Comment 12

[Nicolas Silva [:nical]]

Comment on attachment 832266 [details] [diff] [review]
D3D11 backend for the new textures

Cancelling the review because I'll have to update the patch to use alpha textures. Also forgot to add support to BigImage in the DataTextureSource. Will add it to the next version (not sure how often we encounter textures bigger than the max textrue size on D3D11 hardware but let's be on the safe side).

Comment 13

[Nicolas Silva [:nical]]

Attachment: Implement DataTextureSource for the D3D11 backend

Updated patch, the DataTextureSource now supports tiling. Most of the implementation is based on ShmemDeprecatedTextureHost (for texture uploads and the tile iteration stuff).

Comment 14

[Nicolas Silva [:nical]]

Attachment: D3D11 backend for the new textures

Comment 15

[Nicolas Silva [:nical]]

Attachment: Fallback to cairo if creating a DrawTarget fails (v2)

Bas, how about implementing it in gfxPlatform, while still making sure calling code opts in doing the fallback to cairo, to avoid mixing backends without knowing, like this?

Comment 16

[Bas Schouten (:bas.schouten)]

(In reply to Nicolas Silva [:nical] from comment #15)
> Created attachment 8337861 [details] [diff] [review]
> Fallback to cairo if creating a DrawTarget fails (v2)
> 
> Bas, how about implementing it in gfxPlatform, while still making sure
> calling code opts in doing the fallback to cairo, to avoid mixing backends
> without knowing, like this?

The pref for content backends should be a list? So we should have 'fallback' options, if gfxPlatform is not exposing that info, we should fix that up, but the list in that pref should be respected when picking backends.

Comment 17

[Bas Schouten (:bas.schouten)]

Comment on attachment 8337861 [details] [diff] [review]
Fallback to cairo if creating a DrawTarget fails (v2)

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

See previous comment :).

Comment 18

[Nick Cameron [:nrc]]

Comment on attachment 8337812 [details] [diff] [review]
Implement DataTextureSource for the D3D11 backend

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

r=me with nits fixed

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +340,5 @@
>  
> +TemporaryRef<DataTextureSource>
> +CompositorD3D11::CreateDataTextureSource(TextureFlags aFlags)
> +{
> +  RefPtr<DataTextureSource> result = new DataTextureSourceD3D11(gfx::FORMAT_UNKNOWN,

gfx::

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +20,5 @@
>  
>  namespace layers {
>  
> +static DXGI_FORMAT
> +SurfaceFormatToDXGIFormat(gfx::SurfaceFormat aFormat)

gfx:: (and check elsewhere in the file too please)

@@ +34,5 @@
> +      MOZ_ASSERT(false, "unsupported format");
> +  }
> +}
> +
> +static uint32_t GetRequiredTilesD3D11(uint32_t aSize, uint32_t aMaxSize)

nit: newline after type

::: gfx/layers/d3d11/TextureD3D11.h
@@ +64,5 @@
> +
> +  DataTextureSourceD3D11(ID3D11Texture2D* aTexture,
> +                         gfx::SurfaceFormat aFormat);
> +
> +  ~DataTextureSourceD3D11();

nit: virtual

@@ +108,5 @@
> +  gfx::IntRect GetTileRect(uint32_t aIndex) const;
> +
> +  RefPtr<CompositorD3D11> mCompositor;
> +  gfx::SurfaceFormat mFormat;
> +  std::vector< RefPtr<ID3D11Texture2D> > mTileTextures;

put this field first in the list

@@ +119,1 @@
>  class CompositingRenderTargetD3D11 : public CompositingRenderTarget,

nit: newline after class def

Comment 19

[Nick Cameron [:nrc]]

Comment on attachment 8337816 [details] [diff] [review]
D3D11 backend for the new textures

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

Most of the comments are nits. r=me, assuming none of the questions lead to big changes.

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +583,5 @@
>        SetSamplerForFilter(FILTER_LINEAR);
>  
>        mVSConstants.textureCoords = ycbcrEffect->mTextureCoords;
>  
> +      const int Y = 0, Cb = 1, Cr = 2;

use the same constants I asked you to extract for the other bug

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

nit: asterisks to the left

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +72,5 @@
>    MOZ_COUNT_DTOR(DataTextureSourceD3D11);
>  }
>  
>  
> +template<typename T> // ID3D10Texture2D or ID3D11Texture2D

Why are these generic? We never want to use d3d10 textures afaik

@@ +113,5 @@
> +    default: {
> +      NS_WARNING("Unsupported SurfaceDescriptor type");
> +    }
> +  }
> +  return result.forget();

doesn't need forget()

@@ +121,5 @@
> +TextureClientD3D11::Lock(OpenMode aMode)
> +{
> +  MOZ_ASSERT(!mIsLocked, "The Texture is already locked!");
> +  LockD3DTexture(mTexture.get());
> +  return true;

should you set mIsLocked=true here?

@@ +132,5 @@
> +  UnlockD3DTexture(mTexture.get());
> +  mIsLocked = false;
> +}
> +
> +DataTextureSourceD3D11::DataTextureSourceD3D11(ID3D11Texture2D* aTexture,

is this in the wrong patch or just the wrong place?

@@ +195,5 @@
> +
> +NewTextureSource*
> +DXGITextureHostD3D11::GetTextureSources()
> +{
> +  return mTextureSource.get();

unnecessary get

@@ +707,5 @@
>  
> +DeprecatedTextureHostYCbCrD3D11::DeprecatedTextureHostYCbCrD3D11()
> +  : mCompositor(nullptr)
> +{
> +  mFormat = gfx::FORMAT_YUV;

remove gfx:: namespace, and please check the rest of the changes for this nit

@@ +715,5 @@
> +{
> +}
> +
> +ID3D11Device*
> +DeprecatedTextureHostYCbCrD3D11::GetDevice()

Why do we sometimes use mDevice and sometimes use GetDevice()? Is it more safe in some cases?

@@ +728,2 @@
>  
> +  RefPtr<NewTextureSource> source = mFirstSource.get();

why are you getting the raw pointer and then saving it into a RefPtr? I think you just mean NewTextureSource*

@@ +728,4 @@
>  
> +  RefPtr<NewTextureSource> source = mFirstSource.get();
> +  while (source) {
> +    static_cast<DataTextureSourceD3D11*>(source.get())->SetCompositor(mCompositor);

then you can skip this .get() too

@@ +747,5 @@
>    gfxIntSize size = yuvDeserializer.GetYSize();
>  
> +  RefPtr<DataTextureSource> srcY;
> +  RefPtr<DataTextureSource> srcU;
> +  RefPtr<DataTextureSource> srcV;

YUV/YCbCr consistency

@@ +766,2 @@
>  
> +  RefPtr<gfx::DataSourceSurface> tempY =

temp naming issue as in d3d9 patch

@@ +784,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;

do you need to null some stuff out to indicate we don't have all our textures?

::: gfx/layers/d3d11/TextureD3D11.h
@@ +154,5 @@
> +
> +protected:
> +  RefPtr<DataTextureSourceD3D11> mTextureSource;
> +  RefPtr<ID3D11Device> mDevice;
> +  gfx::IntSize mSize;

mSize above pointers please

@@ +161,5 @@
> +  bool mIsLocked;
> +};
> +
> +/**
> + * A render target for use with the D3D11 backend.

this comment is unnecessary

@@ +345,4 @@
>  {
>  public:
> +  DeprecatedTextureHostYCbCrD3D11();
> +  ~DeprecatedTextureHostYCbCrD3D11();

virtual

Comment 20

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #19)
> Comment on attachment 8337816 [details] [diff] [review]
> D3D11 backend for the new textures
> 
> Review of attachment 8337816 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: gfx/layers/d3d11/CompositorD3D11.cpp
> @@ +583,5 @@
> >        SetSamplerForFilter(FILTER_LINEAR);
> >  
> >        mVSConstants.textureCoords = ycbcrEffect->mTextureCoords;
> >  
> > +      const int Y = 0, Cb = 1, Cr = 2;
> 
> use the same constants I asked you to extract for the other bug

Yeah, what's a good name for those constants? Note that in the other bug you are mentioning using the same indices on the client and host sides, but those constants are only for the host side: on the client and ipc side, the planes are packed together in one buffer. Then the BufferTextureHost creates 3 texture sources out of it, and put them in the Y>Cb>Cr order, naturally. before we had 0, 1, 2 directly in the compositor code, I thought reading Y, Cb, Cr would help readability but there isn't really a mistake to make here. Those constants will only be used in DrawQuad.

> ::: gfx/layers/d3d11/TextureD3D11.cpp
> @@ +72,5 @@
> >    MOZ_COUNT_DTOR(DataTextureSourceD3D11);
> >  }
> >  
> >  
> > +template<typename T> // ID3D10Texture2D or ID3D11Texture2D
> 
> Why are these generic? We never want to use d3d10 textures afaik

I don't know why that is, but there's always been a lot of D3D10 code in TexturesD3D11.cpp.

> > +DataTextureSourceD3D11::DataTextureSourceD3D11(ID3D11Texture2D* aTexture,
> 
> is this in the wrong patch or just the wrong place?
> 

> @@ +707,5 @@
> >  
> > +DeprecatedTextureHostYCbCrD3D11::DeprecatedTextureHostYCbCrD3D11()
> > +  : mCompositor(nullptr)
> > +{
> > +  mFormat = gfx::FORMAT_YUV;
> 
> remove gfx:: namespace, and please check the rest of the changes for this nit

Sure, but what's wrong with writing gfx:: namespaces in general? with unified builds, we are increasing the chances of name collisions with external libraries, especially for gfx stuff like Size/Point/FORMAT.

> 
> @@ +715,5 @@
> > +{
> > +}
> > +
> > +ID3D11Device*
> > +DeprecatedTextureHostYCbCrD3D11::GetDevice()
> 
> Why do we sometimes use mDevice and sometimes use GetDevice()? Is it more
> safe in some cases?

I think it's random. Just shows that I didn't write all of the code in the same day.

Comment 21

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #20)
> (In reply to Nick Cameron [:nrc] from comment #19)
> > Comment on attachment 8337816 [details] [diff] [review]
> > D3D11 backend for the new textures
> > 
> > Review of attachment 8337816 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: gfx/layers/d3d11/CompositorD3D11.cpp
> > @@ +583,5 @@
> > >        SetSamplerForFilter(FILTER_LINEAR);
> > >  
> > >        mVSConstants.textureCoords = ycbcrEffect->mTextureCoords;
> > >  
> > > +      const int Y = 0, Cb = 1, Cr = 2;
> > 
> > use the same constants I asked you to extract for the other bug
> 
> Yeah, what's a good name for those constants? Note that in the other bug you
> are mentioning using the same indices on the client and host sides, but
> those constants are only for the host side: on the client and ipc side, the
> planes are packed together in one buffer. Then the BufferTextureHost creates
> 3 texture sources out of it, and put them in the Y>Cb>Cr order, naturally.
> before we had 0, 1, 2 directly in the compositor code, I thought reading Y,
> Cb, Cr would help readability but there isn't really a mistake to make here.
> Those constants will only be used in DrawQuad.
> 

Oh, I didn't realise we don't use them on the client side. I guess we can still make a mistake of using different numbers in DrawQuad vs texture upload or wherever the other use was, right? If you literally only use them in one place, then I wouldn't even use the local variables. If you use them in more than one place, even if it is always on the compositor, then I prefer to extract the variables to a shared header - it is an easy way to eliminate an annoying error, even if it is pretty unlikely to make that error.

As for names, maybe use a ColorComponent enum with Y, CB, CR values? I guess 'Y' might clash kind of easily. Maybe COMPONENT_Y, etc.

> > ::: gfx/layers/d3d11/TextureD3D11.cpp
> > @@ +72,5 @@
> > >    MOZ_COUNT_DTOR(DataTextureSourceD3D11);
> > >  }
> > >  
> > >  
> > > +template<typename T> // ID3D10Texture2D or ID3D11Texture2D
> > 
> > Why are these generic? We never want to use d3d10 textures afaik
> 
> I don't know why that is, but there's always been a lot of D3D10 code in
> TexturesD3D11.cpp.
> 

I couldn't see any use of ID3D10Texture2D and I don't see d3d10.h included. I hope there isn't d3d10 code in there. I think you can probably do without the generics here.

> > > +DataTextureSourceD3D11::DataTextureSourceD3D11(ID3D11Texture2D* aTexture,
> > 
> > is this in the wrong patch or just the wrong place?
> > 
> 
> > @@ +707,5 @@
> > >  
> > > +DeprecatedTextureHostYCbCrD3D11::DeprecatedTextureHostYCbCrD3D11()
> > > +  : mCompositor(nullptr)
> > > +{
> > > +  mFormat = gfx::FORMAT_YUV;
> > 
> > remove gfx:: namespace, and please check the rest of the changes for this nit
> 
> Sure, but what's wrong with writing gfx:: namespaces in general? with
> unified builds, we are increasing the chances of name collisions with
> external libraries, especially for gfx stuff like Size/Point/FORMAT.
> 

Its just a style thing - being consistent with the rest of Gecko and having less clutter in the code. If we have namespace clashes, we should fix that - we should prefix the external libs' types, not ours.

> > 
> > @@ +715,5 @@
> > > +{
> > > +}
> > > +
> > > +ID3D11Device*
> > > +DeprecatedTextureHostYCbCrD3D11::GetDevice()
> > 
> > Why do we sometimes use mDevice and sometimes use GetDevice()? Is it more
> > safe in some cases?
> 
> I think it's random. Just shows that I didn't write all of the code in the
> same day.

:-) I think you should pick one way of doing it and stick to it.

Comment 22

[Nicolas Silva [:nical]]

This last try push looks good: https://tbpl.mozilla.org/?tree=Try&rev=e73e6821d370
bc all orange but a windows OMTC try push without my patch queue looks the same https://tbpl.mozilla.org/?tree=Try&rev=e73e6821d370

I'll look at the last nits and upload patches today.

Comment 23

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #21)
> I couldn't see any use of ID3D10Texture2D and I don't see d3d10.h included.
> I hope there isn't d3d10 code in there. I think you can probably do without
> the generics here.
> 

If you grep for "D3D10" in TextureD3D11.h/.cpp you'll see it's all over the place. That's what the TextureClient (deprecated and new) is using. It was there before the new textures and I don't know why so if it is an issue it should be fixed in a separate bug.

Comment 24

[Nicolas Silva [:nical]]

Comment on attachment 832367 [details] [diff] [review]
Fallback to cairo if creating a DrawTarget fails in BufferTextureClient::UpdateSurface

Bas is on PTO for a while and I would have preferred to convince him that this patch is the way to go but I want get this stuff landed. Nick, what do you think?

Bas's r- was because he wants this fallback to be hidden in gfxPlatform. He also wants the fallback to respect the backend list in the prefs.

Matt (and I) think it is best to not hide the fallback path because by doing so we risk to make it easier to mix azure backends which is not supported. furthermore, if we respect the backend list, someone might make a backend list that cant CreateDrawTargetForData and basically break compositing (at this point you are probably forced to clearthe profile and build a new one).

Comment 25

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #23)
> (In reply to Nick Cameron [:nrc] from comment #21)
> > I couldn't see any use of ID3D10Texture2D and I don't see d3d10.h included.
> > I hope there isn't d3d10 code in there. I think you can probably do without
> > the generics here.
> > 
> 
> If you grep for "D3D10" in TextureD3D11.h/.cpp you'll see it's all over the
> place. That's what the TextureClient (deprecated and new) is using. It was
> there before the new textures and I don't know why so if it is an issue it
> should be fixed in a separate bug.

You are quite right. Sorry for the misunderstanding. As far as I can see, it doesn't look like a mistake, so I don't think anything needs to change here or in a followup.

Comment 26

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #24)
> Comment on attachment 832367 [details] [diff] [review]
> Fallback to cairo if creating a DrawTarget fails in
> BufferTextureClient::UpdateSurface
> 
> Bas is on PTO for a while and I would have preferred to convince him that
> this patch is the way to go but I want get this stuff landed. Nick, what do
> you think?
> 
> Bas's r- was because he wants this fallback to be hidden in gfxPlatform. He
> also wants the fallback to respect the backend list in the prefs.
> 
> Matt (and I) think it is best to not hide the fallback path because by doing
> so we risk to make it easier to mix azure backends which is not supported.
> furthermore, if we respect the backend list, someone might make a backend
> list that cant CreateDrawTargetForData and basically break compositing (at
> this point you are probably forced to clearthe profile and build a new one).

I agree with Bas that it should be in gfxPlatform and that ideally we should respect the prefs list. However, I agree with you that we really don't want to have incompatible Azure backends.

So, how about API on gfxPlatform which is something like

DrawTarget GetFallbackSurfaceForContent(BackendType aCompatibleType);

(Changing the name to match the conventions in gfxPlatform and the types so they are correct, etc.)

This would then return a draw target which is compatible with aCompatibleType but isn't aCompatibleType. For now, I think it is OK if this always returns a Cairo DrawTarget. But in the future, it will make it easier to move to Skia as our default fallback or to look at the pref list if we expect more than just Cairo to be compatible. You should probably have a big old comment explaining all of this too. (And is it possible to assert compatibility between DrawTargets ahead of time? Would be nice if we could do that)

Comment 27

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #26)
> (In reply to Nicolas Silva [:nical] from comment #24)
> So, how about API on gfxPlatform which is something like
> 
> DrawTarget GetFallbackSurfaceForContent(BackendType aCompatibleType);
> 
> (Changing the name to match the conventions in gfxPlatform and the types so
> they are correct, etc.)
> 
> This would then return a draw target which is compatible with
> aCompatibleType but isn't aCompatibleType.

I am not sure, but don't think there is any compatibility between backends. DataSourceSurface implementations are backend agnostic but that seems to be all.

> compatibility between DrawTargets ahead of time? Would be nice if we could
> do that)

I don't think we can do that. Maybe Jeff knows?

Comment 28

[Nicolas Silva [:nical]]

Doing this fallback to a backend that can draw in RAM is a very dangerous thing that we are forced to do here because we need to copy into a shmem.
I am a bit worried about exposing this. Where else would we be willing to take that risk (mixing backends)? Hiding something risky like this will make it harder to detect the bug if someone uses that where he should not have: then who's to blame, gfxPlatform for exposing something that only works in very specific conditions or the calling code which just looked at what gfxPlatform provides and found an interface that matched his need (but did not fulfill the specific conditions for it to work)?

My thinking is that this should be considered as a hack and should look like one, and the hackiness should be visible where people use it (that is where the conditions for the thing to work are to be held) rather than abstracted out in a way that make it look like an entirely supported thing.

But that's based on my understanding that backends are completely incompatible with one another, which may not be entirely true.

Comment 29

[Nicolas Silva [:nical]]

The more I think about it the more I tend to not want to fallback at all actually. I don't know for sure that, for example, ContentClient will not try to update the texture client's DrawTarget using an incompatible backend.

We could have TextureClientDrawTarget return a draw target backed by its shared memory if possible and if it fails, create a new DrawTarget.
Then, in Unlock(), if a the DrawTarget was not backed by the shared memory, copy the snapshot of the DrawTarget in shared memory doing dt->Snapshot()->GetDataSurface().

This means that D2D backend would always have an extra copy when using shmem or memory TextureClients, but at least it's bullet-proof.

The best solution would be that all backends support CreateDrawTargetForData.

Comment 30

[Nicolas Silva [:nical]]

Attachment: implement GetAsDrawTarget for BufferTextureClient and TextureClientD3D11

This patch does 2 things:
* Make it possible to get a D2D DrawTarget from a BufferTextureClient without resorting to an unsafe fallback to a cairo DrawTarget.
* Implement the missing parts of TextureClientD3D11

try push: https://tbpl.mozilla.org/?tree=Try&rev=32ceb527c2a6

Comment 31

[Nick Cameron [:nrc]]

Comment on attachment 8343750 [details] [diff] [review]
implement GetAsDrawTarget for BufferTextureClient and TextureClientD3D11

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

Looks good, r=me with the nits fixed

::: gfx/layers/CompositorTypes.h
@@ +273,5 @@
> +const OpenMode OPEN_READ        = 0x1;
> +const OpenMode OPEN_WRITE       = 0x2;
> +const OpenMode OPEN_READ_WRITE  = OPEN_READ|OPEN_WRITE;
> +const OpenMode OPEN_READ_ONLY   = OPEN_READ;
> +const OpenMode OPEN_WRITE_ONLY  = OPEN_WRITE;

these aliases seem non-ideal because they don't in fact enforce the 'only'-ness

::: gfx/layers/client/CompositableClient.cpp
@@ +224,5 @@
>    RefPtr<TextureClient> result;
>  
>  #ifdef XP_WIN
>    LayersBackend parentBackend = GetForwarder()->GetCompositorBackendType();
>    // XXX[nrc] uncomment once we have new texture clients for windows

please keep this comment up to date - it only applies to d3d9 now, right?

::: gfx/layers/client/TextureClient.cpp
@@ +339,5 @@
> +  if (mDrawTarget) {
> +    return mDrawTarget;
> +  }
> +
> +  bool needsRead = (!mDrawTarget && mOpenMode & OPEN_READ);

!mDrawTarget is always true here due to the above conditional

And actually you don't seem to use this variable anyway

@@ +344,4 @@
>  
>    ImageDataSerializer serializer(GetBuffer());
>    if (!serializer.IsValid()) {
> +    return false;

return nullptr?

@@ +348,3 @@
>    }
>  
> +  mUsingFallbackDrawTarget = false;

assert this instead of setting here (I like having the mDrawTarget == nullptr => mUsingFallbackDrawTarget == false invariant)

@@ +348,4 @@
>    }
>  
> +  mUsingFallbackDrawTarget = false;
> +  mDrawTarget = serializer.GetAsDrawTarget();

if (mDrawTarget = serializer.GetAsDrawTarget()) {
  return mDrawTarget;
}

then avoid the below if

@@ +354,5 @@
> +    // DrawTarget around raw memory. This is going to be slow :(
> +    mDrawTarget = gfxPlatform::GetPlatform()->CreateOffscreenContentDrawTarget(
> +      serializer.GetSize(), serializer.GetFormat());
> +    mUsingFallbackDrawTarget = true;
> +    if (mDrawTarget && mOpenMode & OPEN_READ) {

if (!mDrawTarget) {
  return;
}

before setting mUsingFallbackDrawTarget and then only check the open mode

@@ +392,5 @@
> +    RefPtr<SourceSurface> snapshot = mDrawTarget->Snapshot();
> +    RefPtr<DataSourceSurface> surface = snapshot->GetDataSurface();
> +    ImageDataSerializer serializer(GetBuffer());
> +    if (!serializer.IsValid()) {
> +      return;

should we warn here? Seems like we are loosing a frame of data

also reset mDrawTarget and mUsingFallbackDrawTarget

@@ +401,5 @@
> +             surface->GetData() + i*surface->Stride(),
> +             surface->GetSize().width * bpp);
> +    }
> +  }
> +  mDrawTarget = nullptr;

set mUsingFallbackDrawTarget to false here

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +144,5 @@
> +
> +TextureClientD3D11::TextureClientD3D11(gfx::SurfaceFormat aFormat, TextureFlags aFlags)
> +  : TextureClient(aFlags)
> +  , mFormat(aFormat)
> +  , mIsLocked(false)

Should we initialise mSize?

@@ +148,5 @@
> +  , mIsLocked(false)
> +{}
> +
> +TextureClientD3D11::~TextureClientD3D11()
> +{}

remove the destructor

@@ +163,5 @@
>  
>  void
>  TextureClientD3D11::Unlock()
>  {
> +  // XXX - this should be an assertion

make it so? are we hitting these as it stands?

@@ +210,5 @@
> +bool
> +TextureClientD3D11::ToSurfaceDescriptor(SurfaceDescriptor& aOutDescriptor)
> +{
> +  if (!IsAllocated()) {
> +    return false;

warn here

::: gfx/layers/d3d11/TextureD3D11.h
@@ +42,5 @@
>    virtual void Unlock() MOZ_OVERRIDE;
>  
>    virtual bool ToSurfaceDescriptor(SurfaceDescriptor& aOutDescriptor) MOZ_OVERRIDE;
> +
> +  virtual gfx::IntSize GetSize() const { return mSize; }

MOZ_OVERRIDE

@@ +44,5 @@
>    virtual bool ToSurfaceDescriptor(SurfaceDescriptor& aOutDescriptor) MOZ_OVERRIDE;
> +
> +  virtual gfx::IntSize GetSize() const { return mSize; }
> +
> +  virtual gfx::SurfaceFormat GetFormat() const { return mFormat; }

MOZ_OVERRIDE and under the TextureClientDrawTarget comment

Comment 32

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #31)
> Comment on attachment 8343750 [details] [diff] [review]
> implement GetAsDrawTarget for BufferTextureClient and TextureClientD3D11
> 
> Review of attachment 8343750 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +348,3 @@
> >    }
> >  
> > +  mUsingFallbackDrawTarget = false;
> 
> assert this instead of setting here (I like having the mDrawTarget ==
> nullptr => mUsingFallbackDrawTarget == false invariant)

This is not true: mDrawTarget is equal to whatever draw target we created be it the fallback or one that actually wraps the TextureClient's memory.

> 
> @@ +348,4 @@
> >    }
> >  
> > +  mUsingFallbackDrawTarget = false;
> > +  mDrawTarget = serializer.GetAsDrawTarget();
> 
> if (mDrawTarget = serializer.GetAsDrawTarget()) {
>   return mDrawTarget;
> }
> 
> then avoid the below if

I have a grudge against this pattern. Every time I read something like this I think someone introduced a bug by typing "=" instead of "==".

> 
> @@ +401,5 @@
> > +             surface->GetData() + i*surface->Stride(),
> > +             surface->GetSize().width * bpp);
> > +    }
> > +  }
> > +  mDrawTarget = nullptr;
> 
> set mUsingFallbackDrawTarget to false here
> 
> ::: gfx/layers/d3d11/TextureD3D11.cpp
> @@ +144,5 @@
> > +
> > +TextureClientD3D11::TextureClientD3D11(gfx::SurfaceFormat aFormat, TextureFlags aFlags)
> > +  : TextureClient(aFlags)
> > +  , mFormat(aFormat)
> > +  , mIsLocked(false)
> 
> Should we initialise mSize?

the default constructor for sizes intializes the fields to zero. We could do it explicitly here but it would not really make a difference.

> 
> @@ +148,5 @@
> > +  , mIsLocked(false)
> > +{}
> > +
> > +TextureClientD3D11::~TextureClientD3D11()
> > +{}
> 
> remove the destructor

This is what lets us have a RefPtr<DrawTarget> with a forward reference in the header instead of #including 2D.h

> 
> @@ +163,5 @@
> >  
> >  void
> >  TextureClientD3D11::Unlock()
> >  {
> > +  // XXX - this should be an assertion
> 
> make it so? are we hitting these as it stands?
> 

At the moment yes with the new ContentClient (I am still toying around getting the TextureClientD3D11 to work there)

Comment 33

[Nicolas Silva [:nical]]

The last try push was crashing in CompositorD3D9::DrawQuad when one or several YCbCr planes failed to upload (texture too big). new try push with a fix https://tbpl.mozilla.org/?tree=Try&rev=fa11acd5b500

Comment 34

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #32)
> (In reply to Nick Cameron [:nrc] from comment #31)
> > Comment on attachment 8343750 [details] [diff] [review]
> > implement GetAsDrawTarget for BufferTextureClient and TextureClientD3D11
> > 
> > Review of attachment 8343750 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > @@ +348,3 @@
> > >    }
> > >  
> > > +  mUsingFallbackDrawTarget = false;
> > 
> > assert this instead of setting here (I like having the mDrawTarget ==
> > nullptr => mUsingFallbackDrawTarget == false invariant)
> 
> This is not true: mDrawTarget is equal to whatever draw target we created be
> it the fallback or one that actually wraps the TextureClient's memory.
> 

Sorry, you misunderstand me, I think. I meant the invariant should be that if mDrawTarget is null, then mUsing should be false. If mDrawTarget is non-null, then mUsing should be true or false - reflecting whether mDrawTarget is fallback or not. The only difference from now is when mDrawTarget is null, the value of mUsing is undefined (well, reflects what the now unreferenced draw target was).

> > 
> > @@ +348,4 @@
> > >    }
> > >  
> > > +  mUsingFallbackDrawTarget = false;
> > > +  mDrawTarget = serializer.GetAsDrawTarget();
> > 
> > if (mDrawTarget = serializer.GetAsDrawTarget()) {
> >   return mDrawTarget;
> > }
> > 
> > then avoid the below if
> 
> I have a grudge against this pattern. Every time I read something like this
> I think someone introduced a bug by typing "=" instead of "==".
> 

Fair enough, I know some people don't like. You can probably still make the code clearer by shuffling the 'if's, even without moving the assignment into the if clause.

> > 
> > @@ +401,5 @@
> > > +             surface->GetData() + i*surface->Stride(),
> > > +             surface->GetSize().width * bpp);
> > > +    }
> > > +  }
> > > +  mDrawTarget = nullptr;
> > 
> > set mUsingFallbackDrawTarget to false here
> > 
> > ::: gfx/layers/d3d11/TextureD3D11.cpp
> > @@ +144,5 @@
> > > +
> > > +TextureClientD3D11::TextureClientD3D11(gfx::SurfaceFormat aFormat, TextureFlags aFlags)
> > > +  : TextureClient(aFlags)
> > > +  , mFormat(aFormat)
> > > +  , mIsLocked(false)
> > 
> > Should we initialise mSize?
> 
> the default constructor for sizes intializes the fields to zero. We could do
> it explicitly here but it would not really make a difference.
> 

Oh, that is good. I didn't know that. Ignore this comment then :-)
> > 
> > @@ +148,5 @@
> > > +  , mIsLocked(false)
> > > +{}
> > > +
> > > +TextureClientD3D11::~TextureClientD3D11()
> > > +{}
> > 
> > remove the destructor
> 
> This is what lets us have a RefPtr<DrawTarget> with a forward reference in
> the header instead of #including 2D.h
> 

Oh, ok, fair enough.

> > 
> > @@ +163,5 @@
> > >  
> > >  void
> > >  TextureClientD3D11::Unlock()
> > >  {
> > > +  // XXX - this should be an assertion
> > 
> > make it so? are we hitting these as it stands?
> > 
> 
> At the moment yes with the new ContentClient (I am still toying around
> getting the TextureClientD3D11 to work there)

OK, could you put that in the comment please?

Comment 35

[Matt Woodrow (:mattwoodrow)]

Tried these patches out and found a couple of bugs:

DataTextureSourceD3D11::Update() needs to return true at the end of the function.

DataTextureSourceD3D11 can take an existing texture in it's constructor, and this doesn't provide a value for mCompositor. This happens when we have a DXGITextureHostD3D11.
DataTextureSourceD3D11::GetTileRect then tries to read mCompositor and crashes.

Calling SetCompositor doesn't work, because it clears the texture. Locally I fixed this by adding a CompositorD3D11* parameter to the constructor, and having DXGITextureHostD3D11 store one of these and pass it in.

Comment 36

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix some bugs

Comment 37

[Nicolas Silva [:nical]]

try push with a mix of D3D11/9 textures patches after a (painful) rebase https://tbpl.mozilla.org/?tree=Try&rev=6e954b238b4b

I'll upload the patches soon

Comment 38

[Nicolas Silva [:nical]]

try push with build fixes https://tbpl.mozilla.org/?tree=Try&rev=2b94f963dc69

Comment 39

[Nicolas Silva [:nical]]

try push with omtc on (windows only) https://tbpl.mozilla.org/?tree=Try&rev=88a89f7366b8

Comment 40

[Matt Woodrow (:mattwoodrow)]

Can this land now? I'd like to get bug 904890 landed this week.

Comment 41

[Nicolas Silva [:nical]]

Attachment: Some fixes + make new d3d11 texture work for mask layers.

Comment 42

[Nick Cameron [:nrc]]

Comment on attachment 8350224 [details] [diff] [review]
Some fixes + make new d3d11 texture work for mask layers.

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

lgtm

::: gfx/layers/client/ImageClient.cpp
@@ +254,5 @@
> +    {
> +      // We must not keep a reference to the DrawTarget after it has been unlocked.
> +      RefPtr<DrawTarget> dt = mFrontBuffer->AsTextureClientDrawTarget()->GetAsDrawTarget();
> +      RefPtr<SourceSurface> source = gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(dt, surface);
> +      MOZ_ASSERT(source.get());

nit: don't need the get() (I hope)

@@ +255,5 @@
> +      // We must not keep a reference to the DrawTarget after it has been unlocked.
> +      RefPtr<DrawTarget> dt = mFrontBuffer->AsTextureClientDrawTarget()->GetAsDrawTarget();
> +      RefPtr<SourceSurface> source = gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(dt, surface);
> +      MOZ_ASSERT(source.get());
> +      dt->CopySurface(source, IntRect(IntPoint(), source->GetSize()), IntPoint());

I wonder if it is worth making this whole block a method on TextureClientDrawTarget? I don't feel too strongly about it though

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +458,5 @@
> +  default:
> +    return FORMAT_B8G8R8A8;
> +  }
> +}
> +

Use gfxPlatform::Optimal2DFormatForContent instead of this

Comment 43

[Nicolas Silva [:nical]]

a more recent try push https://tbpl.mozilla.org/?tree=Try&rev=72aee321fbd4

Comment 44

[Nicolas Silva [:nical]]

err, had to do a small fix and here is a new try push https://tbpl.mozilla.org/?tree=Try&rev=92af08bc9d27

Comment 45

[Nicolas Silva [:nical]]

had another painful rebase so here is another try push, https://tbpl.mozilla.org/?tree=Try&rev=831c0d7261ca

Comment 46

[Nicolas Silva [:nical]]

Of course that last try push was rebased on top of a busted revision of inbound.
Another one: https://tbpl.mozilla.org/?tree=Try&rev=6de22a9d81a7

Comment 47

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/033f40f8414d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f429da79b7e3

Comment 48

[Nicolas Silva [:nical]]

Fix for the mac test failure (caused by the loss two lines in my last rebase): https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ff328828dfda

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f429da79b7e3
https://hg.mozilla.org/mozilla-central/rev/033f40f8414d
https://hg.mozilla.org/mozilla-central/rev/ff328828dfda