700240 - Improve display list debugging

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

I have a few ideas to make debugging display lists easier. Patches incoming.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Print content of TextFrames

More or less the same as XULTextBox

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Display SVGEffects types

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: 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 (inactive)]

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)]

Comment on attachment 572406 [details] [diff] [review]
Print display lists to a file

This is a really cool idea!

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Example output file

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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)]

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)]

(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)]

Attachment: Example output file v2

Added inline data urls and dumping of ContainerLayerOGL

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Print content of TextFrames v2

Fixed comments

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Display SVGEffects types v2

Made DumpEffects DEBUG only. Carrying forward r=roc

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: 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 (inactive)]

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)]

Attachment: Print content of TextFrames v3

Fixed review comments. Carrying forward r=mats

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: 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)]

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)]

(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)]

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)]

(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)]

(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)]

(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)]

(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)]

Attachment: 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)]

Attachment: Print content of TextFrames v4

Rebased patch, carrying forward r=mats

Comment 26

[Matt Woodrow (:mattwoodrow)]

Attachment: Display SVGEffects types v3

Rebased patch, carrying forward r=roc

Comment 27

[Matt Woodrow (:mattwoodrow)]

Attachment: Print display lists to a file v4

Fixed review comments.

Comment 28

[Matt Woodrow (:mattwoodrow)]

Attachment: Add DumpAsDataURL et al. for DrawTarget

Comment 29

[Matt Woodrow (:mattwoodrow)]

(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)]

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)]

> @@ +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)]

(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 (Exited; not receiving bugmail, old account, do not use)]

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)]

Attachment: Make display list debugging independant of DEBUG v2

Removed AC_SUBST and autoconf.mk.in changes.

Comment 35

[Matt Woodrow (:mattwoodrow)]

Attachment: Print display lists to a file v5

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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)]

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)]

(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 39

[Matt Woodrow (:mattwoodrow)]

Landed all but one of the patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed596fa3b48
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a137a1dabb9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9740cfa38c78
https://hg.mozilla.org/integration/mozilla-inbound/rev/34b221ee7e09

Comment 40

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/5ed596fa3b48
https://hg.mozilla.org/mozilla-central/rev/2a137a1dabb9
https://hg.mozilla.org/mozilla-central/rev/9740cfa38c78
https://hg.mozilla.org/mozilla-central/rev/34b221ee7e09

I suppose "landed all but one" means the bug should stay open.

Comment 41

[Matt Woodrow (:mattwoodrow)]

Attachment: Print display lists to a file v6

Comment 42

[Benoit Jacob [:bjacob] (mostly away)]

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)]

 
> ::: 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)]

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)]

Can this land?

Comment 46

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec9be13d5fad

Comment 47

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/ec9be13d5fad