Last Comment Bug 700240 - Improve display list debugging
: Improve display list debugging
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: Matt Woodrow (:mattwoodrow)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 732598 732706
Blocks: 703240
  Show dependency treegraph
 
Reported: 2011-11-07 01:11 PST by Matt Woodrow (:mattwoodrow)
Modified: 2012-03-03 08:09 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Print content of TextFrames (1.01 KB, patch)
2011-11-07 01:12 PST, Matt Woodrow (:mattwoodrow)
mats: feedback-
Details | Diff | Splinter Review
Display SVGEffects types (3.83 KB, patch)
2011-11-07 01:13 PST, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
Print display lists to a file (8.44 KB, patch)
2011-11-07 01:20 PST, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Example output file (22.50 KB, text/html)
2011-11-07 19:50 PST, Matt Woodrow (:mattwoodrow)
no flags Details
Example output file v2 (932.17 KB, text/html)
2011-11-08 00:12 PST, Matt Woodrow (:mattwoodrow)
no flags Details
Print content of TextFrames v2 (1.73 KB, patch)
2011-11-08 01:52 PST, Matt Woodrow (:mattwoodrow)
mats: review+
Details | Diff | Splinter Review
Display SVGEffects types v2 (3.87 KB, patch)
2011-11-08 01:53 PST, Matt Woodrow (:mattwoodrow)
matt.woodrow: review+
Details | Diff | Splinter Review
Print display lists to a file v2 (26.61 KB, patch)
2011-11-08 01:55 PST, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Print content of TextFrames v3 (2.61 KB, patch)
2011-11-08 15:52 PST, Matt Woodrow (:mattwoodrow)
matt.woodrow: review+
Details | Diff | Splinter Review
Print display lists to a file v3 (39.91 KB, patch)
2011-11-08 15:53 PST, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Make display list debugging independant of DEBUG (13.00 KB, patch)
2011-11-10 01:55 PST, Matt Woodrow (:mattwoodrow)
roc: review+
khuey: review-
Details | Diff | Splinter Review
Print content of TextFrames v4 (2.61 KB, patch)
2011-11-10 01:56 PST, Matt Woodrow (:mattwoodrow)
matt.woodrow: review+
Details | Diff | Splinter Review
Display SVGEffects types v3 (3.89 KB, patch)
2011-11-10 01:57 PST, Matt Woodrow (:mattwoodrow)
matt.woodrow: review+
Details | Diff | Splinter Review
Print display lists to a file v4 (40.06 KB, patch)
2011-11-10 01:57 PST, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Add DumpAsDataURL et al. for DrawTarget (3.13 KB, patch)
2011-11-10 01:58 PST, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
Make display list debugging independant of DEBUG v2 (12.01 KB, patch)
2011-11-10 15:39 PST, Matt Woodrow (:mattwoodrow)
khuey: review+
Details | Diff | Splinter Review
Print display lists to a file v5 (41.31 KB, patch)
2011-11-10 15:39 PST, Matt Woodrow (:mattwoodrow)
roc: review+
jacob.benoit.1: review-
Details | Diff | Splinter Review
Print display lists to a file v6 (42.44 KB, patch)
2011-11-17 19:26 PST, Matt Woodrow (:mattwoodrow)
jacob.benoit.1: review+
Details | Diff | Splinter Review

Description Matt Woodrow (:mattwoodrow) 2011-11-07 01:11:52 PST
I have a few ideas to make debugging display lists easier. Patches incoming.
Comment 1 Matt Woodrow (:mattwoodrow) 2011-11-07 01:12:31 PST
Created attachment 572404 [details] [diff] [review]
Print content of TextFrames

More or less the same as XULTextBox
Comment 2 Matt Woodrow (:mattwoodrow) 2011-11-07 01:13:11 PST
Created attachment 572405 [details] [diff] [review]
Display SVGEffects types
Comment 3 Matt Woodrow (:mattwoodrow) 2011-11-07 01:20:57 PST
Created attachment 572406 [details] [diff] [review]
Print display lists to a file

WIP - Needs a lot of work, just wanted to see if anyone had other ideas.

