From https://bugzilla.mozilla.org/show_bug.cgi?id=1298058#c9 , it's good to have a test to prevent the usage of using basicCompositor with d3d11/d3d9TextureHost. We already have a gtests[1] for some compositor functionalities. Maybe I could create a new one here. [1] https://dxr.mozilla.org/mozilla-central/rev/dbe4b47941c7b3d6298a0ead5e40dd828096c808/gfx/tests/gtest/TestCompositor.cpp

Jerry, I think this is a good practice for Kevin to work on.

This is the current WIP patch, I will check if there is any errors in the code and upload the next version.

Hi Jerry, Here are some prepared works for adding the new gtest. In this patch, I do some modifications to the original tests to avoid some file unified errors and move some classes out from original file so that I can reuse it in the following patches. Can you take a look to this patch and see if the modifications are proper ?

Hi Jerry, This is the main flow of the gtest. Could you help me to check if this flow is correct ?

Hi Jerry, In this patch, I implement the variant TextureClient-creating functions. And for some TextureDatas, like DXGIYCbCrTextureData, I do a lot of extra works to create it, like initialize IDirect3DDevice9 or binding the surfaces, I wonder if there an easier way to do it. Could you help me to check if I miss something or are there other type of TextureClients I should cover in this test ?

Comment on attachment 8800044 [details] [diff] [review] (Part 1) Some prepared works for adding the new gtest Review of attachment 8800044 [details] [diff] [review]: ----------------------------------------------------------------- Please show more details for what you want to do in this patch (e.g. make mockWidget from TestCompositor.cpp into separated file). ::: gfx/tests/gtest/TestJobScheduler.cpp @@ +19,5 @@ > namespace test_scheduler { > > using namespace mozilla::gfx; > using namespace mozilla; > +using mozilla::gfx::SyncObject; Why do we need to update this file?

Comment on attachment 8800045 [details] [diff] [review] (Part 2) Add a gtest to verify the behavior of applying incompatible compositor to a TextureHost. Review of attachment 8800045 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/tests/gtest/TestDriverReset.cpp @@ +23,5 @@ > +void > +CreateTextureWithBackend(LayersBackend& aLayersBackend, > + nsTArray<RefPtr<TextureHost>>& aTextures) > +{ > + nsTArray<RefPtr<TextureClient>> textureClients; Is the textureClient's textureData still valid when we go out of this scope? @@ +54,5 @@ > + > +/** > + * This function will return a BasicCompositor to caller. > + */ > +Compositor* return already_AddRefed<Compositor> @@ +68,5 @@ > + return nullptr; > +} > + > +/** > + * This function checks if given compositor is compatible with the textures. update the comment as: this function check if the textures are compatible with given compositor. @@ +92,5 @@ > + } > + } > +} > + > +TEST(Gfx, DriverResetTest) This testing is used for checking the behavior of applying incompatible textureHost to the compositor. Please update the file name and this test name.

Comment on attachment 8800073 [details] [diff] [review] (Part 3) Implement the TextureClient creating functions for the gtest. Review of attachment 8800073 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/tests/gtest/TextureHelper.h @@ +52,5 @@ > +/** > + * This function create texture from IDirect3DDevice9. > + */ > +static already_AddRefed<IDirect3DTexture9> > +InitTextures(IDirect3DDevice9* aDevice, const IntSize& aSize, How about extract InitTexture(), FinishTextures() and other functions from TextureD3D9.cpp to a helper .cpp file. The functions in TextureHelper.h are the same as the functions in TextureD3D9.cpp. @@ +251,5 @@ > CreateYCbCrTextureClientWithBackend(LayersBackend aLayersBackend) > { > + > + TextureData* data = nullptr; > + RefPtr<ID3D11Device> device = DeviceManagerDx::Get()->GetContentDevice(); Put these lines into XP_WIN def section.

Hello Matt, can you help me to review the patch? In this patch, I move MockWidget from TestCompositor for code reusing and also fix some ambiguous error caused by the file unified.

Hello Jerry, Can you help me to review this patch ? This patch implement the main frame of the gtest, it try to create TextureHost and fit it into the Compositor.

Comment on attachment 8830206 [details] [diff] [review] (Part 1) Some prepared works for adding the new gtest. Forward the review to Jerry.

Hi Jerry, In this patch, I implement the variant TextureClient-creating functions. And I expose some functions from IMFYCbCrImage for TCbCrTextureData according to comment 8. Could you help me to review it? Thank you.

Comment on attachment 8830211 [details] [diff] [review] (Part 2) Add a gtest to verify the behavior of applying incompatible compositor to a TextureHost. Review of attachment 8830211 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/TextureClient.h @@ +732,5 @@ > > friend class TextureChild; > friend void TestTextureClientSurface(TextureClient*, gfxImageSurface*); > friend void TestTextureClientYCbCr(TextureClient*, PlanarYCbCrData&); > + friend already_AddRefed<TextureHost> TestCreateTextureHost(TextureClient*, Please export the function ToSurfaceDescriptor in TextureClient and make this TestCreateTextureHost out of TextureClient. ::: gfx/tests/gtest/TestTextureCompatibility.cpp @@ +65,5 @@ > +/** > + * This function will return a BasicCompositor to caller. > + */ > +already_AddRefed<Compositor> > +CreateTestCompositor() how about "CreateBasicCompositor" instead? @@ +92,5 @@ > + if (!aTextures[i]) { > + continue; > + } > + aTextures[i]->SetCompositor(aCompositor); > + bool lockResult = aTextures[i]->Lock(); Please make some comments here like: "The lock will be failed if the texture client is not compatible with given compositor." ::: gfx/tests/gtest/TextureHelper.h @@ +53,5 @@ > + RefPtr<TextureHost> textureHost; > + > + aClient->ToSurfaceDescriptor(descriptor); > + > + if (aLayersBackend == LayersBackend::LAYERS_BASIC) { What's the reason to use "CreateBackendIndependentTextureHost()" for LAYERS_BASIC backend? Could we just use "TextureHost::Create"?

Comment on attachment 8830212 [details] [diff] [review] (Part 3) Implement the TextureClient creating functions for the gtest. Review of attachment 8830212 [details] [diff] [review]: ----------------------------------------------------------------- I change my mind in comment 8. I think we don't need to move the d3d related code into textureHelper.h. Let's make the "CreateTextureClientWithBackend" and "CreateYCbCrTextureClientWithBackend" back into TextureClient. The textureClient.cpp is already including the d3d related header. Then we don't need to have too much platform dependent code in TextureHelper.h.

Comment on attachment 8830212 [details] [diff] [review] (Part 3) Implement the TextureClient creating functions for the gtest. Review of attachment 8830212 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, the TextureHelper is only used for gtest. So, skip my comment 14. ::: gfx/layers/IMFYCbCrImage.h @@ +13,5 @@ > > namespace mozilla { > namespace layers { > > +struct AutoLockTexture Why do we need to move this struct to IMFYCbCrImage.h. Only IMFYCbCrImage.cpp uses that. @@ +62,5 @@ > protected: > > TextureClient* GetD3D9TextureClient(KnowsCompositor* aForwarder); > + TextureClient* GetD3D11TextureClient(KnowsCompositor* aForwarder); > + static bool UploadData(IDirect3DDevice9* aDevice, I don't see the changes for UploadData() to make it becomes a member function of this class.

Carry r+.

Hello Jerry, Could you help me to review it ? >> friend class TextureChild; >> friend void TestTextureClientSurface(TextureClient*, gfxImageSurface*); >> friend void TestTextureClientYCbCr(TextureClient*, PlanarYCbCrData&); >> + friend already_AddRefed<TextureHost> TestCreateTextureHost(TextureClient*, >Please export the function ToSurfaceDescriptor in TextureClient and make this TestCreateTextureHost out of TextureClient. Keep this modify according to our discussion. >> + RefPtr<TextureHost> textureHost; >> + >> + aClient->ToSurfaceDescriptor(descriptor); >> + >> + if (aLayersBackend == LayersBackend::LAYERS_BASIC) { >What's the reason to use "CreateBackendIndependentTextureHost()" for >LAYERS_BASIC backend? Could we just use "TextureHost::Create"? The original reason of using CreateBackendIndependentTextureHost is to debug, I've removed this condition in this version.

Hello Jerry, I've fixed the feedback in comment 15, could you help me to review it? Thank you.

Comment on attachment 8834315 [details] [diff] [review] (Part 2) Add a gtest to verify the behavior of applying incompatible compositor to a TextureHost. Review of attachment 8834315 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/tests/gtest/TestTextureCompatibility.cpp @@ +28,5 @@ > +using mozilla::widget::CompositorWidget; > +using mozilla::widget::InProcessCompositorWidget; > + > +/** > + * This function will create possible TextureHosts according to the given backend. s/create/create the/ @@ +33,5 @@ > + */ > +void > +CreateTextureWithBackend(LayersBackend& aLayersBackend, > + nsTArray<RefPtr<TextureClient>>& aTextureClients, > + nsTArray<RefPtr<TextureHost>>& aTextures) s/aTextures/aTextureHosts/ @@ +83,5 @@ > +/** > + * This function checks if the textures are compatible with given compositor.. > + */ > +void > +VerifyTextures(LayersBackend aBackends, The expected lock results in this function are tested with basicCompositor. Please rename this function and its comment. @@ +93,5 @@ > + continue; > + } > + aTextures[i]->SetCompositor(aCompositor); > + > + // The lock will be failed if the texture client is not compatible with s/texture client/texture/ ::: gfx/tests/gtest/TextureHelper.h @@ +38,5 @@ > + return nullptr; > +} > + > +/** > + * This function create TextureHost according to given TextureClients. s/create/creates a/ s/to given/to the given/

Comment on attachment 8834316 [details] [diff] [review] (Part 3) Implement the TextureClient creating functions for the gtest. Review of attachment 8834316 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/tests/gtest/TextureHelper.h @@ +125,5 @@ > + data = IMFYCbCrImage::GetD3D11TextureData(clientData, IntSize(200, 150)); > +#endif > + > + if (data) { > + return MakeAndAddRef<TextureClient>(data, TextureFlags::DEALLOCATE_CLIENT, Please share the textureClient creation code. Just like the if conditions in CreateTextureClientWithBackend() @@ +154,5 @@ > + > +#ifdef XP_WIN > + > + // Create DXGITextureData. > + if (aLayersBackend == LayersBackend::LAYERS_D3D11 && These if conditions are exclusive. Use "if" and "else if" here.

Hello Jerry, I've modified the patch according to comment 19, please help me to review it again, thank you.

Hello Jerry, I've modified the patch according to comment 20. Could you help me to review it again ? Thank you.

Comment on attachment 8835935 [details] [diff] [review] (Part 2) Add a gtest to verify the behavior of applying incompatible compositor to a TextureHost_v4 Review of attachment 8835935 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/tests/gtest/TestTextureCompatibility.cpp @@ +28,5 @@ > +using mozilla::widget::CompositorWidget; > +using mozilla::widget::InProcessCompositorWidget; > + > +/** > + * This function will create the possible TextureHosts according to the given backend. s/create the possible TextureHosts/create the possible TextureClient and TextureHost pairs/

Comment on attachment 8835935 [details] [diff] [review] (Part 2) Add a gtest to verify the behavior of applying incompatible compositor to a TextureHost_v4 Review of attachment 8835935 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/tests/gtest/TextureHelper.h @@ +38,5 @@ > + return nullptr; > +} > + > +/** > + * Create a TextureHost according to the given TextureClients. s/TextureClients/TextureClient/

Carry r+ from comment 23.

The try result looks good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ea053ef3d45696f5d7db8469865d10343f45423&selectedJob=77070742

Carry r+, fix the conflict caused by Part 2 patch's fifth version.

Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3710884f415d (Part 1) Refactor some tests so that the new test can reuse the sources. r=jerry https://hg.mozilla.org/integration/mozilla-inbound/rev/50d8857f978e (Part 2) Add a gtest to verify the behavior of applying incompatible compositor to a TextureHost. r=jerry https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8ed553e230 (Part 3) Implement the TextureClient creating functions for the gtest. r=jerry

https://hg.mozilla.org/mozilla-central/rev/3710884f415d https://hg.mozilla.org/mozilla-central/rev/50d8857f978e https://hg.mozilla.org/mozilla-central/rev/3f8ed553e230

No plan to have dot release for 51. Mark 51 won't fix.

Is this worth uplifting to 53?

From my perspective, we don't need to uplift this since it do not change the behavior of the program.