285038 - Support offscreen rendering API in nsIViewManager

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

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

See http://weblogs.mozillazine.org/roc/archives/2005/03/visual_regressi.html

I'll attach a patch which adds a convenient RenderOffscreen API to
nsIViewManager. It also adds a user of that API in nsDocumentViewer to dump all
visited pages to PPM files, for debugging, testing or other purposes.

Comment 1

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

Attachment: fix

In the view manager, in addition to the actual new code, I've added support to
BuildDisplayList to disable clipping from views that are ancestors of the
rendered view. This is necessary to make whole-document snapshots work.

In nsDocumentViewer I've stuffed the code to interpret
MOZ_FORCE_PAINT_AFTER_ONLOAD as a file specification to dump serial-numbered
PPM images out to. I could put it somewhere else but I'm not sure where.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176502 [details] [diff] [review]
fix

>Index: view/public/nsIViewManager.h
>+   * @param aRect is the region to capture into the offscreen buffer

In what coordinates?  Presumably the coordinate system of the view, right (so
not relative to the top-left of the view, but relative to the coordinate system
origin)?  Document this, please.

>+   * @param aRenderedContext gets set to a rendering context whose offscreen
>+   * buffer can be locked to get the data. The buffer's size will be aRect's size.
>+   * The caller must clean it up by calling
>+   * cx->DestroyDrawingSurface(cx->GetDrawingSurface()).

Hmm...	So caller has to do that after locking, right?	Do they need to do that
if they never lock?  Or if locking fails?

Is there a reason that doesn't happen automagically when the last ref to the
rendering context is released?

>Index: view/src/nsView.cpp
> // Returns PR_TRUE iff we found a placeholder parent edge on the way
>+static PRBool ApplyClipRect(const nsView* aView, nsRect* aRect, PRBool aFollowPlaceholders,
>+                            nsIView* aStopAtView)

Document what this returns and what aStopAtView means (eg, is the clip rect of
aStopAtView itself applied?)

>Index: view/src/nsViewManager.cpp

>+NS_IMETHODIMP nsViewManager::RenderOffscreen(nsIView* aView, nsRect >+  rv = NewOffscreenContext(mContext, surface, aRect, getter_AddRefs(localcx));
>+  if (NS_FAILED(rv))
>+    return NS_ERROR_FAILURE;

Do we need to deallocate the drawing surface here?

>+  PL_INIT_ARENA_POOL(&displayArena, "displayArena", 1024);

Can that fail?	Should we bail if it does?

>+  BuildRenderingDisplayList(view, nsRegion(aRect), &displayList, displayArena,
>+                            PR_TRUE, PR_TRUE);
>+  RenderViews(view, *localcx, nsRegion(aRect), PR_FALSE, displayList);

PR_FALSE for an nsIDrawingSurface* arg?  I know you copied this; fix this in
the other place too?

>+  NS_ADDREF(*aRenderedContext = localcx);

Why not use .swap()?

>Index: layout/base/nsDocumentViewer.cpp
>+  if (r.height > 150000)
>+    r.height = 150000;

Want to do this comparison in pixels, not twips?  I think that would be
clearer...

>+                  // err ENDIANNESS alert

Meaning what?  Should this have some big/little endian ifdefs?

>+  nsIURI *uri = mDocument->GetDocumentURI();
>+  nsCAutoString spec;
>+  if (uri) {
>+    uri->GetSpec(spec);

You sure you don't want GetAsciiSpec()?  I doubt you really want UTF-8 in your
file....

r+sr=bzbarsky with these issues addressed.

Comment 3

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

(In reply to comment #2)
> (From update of attachment 176502 [details] [diff] [review] [edit])
> >Index: view/public/nsIViewManager.h
> >+   * @param aRect is the region to capture into the offscreen buffer
> 
> In what coordinates?  Presumably the coordinate system of the view, right (so
> not relative to the top-left of the view, but relative to the coordinate
> system origin)?  Document this, please.

Yep

> >+   * @param aRenderedContext gets set to a rendering context whose offscreen
> >+   * buffer can be locked to get the data. The buffer's size will be aRect's
size.
> >+   * The caller must clean it up by calling
> >+   * cx->DestroyDrawingSurface(cx->GetDrawingSurface()).
> 
> Hmm...	So caller has to do that after locking, right?	Do they need to
> do that if they never lock?  Or if locking fails?

Yes, in all cases. I'll note that.

> Is there a reason that doesn't happen automagically when the last ref to the
> rendering context is released?

"Gfx sucks"

> >Index: view/src/nsView.cpp
> > // Returns PR_TRUE iff we found a placeholder parent edge on the way
> >+static PRBool ApplyClipRect(const nsView* aView, nsRect* aRect, PRBool
aFollowPlaceholders,
> >+                            nsIView* aStopAtView)
> 
> Document what this returns and what aStopAtView means (eg, is the clip rect of
> aStopAtView itself applied?)

It is, I'll note that.

> >Index: view/src/nsViewManager.cpp
> 
> >+NS_IMETHODIMP nsViewManager::RenderOffscreen(nsIView* aView, nsRect >+  rv =
NewOffscreenContext(mContext, surface, aRect, getter_AddRefs(localcx));
> >+  if (NS_FAILED(rv))
> >+    return NS_ERROR_FAILURE;
> 
> Do we need to deallocate the drawing surface here?

Yeah, good catch.

> >+  PL_INIT_ARENA_POOL(&displayArena, "displayArena", 1024);
> 
> Can that fail?	Should we bail if it does?

Apparently not.

> >+  BuildRenderingDisplayList(view, nsRegion(aRect), &displayList, displayArena,
> >+                            PR_TRUE, PR_TRUE);
> >+  RenderViews(view, *localcx, nsRegion(aRect), PR_FALSE, displayList);
> 
> PR_FALSE for an nsIDrawingSurface* arg?  I know you copied this; fix this in
> the other place too?

Actually I should be passing 'surface' in there so that opacity works.

> >+  NS_ADDREF(*aRenderedContext = localcx);
> 
> Why not use .swap()?

OK

> >Index: layout/base/nsDocumentViewer.cpp
> >+  if (r.height > 150000)
> >+    r.height = 150000;
> 
> Want to do this comparison in pixels, not twips?  I think that would be
> clearer...

It's an arbitrary limit either way. I guess I'll do it in pixels. I should also
limit the width too, I suppose. I'll do 5000 pixels each way.

> >+                  // err ENDIANNESS alert
> 
> Meaning what?  Should this have some big/little endian ifdefs?

Yes, in theory. I'll put them in. But I don't actually know what formats
different big-endian platforms might produce. All I know is that this code works
in 16 and 24bit GTK2.

> >+  nsIURI *uri = mDocument->GetDocumentURI();
> >+  nsCAutoString spec;
> >+  if (uri) {
> >+    uri->GetSpec(spec);
> 
> You sure you don't want GetAsciiSpec()?  I doubt you really want UTF-8 in your
> file....

OK

Comment 4

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

checked in

Comment 5

[Boris Zbarsky [:bzbarsky]]

> "Gfx sucks"

Want to file a bug on that?  ;)

Comment 6

[Aleksey Nogin]

This have introduced a warning to brad TBox:

+layout/base/nsDocumentViewer.cpp:873
+ `const char*status' might be used uninitialized in this function