The basic idea is to generate a png file for every nsDisplayItem that gets printed and then dump the display list as formatted html with links to the png files.

This patch more or less works when gDumpPaintList is set to true.

Other things I want it to do:

- Print both display lists (optimized and unoptimized) to a single file
- Print layers to the file too and dump snapshots of them too
- Have proper settings to enable it, with a output path
- Each frame should be within a separate folder
- Configuration options (though I don't know how exactly this would work) to choose which frames to dump. It would be nicer to be able to trigger a full invalidation and just capture the repaint at least.
- Improve the formatting of the outputted page. Maybe I should just be dumping some intermediate format and use a tool similar to reftest_analyzer to generate the viewer.

Anyone have any thoughts or ideas here?
Comment 4 Mats Palmgren (:mats) 2011-11-07 01:46:25 PST
Comment on attachment 572404 [details] [diff] [review]
Print content of TextFrames

This will make nsTextFrame::List print the text twice.  It will also
affect nsTextFrame::ListTag which has traditionally been "short".
I propose:
1. remove the printout of the text in List
2. clamp the output to 40 chars or so, and indicate there's more in
   that case - perhaps by printing the length?
3. [value=] can be shortened to ""
Comment 5 Timothy Nikkel (:tnikkel) 2011-11-07 13:03:24 PST
Comment on attachment 572406 [details] [diff] [review]
Print display lists to a file

This is a really cool idea!
Comment 6 Matt Woodrow (:mattwoodrow) 2011-11-07 19:50:51 PST
Created attachment 572733 [details]
Example output file
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-07 20:44:49 PST
Perhaps it would be cleaner to output the images as data: URIs so everything gets into one big file? Easier to copy and paste snippets etc too.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-07 20:47:41 PST
Comment on attachment 572405 [details] [diff] [review]
Display SVGEffects types

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

Put PrintEffects in #ifdef DEBUG
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-07 20:50:53 PST
(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> - Have proper settings to enable it, with a output path
> - Each frame should be within a separate folder

I think it could be a single file per frame, all in the same folder.

> - Improve the formatting of the outputted page. Maybe I should just be
> dumping some intermediate format and use a tool similar to reftest_analyzer
> to generate the viewer.

Maybe ... but it would have to be a fairly complex format. HTML might be easier.
Comment 10 Matt Woodrow (:mattwoodrow) 2011-11-08 00:12:15 PST
Created attachment 572749 [details]
Example output file v2

Added inline data urls and dumping of ContainerLayerOGL
Comment 11 Matt Woodrow (:mattwoodrow) 2011-11-08 01:52:45 PST
Created attachment 572758 [details] [diff] [review]
Print content of TextFrames v2

Fixed comments
Comment 12 Matt Woodrow (:mattwoodrow) 2011-11-08 01:53:31 PST
Created attachment 572759 [details] [diff] [review]
Display SVGEffects types v2

Made DumpEffects DEBUG only. Carrying forward r=roc
Comment 13 Matt Woodrow (:mattwoodrow) 2011-11-08 01:55:30 PST
Created attachment 572760 [details] [diff] [review]
Print display lists to a file v2

Now prints all data urls into inline JS and opens these as links.

Added support for ContainerLayerOGL, can do the others as we need them. ThebesLayerOGL is a little more complex since it can have multiple textures (tiling, component alpha) and can be rotated.
Comment 14 Mats Palmgren (:mats) 2011-11-08 04:23:39 PST
Comment on attachment 572758 [details] [diff] [review]
Print content of TextFrames v2

> nsTextFrame::List

You need to add this before the return:
  fputs("\n", out);

Please also remove the following lines:
  7771    PRInt32 totalContentLength;
  7772    nsCAutoString tmp;
  7773    ToCString(tmp, &totalContentLength);
  7774  

and replace 'totalContentLength' on this line:
 7776    bool isComplete = GetContentEnd() == totalContentLength;

with 'GetContent()->TextLength()'
Comment 15 Matt Woodrow (:mattwoodrow) 2011-11-08 15:52:27 PST
Created attachment 573031 [details] [diff] [review]
Print content of TextFrames v3

Fixed review comments. Carrying forward r=mats
Comment 16 Matt Woodrow (:mattwoodrow) 2011-11-08 15:53:24 PST
Created attachment 573032 [details] [diff] [review]
Print display lists to a file v3

Added displaying of ThebesLayerOGL and LayerManagerOGL. Fixed bug displaying ContainerLayerOGL.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-09 03:17:02 PST
Comment on attachment 573032 [details] [diff] [review]
Print display lists to a file v3

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

::: gfx/layers/Layers.cpp
@@ +453,5 @@
>    if (opacity != 1.0f && HasMultipleChildren()) {
>      useIntermediateSurface = true;
> +#ifdef DEBUG
> +  } else if (gDumpPaintList && gDumpPaintToFile) {
> +    useIntermediateSurface = true;

I'm not comfortable with these options potentially changing rendering. Could we just take this out?

@@ +577,4 @@
>  void
>  Layer::Dump(FILE* aFile, const char* aPrefix)
>  {
> +  bool asHTML = !(aFile == stdout || aFile == stderr);

This is a bit gross. I think we should pass the format separately.

Although, is there really a need for the non-HTML format?

@@ +591,5 @@
> +      nsCString string("ThebesLayer-");
> +      string.AppendInt((PRUint64)this);
> +      string.Append("-");
> +      string.AppendInt(gPaintCount);
> +      fprintf(aFile, "href=\"javascript:ViewImage('%s')\"", string.BeginReading());

Share code between the two branches?

Why is gPaintCount needed here?

::: gfx/thebes/GLContext.cpp
@@ +1697,5 @@
> +      for (int i = 0; i < aSize.width; ++i) {
> +        *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
> +        row++;
> +      }
> +    }

This code can be shared with code above

@@ +1711,5 @@
> +      ctx->Translate(-gfxPoint(0.0, aSize.height));
> +      ctx->SetSource(isurf);
> +      ctx->Paint();
> +      isurf = isurf2;
> +    }

So can this

::: layout/base/nsDisplayList.h
@@ +861,5 @@
>    // of the item. Paint implementations can use this to limit their drawing.
>    // Guaranteed to be contained in GetBounds().
>    nsRect    mVisibleRect;
> +  // True if this frame has been painted.
> +  bool      mPainted;

This makes all display items 4 bytes bigger, I think. Let's not do this.
Comment 18 Matt Woodrow (:mattwoodrow) 2011-11-09 11:56:35 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> Comment on attachment 573032 [details] [diff] [review] [diff] [details] [review]
> Print display lists to a file v3
> 
> Review of attachment 573032 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/Layers.cpp
> @@ +453,5 @@
> >    if (opacity != 1.0f && HasMultipleChildren()) {
> >      useIntermediateSurface = true;
> > +#ifdef DEBUG
> > +  } else if (gDumpPaintList && gDumpPaintToFile) {
> > +    useIntermediateSurface = true;
> 
> I'm not comfortable with these options potentially changing rendering. Could
> we just take this out?

I don't know of any other way to capture what was drawn by the ContainerLayer, the same goes for the PaintInactiveLayer changes. I was actually considering making the normal nsDisplayItem::Paint() path do this too, since having it affect rendering would confirm that what is being dumped is the same as what is being drawn to screen.

> 
> @@ +577,4 @@
> >  void
> >  Layer::Dump(FILE* aFile, const char* aPrefix)
> >  {
> > +  bool asHTML = !(aFile == stdout || aFile == stderr);
> 
> This is a bit gross. I think we should pass the format separately.
> 
> Although, is there really a need for the non-HTML format?

I think we still want to print display lists to stdout sometimes, and HTML won't be helpful here.

> 
> @@ +591,5 @@
> > +      nsCString string("ThebesLayer-");
> > +      string.AppendInt((PRUint64)this);
> > +      string.Append("-");
> > +      string.AppendInt(gPaintCount);
> > +      fprintf(aFile, "href=\"javascript:ViewImage('%s')\"", string.BeginReading());
> 
> Share code between the two branches?
> 
> Why is gPaintCount needed here?

It isn't, legacy code, will remove.

> 
> ::: gfx/thebes/GLContext.cpp
> @@ +1697,5 @@
> > +      for (int i = 0; i < aSize.width; ++i) {
> > +        *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
> > +        row++;
> > +      }
> > +    }
> 
> This code can be shared with code above
> 
> @@ +1711,5 @@
> > +      ctx->Translate(-gfxPoint(0.0, aSize.height));
> > +      ctx->SetSource(isurf);
> > +      ctx->Paint();
> > +      isurf = isurf2;
> > +    }
> 
> So can this
> 
> ::: layout/base/nsDisplayList.h
> @@ +861,5 @@
> >    // of the item. Paint implementations can use this to limit their drawing.
> >    // Guaranteed to be contained in GetBounds().
> >    nsRect    mVisibleRect;
> > +  // True if this frame has been painted.
> > +  bool      mPainted;
> 
> This makes all display items 4 bytes bigger, I think. Let's not do this.

Sure, this should be #ifdef DEBUG.
Comment 19 Benoit Girard (:BenWa) 2011-11-09 12:59:28 PST
I'm not a fan of using DEBUG for this kind of thing. To use these kinds of features you need to use DEBUG which pulls in the kitchen sink.

Can't we do something like DEBUG_FEATURE and have DEBUG define that? It would let us selectively enable a feature like this without pulling other DEBUG features.
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-09 15:02:56 PST
(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> I don't know of any other way to capture what was drawn by the
> ContainerLayer, the same goes for the PaintInactiveLayer changes. I was
> actually considering making the normal nsDisplayItem::Paint() path do this
> too, since having it affect rendering would confirm that what is being
> dumped is the same as what is being drawn to screen.

How important is it to capture every ContainerLayer?

> I think we still want to print display lists to stdout sometimes, and HTML
> won't be helpful here.

I think it's probably not worth supporting non-HTML. Redirect to a file, or pipe through something that de-HTML-izes it.

> Sure, this should be #ifdef DEBUG.

Maybe we should have #ifdef MOZ_DUMP_PAINTING or something? It would be great if we can get these dumps in non-debug builds. Would speed things up a lot when you need to dump a lot of frames.
Comment 21 Matt Woodrow (:mattwoodrow) 2011-11-09 15:36:25 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> > I don't know of any other way to capture what was drawn by the
> > ContainerLayer, the same goes for the PaintInactiveLayer changes. I was
> > actually considering making the normal nsDisplayItem::Paint() path do this
> > too, since having it affect rendering would confirm that what is being
> > dumped is the same as what is being drawn to screen.
> 
> How important is it to capture every ContainerLayer?

I feel that it's reasonably important. Since the retained content of ThebesLayers is often rotated, and in multiple buffers (because of tiling and/or component alpha), it's nice to be able to see how this content is actually draw to a surface.

I can't see any reason that forcing intermediate surfaces would affect rendering unless we have bugs. In which case it's better for these to be visible so they get fixed :)

> 
> I think it's probably not worth supporting non-HTML. Redirect to a file, or
> pipe through something that de-HTML-izes it.

Sure, simplifies the code a fair bit then.

> Maybe we should have #ifdef MOZ_DUMP_PAINTING or something? It would be
> great if we can get these dumps in non-debug builds. Would speed things up a
> lot when you need to dump a lot of frames.

Sounds reasonable. Any idea where I can define this so that it's always defined when DEBUG is?
Comment 22 Benoit Girard (:BenWa) 2011-11-09 15:59:55 PST
(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> Sounds reasonable. Any idea where I can define this so that it's always
> defined when DEBUG is?

In the config/Makefile would be one option.

Another would be to make a "feature.h" type header that does this and also documents them briefly in comments. If we only keep pre-processor instructions there it should be cheap to include everywhere but still unnecessarily messy.

It would be nice to nail down a solution for this across the mozilla platform since this comes up as we add more cool debug features like this one. Then again I don't want to hijack your bug.
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-09 16:29:24 PST
(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> I can't see any reason that forcing intermediate surfaces would affect
> rendering unless we have bugs. In which case it's better for these to be
> visible so they get fixed :)

I'm worried that forcing intermediate surfaces could hide bugs. I guess I don't really feel too strongly about it. Maybe have an additional #define to control this.

Just stick the #define in a .h file somewhere. That's what we do elsewhere. Consolidating those into a single file later would be easy.
Comment 24 Matt Woodrow (:mattwoodrow) 2011-11-10 01:55:34 PST
Created attachment 573458 [details] [diff] [review]
Make display list debugging independant of DEBUG

I possibly went a little overboard here, I figured it was worth doing right..
Comment 25 Matt Woodrow (:mattwoodrow) 2011-11-10 01:56:35 PST
Created attachment 573459 [details] [diff] [review]
Print content of TextFrames v4

Rebased patch, carrying forward r=mats
Comment 26 Matt Woodrow (:mattwoodrow) 2011-11-10 01:57:12 PST
Created attachment 573460 [details] [diff] [review]
Display SVGEffects types v3

Rebased patch, carrying forward r=roc
Comment 27 Matt Woodrow (:mattwoodrow) 2011-11-10 01:57:59 PST
Created attachment 573461 [details] [diff] [review]
Print display lists to a file v4

Fixed review comments.
Comment 28 Matt Woodrow (:mattwoodrow) 2011-11-10 01:58:54 PST
Created attachment 573462 [details] [diff] [review]
Add DumpAsDataURL et al. for DrawTarget
Comment 29 Matt Woodrow (:mattwoodrow) 2011-11-10 01:59:43 PST
(In reply to Matt Woodrow (:mattwoodrow) from comment #24)
> Created attachment 573458 [details] [diff] [review] [diff] [details] [review]
> Make display list debugging independant of DEBUG

*Independent
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-10 02:18:56 PST
Comment on attachment 573461 [details] [diff] [review]
Print display lists to a file v4

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

::: gfx/layers/opengl/ContainerLayerOGL.cpp
@@ +36,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
>  #include "ContainerLayerOGL.h"
>  #include "gfxUtils.h"
> +#include "nsLayoutUtils.h"

This is a bad layering violation. Let's find another way to avoid having use gDumpPaintList like this. Maybe move it to gfxUtils and call it gDumpPainting?

::: gfx/thebes/gfxASurface.cpp
@@ +857,5 @@
>    if (aFile) {
>      fprintf(aFile, "%s", string.BeginReading());
> +    if (aFile == stdout || aFile == stderr) {
> +      fprintf(aFile, "\n");
> +    }

This is very magical and we probably should make the caller do it.

::: layout/base/FrameLayerBuilder.cpp
@@ +2156,5 @@
>        }
> +#ifdef MOZ_DUMP_PAINTING
> +      nsRect bounds = cdi->mItem->GetBounds(builder);
> +      gfxRect gfxBounds(bounds.x, bounds.y, bounds.width, bounds.height);
> +      gfxBounds.ScaleInverse(rc->AppUnitsPerDevPixel());

Can't these go into the if (gDumpPaintList)?

Also, call gfxBounds something else. gfxCamelCase is used like a type.

@@ +2174,5 @@
> +    
> +        surf->SetDeviceOffset(gfxPoint(0, 0));
> +        rc->ThebesContext()->SetSource(surf, gfxBounds.TopLeft());
> +        rc->ThebesContext()->Rectangle(gfxBounds);
> +        rc->ThebesContext()->Fill();

I'm a bit worried that this might cause bugs to appear or disappear due to the intermediate surface. Can we just paint it twice?

@@ +2175,5 @@
> +        surf->SetDeviceOffset(gfxPoint(0, 0));
> +        rc->ThebesContext()->SetSource(surf, gfxBounds.TopLeft());
> +        rc->ThebesContext()->Rectangle(gfxBounds);
> +        rc->ThebesContext()->Fill();
> +      } else {

Move this out to a helper function? It bloats DrawThebesLayer.

::: layout/base/nsLayoutUtils.cpp
@@ +1273,3 @@
>  static bool gDumpEventList = false;
> +FILE *gDumpPaintFile = NULL;
> +bool gDumpPaintToFile = false;

So the idea is that you attach a debugger to use these two?
Comment 31 Matt Woodrow (:mattwoodrow) 2011-11-10 02:58:11 PST
> @@ +2174,5 @@
> > +    
> > +        surf->SetDeviceOffset(gfxPoint(0, 0));
> > +        rc->ThebesContext()->SetSource(surf, gfxBounds.TopLeft());
> > +        rc->ThebesContext()->Rectangle(gfxBounds);
> > +        rc->ThebesContext()->Fill();
> 
> I'm a bit worried that this might cause bugs to appear or disappear due to
> the intermediate surface. Can we just paint it twice?

I'm equally worried about the reverse as well. I've already had a bug where the dumped image wasn't the same as what was actually drawn (with Azure/Thebes, the actual context was Azure backed and the temporary was cairo backed), which made this tool useless. My hope was that any changes to rendering by enabling this would be obvious and lead to quick detection/fixing rather than subtly hiding them.

At least for ContainerLayerOGL and PaintInactiveLayer I don't see any obvious alternatives, so it made sense to make this consistent.

I'm open to being convinced otherwise though.

> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +1273,3 @@
> >  static bool gDumpEventList = false;
> > +FILE *gDumpPaintFile = NULL;
> > +bool gDumpPaintToFile = false;
> 
> So the idea is that you attach a debugger to use these two?

This is the only way that I've ever used this functionality. I'll add env var checking as well to stay consistent with the existing code.

> 
> ::: gfx/thebes/gfxASurface.cpp
> @@ +857,5 @@
> >    if (aFile) {
> >      fprintf(aFile, "%s", string.BeginReading());
> > +    if (aFile == stdout || aFile == stderr) {
> > +      fprintf(aFile, "\n");
> > +    }
> 
> This is very magical and we probably should make the caller do it.

I don't believe this has any callers other than this patch currently, but I frequently call it from a debugger, where emitting the newline automatically is useful. Agreed that this is pretty ugly though.

Maybe a bool argument with a default value..
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-10 12:57:57 PST
(In reply to Matt Woodrow (:mattwoodrow) from comment #31)
> I'm equally worried about the reverse as well. I've already had a bug where
> the dumped image wasn't the same as what was actually drawn (with
> Azure/Thebes, the actual context was Azure backed and the temporary was
> cairo backed), which made this tool useless. My hope was that any changes to
> rendering by enabling this would be obvious and lead to quick
> detection/fixing rather than subtly hiding them.

OK, we can do this.

> > ::: gfx/thebes/gfxASurface.cpp
> > @@ +857,5 @@
> > >    if (aFile) {
> > >      fprintf(aFile, "%s", string.BeginReading());
> > > +    if (aFile == stdout || aFile == stderr) {
> > > +      fprintf(aFile, "\n");
> > > +    }
> > 
> > This is very magical and we probably should make the caller do it.
> 
> I don't believe this has any callers other than this patch currently, but I
> frequently call it from a debugger, where emitting the newline automatically
> is useful. Agreed that this is pretty ugly though.

Have two versions, one which appends the \n and one which doesn't?
Comment 33 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-10 13:14:51 PST
Comment on attachment 573458 [details] [diff] [review]
Make display list debugging independant of DEBUG

The AC_SUBSTs and autoconf.mk.in changes are not necessary.  AC_SUBST defines variables in makefiles, AC_DEFINE defines variables in C/C++ files, and you're only using the latter.
Comment 34 Matt Woodrow (:mattwoodrow) 2011-11-10 15:39:18 PST
Created attachment 573678 [details] [diff] [review]
Make display list debugging independant of DEBUG v2

Removed AC_SUBST and autoconf.mk.in changes.
Comment 35 Matt Woodrow (:mattwoodrow) 2011-11-10 15:39:51 PST
Created attachment 573679 [details] [diff] [review]
Print display lists to a file v5
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-10 15:50:45 PST
Comment on attachment 573679 [details] [diff] [review]
Print display lists to a file v5

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

Someone else should review the GL changes
Comment 37 Benoit Jacob [:bjacob] (mostly away) 2011-11-16 19:32:07 PST
Comment on attachment 573679 [details] [diff] [review]
Print display lists to a file v5

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

::: gfx/thebes/GLContext.cpp
@@ +219,5 @@
>          { (PRFuncPtr*) &mSymbols.fGetProgramInfoLog, { "GetProgramInfoLog", "GetProgramInfoLogARB", NULL } },
>          { (PRFuncPtr*) &mSymbols.fTexParameteri, { "TexParameteri", NULL } },
>          { (PRFuncPtr*) &mSymbols.fTexParameterf, { "TexParameterf", NULL } },
>          { (PRFuncPtr*) &mSymbols.fGetString, { "GetString", NULL } },
> +        { (PRFuncPtr*) &mSymbols.fGetTexImage, { "GetTexImage", NULL } },

This is part of desktop OpenGL 2.1 but not of OpenGL ES 2.0.

On ES you'd have to construct a FBO and readPixels.

Do you want to support ES2? If this is a developer-only feature, I guess that ES2 support is not needed. Though on Windows, ANGLE is ES2.

@@ +1527,5 @@
> +  for (int j = 0; j < size.height; ++j) {
> +    PRUint32 *row = (PRUint32*) (aSurf->Data() + aSurf->Stride() * j);
> +    for (int i = 0; i < size.width; ++i) {
> +      *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
> +      row++;

Whoops, you're incrementing two variables at each iteration here: |row| and |i|. Since apparently this is considered performance critical, you could get rid of |i| altogether and express the termination condition purely in terms of |row|. Also, it would help the compiler a bit if you introduced a reference to *row so it would be very clear that all these dereferences are at the same memory location.

Pushing this a little farther, do ask Jeff M what is the guaranteed alignment of each row in a gfxImageSurface. If it's >= 64 bit, then you can safely process 2 pixels at once by using a PRUint64 here, regardless of the parity of |size.width|.
Comment 38 Matt Woodrow (:mattwoodrow) 2011-11-16 19:47:29 PST
(In reply to Benoit Jacob [:bjacob] from comment #37)
> Comment on attachment 573679 [details] [diff] [review] [diff] [details] [review]
> Print display lists to a file v5
> 
> Review of attachment 573679 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/GLContext.cpp
> @@ +219,5 @@
> >          { (PRFuncPtr*) &mSymbols.fGetProgramInfoLog, { "GetProgramInfoLog", "GetProgramInfoLogARB", NULL } },
> >          { (PRFuncPtr*) &mSymbols.fTexParameteri, { "TexParameteri", NULL } },
> >          { (PRFuncPtr*) &mSymbols.fTexParameterf, { "TexParameterf", NULL } },
> >          { (PRFuncPtr*) &mSymbols.fGetString, { "GetString", NULL } },
> > +        { (PRFuncPtr*) &mSymbols.fGetTexImage, { "GetTexImage", NULL } },
> 
> This is part of desktop OpenGL 2.1 but not of OpenGL ES 2.0.
> 
> On ES you'd have to construct a FBO and readPixels.
> 
> Do you want to support ES2? If this is a developer-only feature, I guess
> that ES2 support is not needed. Though on Windows, ANGLE is ES2.

Good catch, I'll fix this. This is just for developer tools, we can have it fail on ES2 until somebody implements it there.

> 
> @@ +1527,5 @@
> > +  for (int j = 0; j < size.height; ++j) {
> > +    PRUint32 *row = (PRUint32*) (aSurf->Data() + aSurf->Stride() * j);
> > +    for (int i = 0; i < size.width; ++i) {
> > +      *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
> > +      row++;
> 
> Whoops, you're incrementing two variables at each iteration here: |row| and
> |i|. Since apparently this is considered performance critical, you could get
> rid of |i| altogether and express the termination condition purely in terms
> of |row|. Also, it would help the compiler a bit if you introduced a
> reference to *row so it would be very clear that all these dereferences are
> at the same memory location.
> 
> Pushing this a little farther, do ask Jeff M what is the guaranteed
> alignment of each row in a gfxImageSurface. If it's >= 64 bit, then you can
> safely process 2 pixels at once by using a PRUint64 here, regardless of the
> parity of |size.width|.

I don't think performance overly matters here. The developer usage isn't perf critical, and the existing path doesn't appear to be used.
Comment 41 Matt Woodrow (:mattwoodrow) 2011-11-17 19:26:30 PST
Created attachment 575368 [details] [diff] [review]
Print display lists to a file v6
Comment 42 Benoit Jacob [:bjacob] (mostly away) 2011-11-19 14:31:49 PST
Comment on attachment 575368 [details] [diff] [review]
Print display lists to a file v6

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

Sorry for being so picky, but I have one potential security concern about GetTexImage below; also if you dont want to make SwapRandB better please explain the reason why it doesn't matter.

::: gfx/thebes/GLContext.cpp
@@ +1534,5 @@
> +    PRUint32 *row = (PRUint32*) (aSurf->Data() + aSurf->Stride() * j);
> +    for (int i = 0; i < size.width; ++i) {
> +      *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
> +      row++;
> +    }

This is still not addressed. Does this for some reason not matter? If this is not performance-critical, then why is this written using complicated bit operations? 

Also, as I said in my earlier review, this approach would pay off much more if you did it on 64bits at once; right now I dont see it being much faster than a naive swap of R and B.

Finally, you actually don't have to do nested for loops here. A single for loop over all pixels should be enough. Something like

PRUint32 *start = aSurf->Data();
PRUint32 *end = start + aSurf->Stride() * size.height;
for(PRUint32 *pixel = start; pixel != end; pixel++)
  *pixel = (*pixel & 0xff00ff00) | ((*pixel & 0xff) << 16) | ((*pixel & 0xff0000) >> 16);

::: gfx/thebes/GLContext.h
@@ +1924,5 @@
>  
> +    void fGetTexImage(GLenum target, GLint level, GLenum format, GLenum type, GLvoid *img) {
> +        if (!mSymbols.fGetTexImage) {
> +          return;
> +        }

Here you're returning early and leaving the img buffer untouched; but in the general case this buffer would get filled with data, so the caller might be relying on this call to initialize the contents of the buffer.

Wouldn't it be safer to clear the buffer before returning here?
Comment 43 Matt Woodrow (:mattwoodrow) 2011-12-04 15:18:39 PST
 
> ::: gfx/thebes/GLContext.cpp
> @@ +1534,5 @@
> > +    PRUint32 *row = (PRUint32*) (aSurf->Data() + aSurf->Stride() * j);
> > +    for (int i = 0; i < size.width; ++i) {
> > +      *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
> > +      row++;
> > +    }
> 
> This is still not addressed. Does this for some reason not matter? If this
> is not performance-critical, then why is this written using complicated bit
> operations?

I'm not sure why it was written by this, the only usage we have of it is for debugging. Performance really shouldn't be critical at all here.

> ::: gfx/thebes/GLContext.h
> @@ +1924,5 @@
> >  
> > +    void fGetTexImage(GLenum target, GLint level, GLenum format, GLenum type, GLvoid *img) {
> > +        if (!mSymbols.fGetTexImage) {
> > +          return;
> > +        }
> 
> Here you're returning early and leaving the img buffer untouched; but in the
> general case this buffer would get filled with data, so the caller might be
> relying on this call to initialize the contents of the buffer.
> 
> Wouldn't it be safer to clear the buffer before returning here?

This is the same behavior as the actual glGetTexImage function (when it encounters an error). In addition, the only caller is a wrapper function that handles this correctly.
Comment 44 Benoit Jacob [:bjacob] (mostly away) 2011-12-21 12:31:02 PST
Comment on attachment 575368 [details] [diff] [review]
Print display lists to a file v6

Oh, that's fine then.
Comment 45 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-28 07:47:22 PST
Can this land?
Comment 46 Matt Woodrow (:mattwoodrow) 2012-03-01 00:29:01 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec9be13d5fad
Comment 47 Marco Bonardo [::mak] 2012-03-02 06:08:45 PST
https://hg.mozilla.org/mozilla-central/rev/ec9be13d5fad

Note You need to log in before you can comment on or make changes to this bug.