Add options to dump layers content with layers dump

RESOLVED FIXED in mozilla37

Status

()

Core
Graphics: Layers
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 8 obsolete attachments)

271.40 KB, image/png
Details
15.95 KB, patch
bas
: review+
Details | Diff | Splinter Review
4.83 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
18.10 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Created attachment 8529853 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Attachment #8529853 - Attachment is patch: true
(Assignee)

Updated

3 years ago
Assignee: nobody → bgirard
(Assignee)

Comment 1

3 years ago
Created attachment 8529854 [details]
Screenshot
(Assignee)

Comment 2

3 years ago
Created attachment 8530411 [details] [diff] [review]
Part 1: Add CreateDataSourceSurfaceByCloning to moz2d
Attachment #8530411 - Flags: review?(mstange)
Attachment #8530411 - Flags: review?(bas)
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 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.
Attachment #8530411 - Flags: review?(bas) → review-
(Assignee)

Comment 5

3 years ago
Created attachment 8530424 [details] [diff] [review]
Part 2: Add layers.dump-texture feature
Attachment #8530424 - Flags: review?(mstange)
(Assignee)

Comment 6

3 years ago
Created attachment 8530425 [details] [diff] [review]
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 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
Attachment #8530424 - Flags: review?(mstange) → review+
(Assignee)

Comment 8

3 years ago
(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.
(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
(Assignee)

Comment 10

3 years ago
Created attachment 8530442 [details] [diff] [review]
Part 1: Add CreateDataSourceSurfaceByCloning to moz2d
Attachment #8530411 - Attachment is obsolete: true
Attachment #8530411 - Flags: review?(mstange)
Attachment #8530442 - Flags: review?(mstange)
Attachment #8530442 - Flags: review?(bas)
(Assignee)

Comment 11

3 years ago
Created attachment 8530443 [details] [diff] [review]
Part 2: Add layers.dump-texture feature. r=mstange
Attachment #8530424 - Attachment is obsolete: true
Attachment #8530425 - Attachment is obsolete: true
Attachment #8530443 - Flags: review+
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"
Attachment #8530442 - Flags: review?(mstange) → review+
(Assignee)

Comment 13

3 years ago
Created attachment 8530448 [details] [diff] [review]
Part 1: Add CreateDataSourceSurfaceByCloning to moz2d
Attachment #8530442 - Attachment is obsolete: true
Attachment #8530442 - Flags: review?(bas)
Attachment #8530448 - Flags: review?(bas)
(Assignee)

Comment 15

3 years ago
Created attachment 8530486 [details] [diff] [review]
Part 3: Add lz4 support

You're right. This is -way- faster.
Attachment #8530486 - Flags: review?(jmuizelaar)
Comment on attachment 8530448 [details] [diff] [review]
Part 1: Add CreateDataSourceSurfaceByCloning to moz2d

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

I like it.
Attachment #8530448 - Flags: review?(bas) → review+
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?
Attachment #8530486 - Flags: review?(jmuizelaar) → review-
(In reply to Benoit Girard (:BenWa) from comment #10)
> Part 1: Add CreateDataSourceSurfaceByCloning to moz2d

Will gfxUtils::CopySurfaceToDataSourceSurfaceWithFormat do what you want?
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1097159
(Assignee)

Comment 20

3 years ago
(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.
(Assignee)

Comment 21

3 years ago
Created attachment 8533276 [details] [diff] [review]
Part 3: Add lz4 support
Attachment #8530486 - Attachment is obsolete: true
Attachment #8533276 - Flags: review?(jmuizelaar)
(Assignee)

Comment 22

3 years ago
Created attachment 8533277 [details] [diff] [review]
Part 3: Add lz4 support
Attachment #8529853 - Attachment is obsolete: true
Attachment #8533276 - Attachment is obsolete: true
Attachment #8533276 - Flags: review?(jmuizelaar)
Attachment #8533277 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
Attachment #8533277 - Attachment description: patch → Part 3: Add lz4 support
(Assignee)

Comment 23

3 years ago
Created attachment 8533283 [details] [diff] [review]
Part 2: Add layers.dump-texture feature. r=mstange

Fix include
Attachment #8530443 - Attachment is obsolete: true
Attachment #8533277 - Flags: review?(jmuizelaar) → review+
Even though we all like backing out BenWa, this wasn't actually the cause of the non-unified bustage.
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.
B2G-non-unified is still broken with the same error. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7d799447add8

Updated

3 years ago
Flags: needinfo?(bgirard)
https://hg.mozilla.org/mozilla-central/rev/95bb1d9c1fe2
https://hg.mozilla.org/mozilla-central/rev/c73f386da429
https://hg.mozilla.org/mozilla-central/rev/fe7d53430f42
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(Assignee)

Updated

3 years ago
Blocks: 1110998
(Assignee)

Updated

3 years ago
Blocks: 1112718
You need to log in before you can comment on or make changes to this bug.