Closed Bug 919936 Opened 6 years ago Closed 4 years ago

Add texture client/host tests for serialization and deserialization

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: nical, Assigned: nical)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Let's add some automated testing for all the serialization/deserialization logic.
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.
Attachment #809069 - Flags: review?(bgirard)
Whiteboard: [leave open]
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.
Attachment #809069 - Flags: review?(bgirard) → review+
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.
Attached patch Same thing for YCbCr surfaces (obsolete) — Splinter Review
Attachment #812854 - Flags: review?(bgirard)
I have some more gtest stuff coming which reuses some of the helpers so let's move them to their own file.
Attachment #812858 - Flags: review?(bgirard)
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.
Attachment #812854 - Flags: review?(bgirard) → review-
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
Attachment #812858 - Flags: review?(bgirard) → review-
Attachment #812854 - Attachment is obsolete: true
Attachment #815012 - Flags: review?(bgirard)
Attachment #812858 - Attachment is obsolete: true
Attachment #815014 - Flags: review?(bgirard)
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.
Attachment #815012 - Flags: review?(bgirard) → review+
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
Attachment #815014 - Flags: review?(bgirard) → review+
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.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.