Closed Bug 293947 Opened 19 years ago Closed 19 years ago

Add offscreen rendering DOM API

Categories

(Core :: Graphics: Canvas2D, enhancement)

Product:
Core
Component:
Graphics: Canvas2D
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

Details

Attachments

(3 files, 1 obsolete file)

fix #2
39.33 KB, patch
Details | Diff | Splinter Review
fix #3
39.68 KB, patch
vlad
: review+
Details | Diff | Splinter Review
fix #4
24.75 KB, patch
vlad
: review+
darin.moz
: superreview+
asa
: approval1.8b2+
Details | Diff | Splinter Review 
I'm implemented this:
nsIDOMCanvasElement renderToCanvas(long x, long y, long w, long h, DOMString
bgColor);

Most of the code is in canvas --- apart from the fact that I think rendering
into a canvas is good for API users, I'm reusing a lot of canvas code. To make
this a bit more useful I also implemented support for canvas DrawImage() to take
a canvas argument.

This is restricted to chrome callers for now.
Attached patch fix (obsolete) — — Splinter Review 
This is it!
Attachment #183437 - Flags: superreview?(darin)
Attachment #183437 - Flags: review?(vladimir)
Would it make sense from a performance point of view for this function to take a
parameter to pre-scale the resulting canvas?  I wonder if all those RGBA pixels
will make this slow for applications that just want a relatively small thumbnail
of a window.
Note that this patch does produce translucent images as advertised if you feed
it a translucent window. It's pretty cool.

