379015 - Fix nsPresShell::RenderOffscreen

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Eli Friedman]

RenderOffscreen is currently unused outside of DEBUG-only code, and it isn't actually usable with Thebes because there's no way to get the resulting data out of the rendering context (the old way, nsIDrawingSurface::Lock, is not implemented).  nsCanvasRenderingContext2D contains an working implementation, but using it will probably require changing RenderOffscreen to take a gfxContext rather than returning an nsIRenderingContext.

Any suggestions about how the API should change?

(Motivation: I found myself wanting to use RenderOffscreen to debug verifyreflow, but then I discovered this problem.)

Comment 1

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

Taking a gfxContext sounds good to me.

Comment 2

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

We could return an already_AddRefed<gfxASurface>, but taking a gfxContext parameter is more flexible.

Comment 3

[Eli Friedman]

Attachment: WIP

WIP; I figure it's worth posting at this point. Not working. (I don't really know why it isn't working, though.)

Comment 4

[Eli Friedman]

Attachment: Patch v1

I think that the issues with the first version was actually just issues with the code I was using to test it.  Anyways, here's the patch.  Included is some code which dumps a webpage to a PNG.

I'll leave converting the canvas code to use this for later (that file is really messy because it has three versions of a lot of stuff.)

Comment 5

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

+nsresult
+DumpToPNG(nsIPresShell* shell, nsAString& name) {

Make this static, and I think #ifdef DEBUG.

Do we need all the other VerifyReflow changes in this patch? I didn't understand some of them.

+  PRUint32 length;
+  encoder->Available(&length);

I would just choose a buffer size, say 10000.

+    DumpToPNG(sh, stra);
+    DumpToPNG(this, strb);

What is the point of dumping the same frame tree twice.

void
 DocumentViewerImpl::DumpContentToPPM(const char* aFileName)
 {
+#if 0

Just remove all this, I think.

We should change the name of the method to something else, now that it can render to anything. Just Render? Need to explain how it's different from PresShell::Paint, in a comment.

Speaking of which, perhaps we should just add a "aPaintCaret" parameter to nsLayoutUtils::PaintFrame and call it from PresShell::Render?

Comment 6

[Eli Friedman]

(In reply to comment #5)
> +nsresult
> +DumpToPNG(nsIPresShell* shell, nsAString& name) {
> 
> Make this static, and I think #ifdef DEBUG.

Okay; it already is #ifdef DEBUG.

> Do we need all the other VerifyReflow changes in this patch? I didn't
> understand some of them.

Let me give a quick explanation of VerifyReflow: what it does is create a new presshell, and construct it from scratch.  Then, the new frame tree is compared to the old, and any differences are noted in a dump to stdout.

There are some issues; there are a couple of hacks I'm planning on adding to deal with subdocuments.  However, the one bug that I can't seem to figure out is that anonymous content doesn't seem to work right.  So I've been experimenting a bit.  I'll take out the changes I'm not sure about, though.

> +  PRUint32 length;
> +  encoder->Available(&length);
> 
> I would just choose a buffer size, say 10000.

I don't really know the right way to do this.  I was mostly copying code.  I can change it to a fixed size buffer plus a loop, though.

> +    DumpToPNG(sh, stra);
> +    DumpToPNG(this, strb);
> 
> What is the point of dumping the same frame tree twice.

Despite the name, VerifyReflow actually creates another presshell from scratch.  So these are different frame trees.

> void
>  DocumentViewerImpl::DumpContentToPPM(const char* aFileName)
>  {
> +#if 0
> 
> Just remove all this, I think.

So I should get rid of MOZ_FORCE_PAINT_AFTER_ONLOAD?  That's fine.

> We should change the name of the method to something else, now that it can
> render to anything. Just Render? Need to explain how it's different from
> PresShell::Paint, in a comment.

Actually, it can't render to anything; stuff starts asserting like crazy if you try to render to a generic image surface.

> Speaking of which, perhaps we should just add a "aPaintCaret" parameter to
> nsLayoutUtils::PaintFrame and call it from PresShell::Render?

I suppose; it's not a huge simplification, though.

Comment 7

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

> > +  PRUint32 length;
> > +  encoder->Available(&length);
> > 
> > I would just choose a buffer size, say 10000.
> 
> I don't really know the right way to do this.  I was mostly copying code.  I
> can change it to a fixed size buffer plus a loop, though.

There already is a loop, inside WriteFrom. I think choosing the buffer size here only affects performance. 

> > void
> >  DocumentViewerImpl::DumpContentToPPM(const char* aFileName)
> >  {
> > +#if 0
> > 
> > Just remove all this, I think.
> 
> So I should get rid of MOZ_FORCE_PAINT_AFTER_ONLOAD?  That's fine.

Yeah.

> Actually, it can't render to anything; stuff starts asserting like crazy if you
> try to render to a generic image surface.

Yeah, I'm guessing text fails, at least on Windows: but that's just a nasty bug and should be fixed.

> > Speaking of which, perhaps we should just add a "aPaintCaret" parameter to
> > nsLayoutUtils::PaintFrame and call it from PresShell::Render?
> 
> I suppose; it's not a huge simplification, though.

Still, I'd rather reduce the code duplication.

Comment 8

[Eli Friedman]

(In reply to comment #7)
> > > +  PRUint32 length;
> > > +  encoder->Available(&length);
> > > 
> > > I would just choose a buffer size, say 10000.
> > 
> > I don't really know the right way to do this.  I was mostly copying code.  I
> > can change it to a fixed size buffer plus a loop, though.
> 
> There already is a loop, inside WriteFrom. I think choosing the buffer size
> here only affects performance. 

WriteFrom only writes a maximum of the number of bytes I pass to it.  I suppose I could use PRUINT32_MAX, but I don't see any other callers doing that.

Comment 9

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

Oops, you're right. I don't know what I was thinking.

Comment 10

[Eli Friedman]

(In reply to comment #7)
> > Actually, it can't render to anything; stuff starts asserting like crazy if you
> > try to render to a generic image surface.
> 
> Yeah, I'm guessing text fails, at least on Windows: but that's just a nasty bug
> and should be fixed.

Is there a bug filed on this?

> > > Speaking of which, perhaps we should just add a "aPaintCaret" parameter to
> > > nsLayoutUtils::PaintFrame and call it from PresShell::Render?
> > 
> > I suppose; it's not a huge simplification, though.
> 
> Still, I'd rather reduce the code duplication.
> 

I would need to add aIgnoreViewportScrolling support, which would make it quite a bit more complicated; not really worth messing with.

New patch soon.

Comment 11

[Eli Friedman]

Attachment: Patch v2

Comment 12

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

(In reply to comment #10)
> Is there a bug filed on this?

Not that I know of.

Comment 13

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

+  // XXX vlad says this shouldn't both be COLOR_ALPHA but that it is a workaround for some bug

What bug? I'd kinda prefer to write the right code here and fix the underlying bug.

+  // the original operator will be present when we PopGroup

Will it? I don't see how...

Why are you getting rid of the EnterPresShell call?

+  // The document expects to insert document stylesheets itself
+#if 0

Why are you removing this?

I think this is going to introduce warnings about an unused static function for everyone without DEBUG_Eli. You should probably wrap the definition in the same #ifdef.

You should probably add a comment to RenderDocument in nsPresShell.h that the caller should consider doing some flushing before calling this function.

Comment 14

[Eli Friedman]

(In reply to comment #13)
> +  // XXX vlad says this shouldn't both be COLOR_ALPHA but that it is a
> workaround for some bug
> 
> What bug? I'd kinda prefer to write the right code here and fix the underlying
> bug.

I would fix it if I knew... the code is adapted from http://lxr.mozilla.org/seamonkey/source/content/canvas/src/nsCanvasRenderingContext2D.cpp#2633
(I have the change in my tree to switch that over to RenderDocument, but I haven't tested it yet.)  Comment from Bug 342366, but that change wasn't discussed in the bug.

> +  // the original operator will be present when we PopGroup
> 
> Will it? I don't see how...

I didn't originally write it. (See above.) Comment from Bug 336331, the original checkin of this code.

> Why are you getting rid of the EnterPresShell call?

It's not really needed, but I'll add it back in.

> +  // The document expects to insert document stylesheets itself
> +#if 0
> 
> Why are you removing this?

That's VERIFY_REFLOW only; the document asserts while every time if that code is left in.

> I think this is going to introduce warnings about an unused static function for
> everyone without DEBUG_Eli. You should probably wrap the definition in the same
> #ifdef.

Okay.

> You should probably add a comment to RenderDocument in nsPresShell.h that the
> caller should consider doing some flushing before calling this function.

Okay.

CCing vlad and stuart in case they have anything to say.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

it should be COLOR_ALPHA now -- it couldn't be before because we couldn't draw native theme to surfaces with alpha reliably, but that should be fixed now.

Comment 16

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

(In reply to comment #14)
> I would fix it if I knew... the code is adapted from

OK, let's make it CONTENT_COLOR for the alpha == 0xff case and if any problems remain, we'll find them. Frankly I'd expect CONTENT_COLOR to work better than CONTENT_COLOR_ALPHA ... (In comment #15 was Vlad thinking the current code is CONTENT_COLOR on both paths?)

> > +  // the original operator will be present when we PopGroup
> > 
> > Will it? I don't see how...
> 
> I didn't originally write it. (See above.) Comment from Bug 336331, the
> original checkin of this code.

I think you should move the Save and Restore inside the Push/PopGroup to make that comment correct.

> > Why are you getting rid of the EnterPresShell call?
> 
> It's not really needed, but I'll add it back in.

Yeah. You're not drawing the caret so it's not needed right now but I'd prefer to have it in case we add more stuff to EnterPresShell.

> > +  // The document expects to insert document stylesheets itself
> > +#if 0
> > 
> > Why are you removing this?
> 
> That's VERIFY_REFLOW only; the document asserts while every time if that code
> is left in.

OK

Comment 17

[Eli Friedman]

Attachment: Patch v3

Okay, I think this addresses everything.

I have the changes in my to switch nsCanvasRenderingContext2d to this code; I think I'll do that as a followup.

Comment 18

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

Comment on attachment 265583 [details] [diff] [review]
Patch v3

+  NS_IMETHOD RenderDocument(nsRect aRect, PRBool aUntrusted,

make that a const nsRect&

Can you add a comment to nsIPresShell.h that callers may want to call FlushNotifications first.

Comment 19

[Eli Friedman]

Checked in.

Comment 20

[Andrzej Zaborowski]

Attachment: Remove the now unimplemented DumpContentToPPM() member.

What's the alternative to MOZ_FORCE_PAINT_AFTER_ONLOAD?  This change has just hit the regular users with Firefox3 release and everyone is starting to notice that programs that used Firefox to dump rendered pages to .ppm's (such as YWMS plugin for JOSM) stopped working.

In case the answer is basically that the users of MOZ_FORCE_PAINT_AFTER_ONLOAD should not have used firefox in the first place, I'm attaching a diff to remove the stray DumpContentToPPM() declaration now unused that was forgotten in this change.

Comment 21

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

Use an embedding tool like gnome-web-photo.

Or write a Firefox extension or a .xul app (launched using -chrome) that loads pages, snapshots them using canvas.drawWindow, and uses toDataURL to produce a PNG image.

Comment 22

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

Comment on attachment 326660 [details] [diff] [review]
Remove the now unimplemented DumpContentToPPM() member.

thanks for this.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 326660 [details] [diff] [review]
Remove the now unimplemented DumpContentToPPM() member.

http://hg.mozilla.org/index.cgi/mozilla-central/rev/9667fa5213da