1105834 - Add options to dump layers content with layers dump

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: WIP

I want to separate layers.dump & layers.dump.texture into two separate options. It should allow us to just dump the layers tree for debug or also include the texture if there's correctness issues.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Screenshot

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add CreateDataSourceSurfaceByCloning to moz2d

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 8530411 [details] [diff] [review]
Part 1: Add CreateDataSourceSurfaceByCloning to moz2d

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

DataSurfaceHelpers.h/.cpp looks like a good place to put CopyRect into.

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8530411 [details] [diff] [review]
Part 1: Add CreateDataSourceSurfaceByCloning to moz2d

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

Hrm, I'm not sure I feel we should expose this on the factory, separate functions in DataSurfaceHelpers.h as Markus has suggested seem like the way to go here.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add layers.dump-texture feature

Comment 6

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add layers.dump-texture feature

Forgot qref. Added:

// TODO We should combine the OnWhite/OnBlack here an just output a single image.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8530424 [details] [diff] [review]
Part 2: Add layers.dump-texture feature

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

::: gfx/layers/LayersLogging.cpp
@@ +312,5 @@
>    // we usually use std::stringstream to build up giant multi-line gobs
>    // of output. So to avoid the truncation we find the newlines and
>    // print the lines individually.
> +  std::string line;
> +  while (std::getline(aStr, line, '\n')) {

'\n' is the default value of that argument, you can leave it out

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +383,5 @@
>    return LayerRenderState();
>  }
>  
>  TemporaryRef<gfx::DataSourceSurface>
>  GrallocTextureHostOGL::GetAsSurface() {

Is GetAsSurface() only used for debugging and never in production?

::: gfx/thebes/gfxUtils.cpp
@@ +1203,5 @@
>      // Update the length of the vector without overwriting the new data.
>      imgData.growByUninitialized(numReadThisTime);
>  
>      imgSize += numReadThisTime;
> +    imgData.growByUninitialized(numReadThisTime);

nope

Comment 8

[Benoit Girard (:BenWa)]

(In reply to Markus Stange [:mstange] from comment #7)
> ::: gfx/layers/opengl/GrallocTextureHost.cpp
> @@ +383,5 @@
> >    return LayerRenderState();
> >  }
> >  
> >  TemporaryRef<gfx::DataSourceSurface>
> >  GrallocTextureHostOGL::GetAsSurface() {
> 
> Is GetAsSurface() only used for debugging and never in production?
> 

I might be for screenshoting and things like that.

Comment 9

[Nicolas Silva [:nical]]

(In reply to Benoit Girard (:BenWa) from comment #8)
> (In reply to Markus Stange [:mstange] from comment #7)
> > Is GetAsSurface() only used for debugging and never in production?
> > 
> 
> I might be for screenshoting and things like that.

As far as I know it is only used for debugging, and some TextureHost don't even implement it. But it

Comment 10

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add CreateDataSourceSurfaceByCloning to moz2d

Comment 11

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add layers.dump-texture feature. r=mstange

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8530442 [details] [diff] [review]
Part 1: Add CreateDataSourceSurfaceByCloning to moz2d

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

::: gfx/2d/Rect.h
@@ +92,5 @@
>          return IntRectTyped<UnknownUnits>(this->x, this->y, this->width, this->height);
>      }
> +
> +    bool
> +    IntRectOverflows() const {

Could just be called Overflows(), and doesn't need a line break after "bool"

Comment 13

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add CreateDataSourceSurfaceByCloning to moz2d

Comment 14

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=73036cca401c

Comment 15

[Benoit Girard (:BenWa)]

Attachment: Part 3: Add lz4 support

You're right. This is -way- faster.

Comment 16

[Bas Schouten (:bas.schouten)]

Comment on attachment 8530448 [details] [diff] [review]
Part 1: Add CreateDataSourceSurfaceByCloning to moz2d

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

I like it.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8530486 [details] [diff] [review]
Part 3: Add lz4 support

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

::: gfx/thebes/gfxUtils.cpp
@@ +1385,5 @@
>  /* static */ nsCString
> +gfxUtils::GetAsLZ4Base64Str(DataSourceSurface* aSourceSurface)
> +{
> +  int32_t datasize = aSourceSurface->GetSize().height * aSourceSurface->Stride();
> +  auto compresseddata = MakeUnique<char[]>(LZ4::maxCompressedSize(datasize));

How come no camel case?

@@ +1386,5 @@
> +gfxUtils::GetAsLZ4Base64Str(DataSourceSurface* aSourceSurface)
> +{
> +  int32_t datasize = aSourceSurface->GetSize().height * aSourceSurface->Stride();
> +  auto compresseddata = MakeUnique<char[]>(LZ4::maxCompressedSize(datasize));
> +  compresseddata.get()[0] = 0;

What is this for?

Comment 18

[Jonathan Watt [:jwatt]]

(In reply to Benoit Girard (:BenWa) from comment #10)
> Part 1: Add CreateDataSourceSurfaceByCloning to moz2d

Will gfxUtils::CopySurfaceToDataSourceSurfaceWithFormat do what you want?

Comment 20

[Benoit Girard (:BenWa)]

(In reply to Jonathan Watt [:jwatt] from comment #18)
> (In reply to Benoit Girard (:BenWa) from comment #10)
> > Part 1: Add CreateDataSourceSurfaceByCloning to moz2d
> 
> Will gfxUtils::CopySurfaceToDataSourceSurfaceWithFormat do what you want?

    * This function always creates a new surface. Do not call it if aSurface's
    * format is the same as aFormat. Such a non-conversion would just be an
    * unnecessary and wasteful copy (this function asserts to prevent that).

I guess not? I'm not sure if the original function should really be asserting that but it clearly is meant for a very specific use case.

Comment 21

[Benoit Girard (:BenWa)]

Attachment: Part 3: Add lz4 support

Comment 22

[Benoit Girard (:BenWa)]

Attachment: Part 3: Add lz4 support

Comment 23

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add layers.dump-texture feature. r=mstange

Fix include

Comment 24

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=211042a82d33

Comment 25

[Benoit Girard (:BenWa)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6aad17f431d1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c3e505887e9f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/33592fd41f1f

Comment 26

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f2bd10f0159c for nonunified bustage:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4444262&repo=mozilla-inbound

Comment 27

[Phil Ringnalda (:philor)]

Even though we all like backing out BenWa, this wasn't actually the cause of the non-unified bustage.

Comment 28

[Phil Ringnalda (:philor)]

Oh, *cute*!

This *was* the cause of the b2g non-unified bustage, while we had separate Android non-unified bustage which landed in the same interval.

Comment 29

[Benoit Girard (:BenWa)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f0226c3be051
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/47ba9f1d4762
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7aca65122115

Comment 30

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

B2G-non-unified is still broken with the same error. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7d799447add8

Comment 31

[Benoit Girard (:BenWa)]

Args, the second instance of Factory was missing the gfx:: prefix.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/95bb1d9c1fe2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c73f386da429
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fe7d53430f42

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/95bb1d9c1fe2
https://hg.mozilla.org/mozilla-central/rev/c73f386da429
https://hg.mozilla.org/mozilla-central/rev/fe7d53430f42