(In reply to comment #2)
> Would it make sense from a performance point of view for this function to take
> a parameter to pre-scale the resulting canvas?  I wonder if all those RGBA
> pixels will make this slow for applications that just want a relatively small
> thumbnail of a window.

Right now we have no way of rendering with a given scale factor, so all we can
do is to render at full size and then scale the bitmap down.

In the future we'll be able to render with a scale factor, but I doubt the
performance win will be big enough to make it worthwhile. We already repaint the
window often, once more or so is no big deal.
Of course it is important that we not keep lots of huge bitmaps lying around,
but this should be fine ... just use renderToCanvas to a big canvas, draw it
scaled-down into a small canvas, and throw away the big canvas.
Comment on attachment 183437 [details] [diff] [review]
fix

>Index: content/canvas/src/nsCanvasRenderingContext2D.cpp
>+nsCanvasRenderingContext2D::CairoSurfaceFromElement(nsIDOMElement *imgElt,
>+                                                    cairo_surface_t **aCairoSurface,
>+                                                    PRUint8 **imgData,
>+                                                    PRInt32 *widthOut, PRInt32 *heightOut)
> {
> ...
>+    } else {
>+        // maybe a canvas
>+        nsCOMPtr<nsICanvasElement> canvas = do_QueryInterface(imgElt);
>+        if (canvas) {

Need to call canvas->UpdateImageFrame() here to ensure that the gfxIImageFrame
in
the container contains the latest data rendered to the cairo surface.

>+            canvas->GetCanvasImageContainer(getter_AddRefs(imgContainer));
>+        } else {
>+            return NS_ERROR_NOT_AVAILABLE;
>+        }
>+    }

r=vladimir with that change.. looking forward to trying it out!
Attachment #183437 - Flags: review?(vladimir) → review+
(In reply to comment #4)
> Of course it is important that we not keep lots of huge bitmaps lying around,
> but this should be fine ... just use renderToCanvas to a big canvas, draw it
> scaled-down into a small canvas, and throw away the big canvas.

Yes, this works.  But, it means that you will have no opportunity to optimize
for this "memory-hungry" use case later on.  If instead, you make the API
implementation do the scaling in the manner in which you suggest, then you could
optimize it in the future without having to change callers.
BTW, it would also be very nice to think about building a frozen API for this. 
I suspect that the DOM API to the canvas is far from being frozen, but having
stable (i.e., unchanging) APIs will definitely be a good thing for extension
authors.
Comment on attachment 183437 [details] [diff] [review]
fix

>Index: dom/public/idl/base/nsIDOMWindowInternal.idl

>+  /**
>+   * Renders a region of the window into a newly created canvas.  The
>+   * canvas will have width and height set to the given dimensions
>+   * (which are in CSS pixels). The given background color is used to
>+   * fill the canvas before rendering. The contents of the viewport
>+   * are rendered, ignoring viewport clipping and scrolling.
>+   *
>+   * Hints:
>+   * -- If 'rgba(0,0,0,0)' is used for the background color, the
>+   * result will be transparent wherever the window is transparent.
>+   * -- Top-level browsed documents are usually not transparent
>+   * because the user's background-color preference is applied,
>+   * but IFRAMEs are transparent if the page doesn't set a background.
>+   * -- If an opaque color is used for the background color, rendering
>+   * will be faster because we won't have to compute the window's
>+   * transparency.
>+   * -- You can use the canvas APIs with one or more additional
>+   * canvas elements to scale or otherwise transform the image.
>+   *
>+   * This API cannot currently be used by Web content. It is chrome
>+   * only.
>+   */
>+  nsIDOMHTMLCanvasElement renderToCanvas(in long x, in long y, in long w, in long h,
>+                                         in DOMString backgroundColor);
> };

It seems to me that this documentation is lacking.  Can you take
some time and document the parameters?	Also, I presume that the
resulting nsIDOMHTMLCanvasElement has no associated ownerDocument,
right?	What is the format of backgroundColor?

Also, you need to change the UUID of nsIDOMWindowInternal.


>Index: dom/src/base/nsGlobalWindow.cpp

> NS_IMETHODIMP
>+nsGlobalWindow::RenderToCanvas(PRInt32 aX, PRInt32 aY, PRInt32 aW, PRInt32 aH,
>+                               const nsAString& aBGColor,
>+                               nsIDOMHTMLCanvasElement** aResult)
>+{
>+#ifndef MOZ_ENABLE_CANVAS
>+  return NS_ERROR_FAILURE;

NS_ERROR_NOT_IMPLEMENTED seems better


>+  nsCOMPtr<nsIDOMHTMLCanvasElement> domElem = do_QueryInterface(elem);
>+  *aResult = domElem;
>+  NS_ADDREF(*aResult);
>+  return NS_OK;

This could be replaced with:

    return CallQueryInterface(elem, aResult);


>Index: content/canvas/src/nsCanvasRenderingContext2D.cpp

>+NS_IMETHODIMP
>+nsCanvasRenderingContext2D::StyleStringToColor(const nsAString& aString, nscolor* aColor)
>+{
>+    nsString str;
>+    str.Assign(aString);
>+    return mCSSParser->ParseColorString(str, nsnull, 0, PR_TRUE, aColor);
>+}

perhaps you should use PromiseFlatString instead.  that gives you a nsString
from a nsAString in the most efficient manner.	e.g.:

  const nsString &str = PromiseFlatString(aString);


>+    rv = CairoSurfaceFromElement (imgElt, &imgSurf, &imgData, &imgWidth, &imgHeight);

nit: extra space between function name and opening paran seems out of place
t, PRInt32 *heightOut)


>+nsCanvasRenderingContext2D::CairoSurfaceFromElement(nsIDOMElement *imgElt,
...
>+        // maybe a canvas
>+        nsCOMPtr<nsICanvasElement> canvas = do_QueryInterface(imgElt);
>+        if (canvas) {
>+            canvas->GetCanvasImageContainer(getter_AddRefs(imgContainer));
>+        } else {
>+            return NS_ERROR_NOT_AVAILABLE;

should this case get a NS_WARNING?


>+NS_IMETHODIMP
>+nsCanvasRenderingContext2D::AssignFromNativeSurfaces(nsIDrawingSurface* aBlackSurface,
>+                                                     nsIDrawingSurface* aWhiteSurface,
>+                                                     nsIRenderingContext* aBlackContext)
>+{
>+    NS_ASSERTION(mImageFrame, "Must have image frame already");
>+    if (!mImageFrame)
>+        return NS_ERROR_FAILURE;

nit: you can also use NS_ERROR:

      if (!mImageFrame) {
	  NS_ERROR("Must have image frame already");
	  return NS_ERROR_FAILURE;
      }


>Index: content/html/content/src/nsHTMLCanvasElement.cpp

>+NS_IMETHODIMP
>+nsHTMLCanvasElement::RenderWindow(nsPresContext* aPresContext, const nsIntRect& aRect,
>+                                  const nsAString& aBGColor, PRBool aUntrusted)
>+{
>+  // XXX we should show some kind of error message here
>+  if (aRect.width < 0 || aRect.height < 0)
>+    return NS_ERROR_FAILURE;

do you mean a console error?  how about a NS_ERROR or NS_NOTREACHED
for now at least?


>+  nsIDrawingSurface* blackSurface;
>+  blackCtx->GetDrawingSurface(&blackSurface);
>+  if (!blackSurface)
>+    return NS_ERROR_FAILURE;
>+
>+  // Render it!
>+  if (NS_GET_A(bgColor) == 0xFF) {
>+    // opaque background. Do it the easy way.
>+    rv = internalctx->AssignFromNativeSurfaces(blackSurface, nsnull, blackCtx);
>+    blackCtx->DestroyDrawingSurface(blackSurface);
>+    return rv;
>+  }

it seems that it'd be nice if there were a stack based class that
would call DestroyDrawingSurface for you ;-)

Maybe something like this:

  nsAutoDrawingSurface blackSurface(blackCtx);


nothing but nits, sr=darin
Attachment #183437 - Flags: superreview?(darin) → superreview+
(In reply to comment #6)
> (In reply to comment #4)
> > Of course it is important that we not keep lots of huge bitmaps lying around,
> > but this should be fine ... just use renderToCanvas to a big canvas, draw it
> > scaled-down into a small canvas, and throw away the big canvas.
> 
> Yes, this works.  But, it means that you will have no opportunity to optimize
> for this "memory-hungry" use case later on.  If instead, you make the API
> implementation do the scaling in the manner in which you suggest, then you could
> optimize it in the future without having to change callers.


Hmm, I must have misunderstood the renderToCanvas args -- do the x/y/w/h params
specify the region of the window to be rendered to a canvas?  Or do they specify
the size of that region in the canvas? (I'd guess the former, as for the latter
you wouldn't need w/h.)  But maybe add canvasWidth/canvasHeight params to let
you specify desired scaling?


(In reply to comment #7)
> BTW, it would also be very nice to think about building a frozen API for this. 
> I suspect that the DOM API to the canvas is far from being frozen, but having
> stable (i.e., unchanging) APIs will definitely be a good thing for extension
> authors.

What form do you think such a frozen API should take?

> Hmm, I must have misunderstood the renderToCanvas args -- do the x/y/w/h
> params specify the region of the window to be rendered to a canvas?  Or do
> they specify the size of that region in the canvas? (I'd guess the former, as
> for the latter you wouldn't need w/h.)

The former.

> But maybe add canvasWidth/canvasHeight
> params to let you specify desired scaling?

Yeah, I'm convinced that's probably a good idea. I'll code it up, along with the
review comments.
> What form do you think such a frozen API should take?

Well, I suppose it depends on having a frozen HTMLCanvasElement, right?  Until
that is locked down (via some WhatWG recommendation), then there's no API to
freeze here.  Once that is frozen (and the associated rendering context
interface), then we should revisit how to get a HTMLCanvasElement from an
arbitrary Window (assuming nsIDOMWindowInternal will never be frozen).
There is a lot of stuff in nsIDOMWindowInternal that would be useful to freeze.
Attached patch fix #2 — — Splinter Review 
Updated patch.
Attachment #183437 - Attachment is obsolete: true
Attachment #183468 - Flags: superreview?(darin)
Attachment #183468 - Flags: review?(vladimir)
Comment on attachment 183468 [details] [diff] [review]
fix #2

>Index: content/canvas/public/nsICanvasElement.h
>   /**
>+   * Render the contents of the window aPresContext's viewport into this canvas.
>+   * @param aRect the dimensions of the area to render (in CSS pixels)
>+   * @param aBGColor the background color to draw behind the window
>+   */
>+  NS_IMETHOD RenderWindow(nsPresContext* aPresContext, const nsIntRect& aRect,
>+                          const nsIntSize& aCanvasSize,
>+                          const nsAString& aBGColor, PRBool aUntrusted) = 0;


So on second thought, now that aCanvasSize is here, I'm not sure if I like this
method on the canvas element -- it's essentially a rendering operation, not
much different than drawImage() on the 2D context.  The usage is also a little
strange, since that one function call does a lot of things at once (forcing
canvas size, creating a 2D context, as well as doing the rendering) which is
unlike how other canvas stuff works.

I'd suggest moving RenderWindow (maybe as drawWindow?) to the 2D Rendering
Context interface, so that it can get the natural cairo transform applied to it
(translate/scale/etc.), and to keep all rendering happening inside the context.
 Since the RenderToCanvas() would still be the common usage, as it can do the
canvas creation, width/height setting, and obtaining a 2D context for the
caller.

You mentioned on IRC that you didn't really want to have two separate API entry
points for this, though I think exposing DrawWindow() won't really hurt us with
an IsCallerChrome().  And someone might find a compelling use case for
rendering multiple windows directly into one canvas ;)
Attachment #183468 - Flags: superreview?(darin)
Attachment #183468 - Flags: review?(vladimir)
Attached patch fix #3 — — Splinter Review 
Moved the code around. nsIDOMCanvasRenderingContext2D::DrawWindow is now fully
scriptable, so I move the security check there.

See http://weblogs.mozillazine.org/roc/archives/2005/05/rendering_web_p.html
for a demo.
Attachment #183471 - Flags: superreview?(darin)
Attachment #183471 - Flags: review?(vladimir)
How about putting this method in its own interface instead of sticking it in
nsIDOMWindowInternal. How about a nsIDOMCanvasWindow or whatever, and then you
could make only nsGlobalChromeWindow implement it and expose the interface
through its classinfo and leave the web-page window namespace alone for now
(since we apparently don't know if this is a set API yet etc).
Ignore the nsGlobalChromeWindow part of that comment, but I still stand by my
separate interface for this stuff...
alright, I'll look at that next week.
Actually, if that's a concern, how about I just ditch the nsIDOMWindow method
completely?
Comment on attachment 183471 [details] [diff] [review]
fix #3


>Index: content/canvas/public/nsICanvasRenderingContextInternal.h

>+  NS_IMETHOD StyleStringToColor(const nsAString& aString, nscolor* aColor) = 0;
>+

Do we still need this exposed in the interface?  Would rather see it removed
from the interface, but keep it in RenderingContext2D as a separate function
and have it create mCSSParser if it's null, instead of doing the CSSParser
creation in SetCanvasElement.

>Index: content/html/content/src/nsHTMLCanvasElement.cpp
> 
>   // nsICanvasElement
>-  NS_IMETHOD GetCanvasImageContainer(imgIContainer **aImageContainer);
>-  NS_IMETHOD GetPrimaryCanvasFrame(nsIFrame **aFrame);
>+  NS_IMETHOD GetCanvasImageContainer (imgIContainer **aImageContainer);
>+  NS_IMETHOD GetPrimaryCanvasFrame (nsIFrame **aFrame);
>   NS_IMETHOD UpdateImageFrame();

Please don't add the space in before '(' (or bz will yell at you ;)

r=vladimir with those two nits fixed
Attachment #183471 - Flags: review?(vladimir) → review+
I think I'll go ahead and remove the nsGlobalWindow stuff. It's not a big deal
for users to do it from script.
>+NS_IMETHODIMP
>+nsCanvasRenderingContext2D::AssignFromNativeSurfaces(nsIDrawingSurface*

roc: This needs to return nsresult to compile on windows.


> roc: This needs to return nsresult to compile on windows.

It does, doesn't it?
Attached patch fix #4 — — Splinter Review 
Addresses nits. Removes the method from the DOM window object; now there's just
CanvasRenderingContext2D::drawWindow. This simplified the code quite a bit.
Attachment #183668 - Flags: superreview?(darin)
Attachment #183668 - Flags: review?(vladimir)
Comment on attachment 183668 [details] [diff] [review]
fix #4

r=vladimir

I have a handful of changes for fixing bugs in CairoSurfaceFrom(Image)Element,
but all the changes are below the chunks that you've modified.. so we shouldn't
have any conflicts.
Attachment #183668 - Flags: review?(vladimir) → review+
Comment on attachment 183668 [details] [diff] [review]
fix #4

>Index: dom/public/idl/canvas/nsIDOMCanvasRenderingContext2D.idl

>+  /**
>+   * Renders a region of a window into the canvas.  The contents of
>+   * the window's viewport are rendered, ignoring viewport clipping
>+   * and scrolling.a

typo ".a" ?


>+   * x, y, w, and h specify the area of the window to render, in CSS
>+   * pixels.

usually a good idea to use javadoc style @param for these


>+   * @param backgroundColor the canvas is filled with this color
>+   * before we render the window into it. This color may be be

"be be"


>Index: content/canvas/src/nsCanvasRenderingContext2D.cpp

>+nsCanvasRenderingContext2D::DrawWindow(nsIDOMWindow* aWindow, PRInt32 aX, PRInt32 aY,
...
>+    nsresult rv = ssm->SubjectPrincipalIsSystem(&isChrome);
>+    if (!isChrome)
>+        return NS_ERROR_FAILURE;

nit: why capture the return value of SubjectPrincipalIsSystem if you
aren't going to use it?


>+    nscolor bgColor;
>+    rv = mCSSParser->ParseColorString(nsString(aBGColor), nsnull, 0, PR_TRUE, &bgColor);

use PromiseFlatString(aBGColor) instead of nsString(aBGColor).
otherwise, you are just asking for a heap copy when a dependent
string may be sufficient.


>+    PRUint8* data;
>+    PRInt32 rowLen, rowSpan;
>+    rv = aBlackSurface->Lock(0, 0, aSurfaceSize.width, aSurfaceSize.height,
>+                             (void**)&data, &rowSpan, &rowLen,
>+                             NS_LOCK_SURFACE_READ_ONLY);
...
>+    if (!tmpSurf) {
>+        delete[] tmpBuf;
>+        delete[] alphas;
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    }

Should this error case call aBlackSurface->Unlock() ?

Have you considered using nsAutoArrayPtr for some of this to
simplify memory management?


sr=darin
Attachment #183668 - Flags: superreview?(darin) → superreview+
I'll fix all those. nsCanvasRenderingContext2D does nsString(...) all over the
place, BTW.
Comment on attachment 183668 [details] [diff] [review]
fix #4

land this feature. it doesn't touch anything much but canvas.
Attachment #183668 - Flags: approval1.8b2?
> nsCanvasRenderingContext2D does nsString(...) all over the place, BTW.

probably because the string API is so easy to understand and use properly ;-)
Comment on attachment 183668 [details] [diff] [review]
fix #4

a=asa
Attachment #183668 - Flags: approval1.8b2? → approval1.8b2+
checked in
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: