919936 - Add texture client/host tests for serialization and deserialization

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

Let's add some automated testing for all the serialization/deserialization logic.

Comment 1

[Nicolas Silva [:nical]]

Attachment: A bit of MemoryTextureClient serialization gtest love

Tests allocating - serializing - deserializing a MemoryTextureClient with several image formats.

The same test could be run on other texture client types but I need to figure out how to allocate shared memory without all the IPDL stuff.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 809069 [details] [diff] [review]
A bit of MemoryTextureClient serialization gtest love

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

Nice!

::: gfx/tests/gtest/TestTextures.cpp
@@ +1,1 @@
> +#include "gtest/gtest.h"

License public domain. See TestLayers.cpp

@@ +120,5 @@
> +    gfxASurface::ImageFormatRGB24,
> +    gfxASurface::ImageFormatA8,
> +  };
> +
> +  for (int f = 0; f<3; ++f) {

style f < 3

@@ +121,5 @@
> +    gfxASurface::ImageFormatA8,
> +  };
> +
> +  for (int f = 0; f<3; ++f) {
> +    RefPtr<gfxImageSurface> surface = new gfxImageSurface(gfxIntSize(800,600), formats[f]);

Do we need to test for such a large size? You filling and reading each pixels. We should get the same coverage by testing a much smaller surface.

Comment 3

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab5af752fa81

Comment 4

[Nicolas Silva [:nical]]

build busted, backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/c1f06e0edcf5

Comment 5

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e56e8fbacb7c

Comment 6

[Nicolas Silva [:nical]]

Adding the same test for YCbCr surfaces is relatively straightforward, except that right now textureHost::GetAsSurface() will return a surface containing only the Y plane.
GetAsSurface() is only intended for debugging puposes (and now it is also used by the gtest stuff), should I fix it to return an actual RGB surface with YUV->RGB convertion, or should I just consider that testing the equality between the client and host Y planes is sufficient?

Otherwise I can also add a bit of API in TextureHost to retrieve the data in the PlanarYCbCrImage::Data form (which would only ever be used by gtests). It would be a bit ugly because TextureHost doesn't any expose any YCbCr-specific API.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e56e8fbacb7c

Comment 8

[Nicolas Silva [:nical]]

Attachment: Same thing for YCbCr surfaces

Comment 9

[Nicolas Silva [:nical]]

Attachment: Regroup some common gtest texture helper functions

I have some more gtest stuff coming which reuses some of the helpers so let's move them to their own file.

Comment 10

[Benoit Girard (:BenWa)]

Comment on attachment 812854 [details] [diff] [review]
Same thing for YCbCr surfaces

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

I'm ok with the test but would like to avoid shipping test code. Tentative r- unless you can convince me this is ok/worth it.

::: gfx/layers/composite/TextureHost.cpp
@@ +456,5 @@
>    return true;
>  }
>  
> +bool
> +BufferTextureHost::GetYCbCrData(PlanarYCbCrData& aData)

Do you plan on using this method later on in release code? Otherwise maybe you can put this code in TestTexture.cpp in a new class and make it a friend. If that's not possible we can put everything under #ifdef DEBUG and have the test only run in debug builds.

::: gfx/layers/composite/TextureHost.h
@@ +452,5 @@
>    virtual gfx::IntSize GetSize() const MOZ_OVERRIDE { return mSize; }
>  
>    virtual already_AddRefed<gfxImageSurface> GetAsSurface() MOZ_OVERRIDE;
>  
> +  virtual bool GetYCbCrData(PlanarYCbCrData& aOutData) MOZ_OVERRIDE;

Also this adds a vtable entry.

Comment 11

[Benoit Girard (:BenWa)]

Comment on attachment 812858 [details] [diff] [review]
Regroup some common gtest texture helper functions

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

You forgot to hg add LayersTestUtils.h

Comment 12

[Nicolas Silva [:nical]]

Attachment: Same thing for YCbCr surfaces (v2)

Comment 13

[Nicolas Silva [:nical]]

Attachment: Regroup some common gtest texture helper functions (v2)

Comment 14

[Benoit Girard (:BenWa)]

Comment on attachment 815012 [details] [diff] [review]
Same thing for YCbCr surfaces (v2)

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

::: gfx/layers/composite/TextureHost.cpp
@@ +17,5 @@
>  #include "mozilla/layers/YCbCrImageDataSerializer.h"
>  #include "nsAString.h"
>  #include "nsAutoPtr.h"                  // for nsRefPtr
>  #include "nsPrintfCString.h"            // for nsPrintfCString
> +#include "ImageContainer.h"

Shouldn't be required here.

::: gfx/layers/composite/TextureHost.h
@@ +47,5 @@
>  class TextureSourceD3D11;
>  class TextureSourceBasic;
>  class TextureParent;
>  class DataTextureSource;
> +class PlanarYCbCrData;

Shouldn't be required here.

Comment 15

[Benoit Girard (:BenWa)]

Comment on attachment 815014 [details] [diff] [review]
Regroup some common gtest texture helper functions (v2)

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

::: gfx/tests/gtest/LayerTestUtils.h
@@ +22,5 @@
> +class Compositor;
> +
> +Compositor* CreateCompositor(LayersBackend aBakend, nsIWidget* aWidget);
> +
> +/// fills the surface with values betwee 0 and 100.

between* here and bellow

Comment 16

[Nicolas Silva [:nical]]

fixed Ben's last comments and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ce7c4424b8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f879cfc861f

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And backed out for breaking Windows builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/257a97e05777

Comment 18

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/048984fc3cd1

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/048984fc3cd1

Comment 20

[Nicolas Silva [:nical]]

No activity here for a long while and I don't plan to work on this so let's consider the stuff that's already landed is enough.