Closed Bug 867656 Opened 7 years ago Closed 7 years ago

[layers-refactoring] MOZ_DUMP_PAINT broken

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: gal, Assigned: nical)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

ContentHost->Dump() returns nullptr and we try to write that to disk.
Blocks: 792119
bjacob helped debug. grep "Dump()" in opengl/ has 0 hits, so looks like we don't implement this for the opengl backend and crash. We should make Dump() pure virtual to avoid this.
It isn't very clear from the code what's meant to happen, but I'm not sure that Dump() is all sorted out:
 * ContentHostBase does implement Dump(), letting TextureHost pointer do the work.
 * TiledContentHost does not implement Dump() - I imagine it should do the same thing as ContentHostBase and pass this to its TextureHost?
 * As Andreas mentioned, the OpenGL implementation of TextureHost does not implement Dump().
Flags: needinfo?(nical.bugzilla)
I'll fix this it shouldn't take long
Assignee: nobody → nical.bugzilla
Flags: needinfo?(nical.bugzilla)
let's start with TextureHost classes.
Attachment #745837 - Flags: review?
Attachment #745837 - Flags: review? → review?(bjacob)
Attachment #745837 - Flags: review?(bjacob) → review+
I changed my mind and Called the methods that return a gfxASurface GetAsSurface to not confuse them With Dump(File,prefix,isHTML).

Also I decided not to put them inside MOZ_DUMP_PAITING, because they are useful to have for debug purposes so it's a shame to have to rebuild when you realize that you want to see what's in the layer tree.

Otherwise it's the same thing as previous version.
Attachment #745837 - Attachment is obsolete: true
Attachment #746076 - Flags: review?(bjacob)
Prints things nicely, if aDumpHtml is true the content of the TextureHosts are links to dataURLs.
Attachment #746078 - Flags: review?(matt.woodrow)
Attachment #746078 - Flags: review?(matt.woodrow) → review+
Comment on attachment 746076 [details] [diff] [review]
Implement GetAsSurface for GL TextureHosts

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

r=me with nits and questions.

::: gfx/layers/composite/CanvasLayerComposite.cpp
@@ +65,5 @@
> +#ifdef MOZ_DUMP_PAINTING
> +  if (gfxUtils::sDumpPainting) {
> +    nsRefPtr<gfxImageSurface> surf = mImageHost->GetAsSurface();
> +    WriteSnapshotToDumpFile(this, surf);
> +  }

Why is this only in CanvasLayerComposite and not in other Layer types?

::: gfx/layers/composite/CompositableHost.h
@@ +156,5 @@
>      SetCompositor(nullptr);
>    }
>  
> +#ifdef MOZ_DUMP_PAINTING
> +  virtual already_AddRefed<gfxImageSurface> GetAsSurface() { return nullptr; }

How about pure virtual? Why would any compositablehost not implement this?

::: gfx/layers/composite/TextureHost.h
@@ +90,5 @@
>     * one device texture and must be tiled internally.
>     */
>    virtual TileIterator* AsTileIterator() { return nullptr; }
>  
> +  virtual already_AddRefed<gfxImageSurface> GetAsSurface() { return nullptr; }

How about pure virtual? Why would any texturehost not implement this?

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +852,5 @@
> +                     false,
> +                     GetShaderProgram())
> +    : nullptr;
> +  return surf.forget();
> +}

Please also implement GrallocTextureHostOGL, and check that no other one is missing.
Attachment #746076 - Flags: review?(bjacob) → review+
(In reply to Benoit Jacob [:bjacob] from comment #7)
> Comment on attachment 746076 [details] [diff] [review]
> Implement GetAsSurface for GL TextureHosts
> 
> Review of attachment 746076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits and questions.
> 
> ::: gfx/layers/composite/CanvasLayerComposite.cpp
> @@ +65,5 @@
> > +#ifdef MOZ_DUMP_PAINTING
> > +  if (gfxUtils::sDumpPainting) {
> > +    nsRefPtr<gfxImageSurface> surf = mImageHost->GetAsSurface();
> > +    WriteSnapshotToDumpFile(this, surf);
> > +  }
> 
> Why is this only in CanvasLayerComposite and not in other Layer types?

It was actually already in the other layer types and missing here
Depends on: 872568
https://hg.mozilla.org/mozilla-central/rev/3ded05a81797
https://hg.mozilla.org/mozilla-central/rev/e6826a5768ef
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 877110
You need to log in before you can comment on or make changes to this bug.