Closed Bug 561168 Opened 10 years ago Closed 10 years ago

convert Canvas to use Layers for rendering

Categories

(Core :: Canvas: 2D, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

References

Details

Attachments

(4 files, 8 obsolete files)

129.42 KB, patch
roc
: review+
bas.schouten
: review+
Details | Diff | Splinter Review
248.17 KB, patch
Details | Diff | Splinter Review
4.46 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.81 KB, patch
roc
: review+
Details | Diff | Splinter Review
Canvas needs to use a custom Layer for rendering, so that the compositing can be hardware accelerated where possible (e.g. WebGL).
Attached patch first pass (obsolete) — Splinter Review
Here's the first pass at this; posting up here to get feedback, because for some reason invalidation doesn't work.  The canvas is drawn at the correct spot, but only get invalidates if I scroll around and reveal the new areas.

However, with layers, scrolling is broken in general, so is this just more of the same problem?
Assignee: nobody → vladimir
I don't think so, normal scrolling is broken because ScrollWindowEx doesn't work. It should in theory have nothing to do with invalidation.
Move GetLayerManagerForDoc to nsLayoutUtils and share it please!

+  nsIntRect mUpdatedRect;

You don't need this. Updated can be a no-op for BasicCanvasLayer.
- Implement basic CanvasLayer (gfxASurface only)
- Teach 2D context how to return a CanvasLayer
- move nsHTMLCanvasElement.h into public dir
- teach HTMLCanvasFrame how to render using layers
Attachment #440844 - Attachment is obsolete: true
Attachment #443133 - Flags: review?(roc)
Attachment #443133 - Flags: review?(bas.schouten)
Attached patch patch 2: remove nsICanvasElement (obsolete) — Splinter Review
- move nsICanvasElement functionality into nsHTMLCanvasElement
- create nsICanvasElementExternal, for outside-of-layout consumers
Attachment #443134 - Flags: review?(roc)
Attachment #443134 - Attachment is patch: true
Attachment #443134 - Attachment mime type: application/octet-stream → text/plain
Attached patch patch 3: WebGL/pbuffers on OSX (obsolete) — Splinter Review
- fix some refcounting issues
- add support for creating pbuffers on OSX
- move WebGL to GLContext pbuffers on OSX
Attachment #443135 - Flags: review?(roc)
Attachment #443135 - Flags: review?(bas.schouten)
Attached patch patch 4: add WGL pbuffer support (obsolete) — Splinter Review
- add GLContext pbuffer support for Win32, use it in WebGL
Attachment #443136 - Flags: review?(roc)
Attachment #443136 - Flags: review?(bas.schouten)
Attached patch patch 5: cleanup (obsolete) — Splinter Review
A bunch of cleanup from earlier patches; remove obsolete WebGL pbuffer interfaces, remove non-layers code paths from canvas.

Things that are still to do:
- Linux pbuffers
- Use the context attribs in pbuffers better, and teach webgl how to request the proper ones
- Check callers of RenderContextsExternal (such as d&d code) to make sure that they still work
Attachment #443137 - Flags: review?(roc)
Attached patch patch 5: cleanup (obsolete) — Splinter Review
(Forgot to qrefresh patch 5 before attaching, this is the real one)
Attachment #443137 - Attachment is obsolete: true
Attachment #443138 - Flags: review?(roc)
Attachment #443137 - Flags: review?(roc)
Your lack of 'a' prefixes on parameters makes me itch. I will let it slide on the canvas rendering context code, but not elsewhere in content/ or in layers.

+/* XXX move into nsContentUtils */

This needs to actually move into nsContentUtils :-).

Also the version in nsHTMLMediaElement has improved, so you should move that one to nsContentUtils.

+    PRBool mIsOpaque;

We don't need this. You can just use mIsOpaqueContent/SetIsOpaqueContent that the layer already has.

+  virtual void Initialize(const Data& aData) = 0;
+  virtual void Updated(const nsIntRect& aRect) = 0;

Document these --- and CanvasLayer itself.

+  NS_ASSERTION(mUpdatedRect.IsEmpty(), "CanvasLayer::Updated called more than once during a transaction!");
+  NS_ASSERTION(mUpdatedRect.IsEmpty() || mBounds.Contains(mUpdatedRect), "CanvasLayer: Updated rect bigger than bounds!");

Break these lines ... think of the teletypes!

+  nsIntRect mBounds;

We only need an nsIntSize here, right?

+  nsIntRect mUpdatedRect;

We don't need this in BasicLayers, so I would just not have it at all.

Shouldn't we be calling Updated() somewhere?

+#if 0
   void PaintVideo(nsIRenderingContext& aRenderingContext,
                    const nsRect& aDirtyRect, nsPoint aPt);
-                              
+#endif

Er ... lose this?
+ * This interface contains methods that are needed outside of the content/layout
+ * modules, specifically widget.  It should eventually go away when we support
+ * libxul builds, and nsHTMLCanvasElement be used directly.
+ *
+ * Code internal to content/layout should /never/ use this interface; if the
+ * same functionality is needed in both places, two separate methods should be
+ * used.

I think the correct thing to do would be to move Aero Peek out of widget. We could put the interfaces in xpcom/system and the implementation in toolkit/system.
Oops, disregard comment #11. I see we need that for the drag code, not Aero Peek.
Patch 2 contains your LayerManagerOGL canvas implemention, shouldn't that be in patch 1 or its own patch?
Will this help bug 520776?
Bas, roc, do you guys mind if I just merge all the patches?  (With the large file removal patch kept separate still.)  Would be easier to make changes as the patches really build on top of one another and aren't discrete chunks of work.

Martijn, nope, that's an issue with Linux.
Comment on attachment 443133 [details] [diff] [review]
patch 1: basic CanvasLayer + 2D code

>@@ -416,6 +423,24 @@ protected:
>   Layer* mFirstChild;
> };
> 
>+class THEBES_API CanvasLayer : public Layer {
>+public:
>+  struct Data {
>+    gfxASurface* mSurface;
>+    //GLContext* mGLContext;

Presumably debug code :-)

>+
>+  if (!aData.mSurface) {
>+    NS_ERROR("CanvasLayer created with null mSurface, what's the point?");

We probably want to either return here, in case we want this check in release builds (I suspect we do), or assert, because right now this would be a pointless if in a release build.

>+void
>+BasicCanvasLayer::Updated(const nsIntRect& aRect)
>+{
>+  NS_ASSERTION(mUpdatedRect.IsEmpty(), "CanvasLayer::Updated called more than once during a transaction!");
>+
>+  mUpdatedRect.UnionRect(mUpdatedRect, aRect);

If we require it to be empty, wouldn't assigning be cheaper than Union? It would save us a little bit of if-ing in UnionRect.
Attachment #443133 - Flags: review?(bas.schouten) → review+
(In reply to comment #15)
> Bas, roc, do you guys mind if I just merge all the patches?  (With the large
> file removal patch kept separate still.)  Would be easier to make changes as
> the patches really build on top of one another and aren't discrete chunks of
> work.

I'd actually like that. It would make it easier to review as well.
Generally a single big patch is harder to review, but if it's a lot of work to produce cleanly separated patches, I'm willing to review a big patch.
Attached patch updated and marged (obsolete) — Splinter Review
This merges the patches and updates them with review comments.  It can largely be reviewed in toplevel chunks -- that is, per directory/directory group.

mUpdatedRect and friends are easier to keep as a rect than a Size due to unioning/clipping/etc.  In the future I think it might be possible to call updated more than once before finalization, hence the checks, though maybe I need to think through that a little more.
Attachment #443133 - Attachment is obsolete: true
Attachment #443134 - Attachment is obsolete: true
Attachment #443135 - Attachment is obsolete: true
Attachment #443136 - Attachment is obsolete: true
Attachment #443138 - Attachment is obsolete: true
Attachment #444506 - Flags: review?(roc)
Attachment #444506 - Flags: review?(bas.schouten)
Attachment #443133 - Flags: review?(roc)
Attachment #443134 - Flags: review?(roc)
Attachment #443138 - Flags: review?(roc)
Attachment #443135 - Flags: review?(roc)
Attachment #443135 - Flags: review?(bas.schouten)
Attachment #443136 - Flags: review?(roc)
Attachment #443136 - Flags: review?(bas.schouten)
-        persist="screenX screenY width height sizemode"> 
+        persist="screenX screenY width height sizemode"
+	>

Discard this hunk

@@ -1781,6 +1799,7 @@ public:
   ~nsAutoScriptBlocker() {
     nsContentUtils::RemoveScriptBlocker();
   }
+

and this one

@@ -5980,3 +6031,4 @@ nsIContentUtils::IsSafeToRunScript()
 {
   return nsContentUtils::IsSafeToRunScript();
 }
+

and this one

+  NS_IMETHOD RenderContextsExternal (gfxContext *ctx, gfxPattern::GraphicsFilter aFilter) = 0;

unnecessary space before (

 WebGLContext::Render(gfxContext *ctx, gfxPattern::GraphicsFilter f)
 {
...
+    return NS_ERROR_NOT_IMPLEMENTED;

Hmm ... shouldn't this be fixed before checking in? I assume this gets called when we use the canvas as an image source via nsLayoutUtils::SurfaceForElement?

+    //    if (mCanvasLayer)
+    //        return mCanvasLayer;

Might as well remove this

+    // This flush is required
+    MakeContextCurrent();
+    gl->fFlush();

why is it required?

+    nsRefPtr<layers::CanvasLayer> mCanvasLayer;

Not used, remove. Even with retained layers, you will not be remembering your layer directly, you'll ask the FrameLayerBuilder (new class) for your old layer.

+    canvasLayer->SetIsOpaqueContent(PR_FALSE);

Should probably respect the canvas opaque attribute. Maybe instead of the canvas context exporting GetCanvasLayer, it should be GetCanvasData, returning a CanvasLayer::Data that the caller creates a layer for? Then the rest of the layer initialization can be shared across contexts.

+    nsRefPtr<CanvasLayer> mCanvasLayer;

Don't have this here in the 2D context either. Just create a new one every time.

+      nsCOMPtr<nsIContent> content = do_QueryInterface(static_cast<nsIDOMHTMLCanvasElement*>(mCanvasElement));
+    nsCOMPtr<nsIContent> content = do_QueryInterface(static_cast<nsIDOMHTMLCanvasElement*>(mCanvasElement));
+    nsCOMPtr<nsIContent> content = do_QueryInterface(static_cast<nsIDOMHTMLCanvasElement*>(mCanvasElement));

teletype issue

+    //    if (mCanvasLayer)
+    //        return mCanvasLayer;

remove

+  } else {
+    nsRect r = frame->GetRect();
+    r.x = r.y = 0;
+    frame->Invalidate(r);

Should invalidate just the content area, so invalidate GetContentRect() - GetPosition().

nsHTMLCanvasElement::GetCanvasLayer should take a layer manager as a parameter instead of using LayerManagerForDocument. You might be asked to create a canvas layer for a different layer manager than the one being used to render the browser window (e.g., if someone does drawWindow to an arbitrary Thebes context).

+  struct Data {
+      Data()
+  : mSurface(nsnull), mGLContext(nsnull)
+      { }

Fix indent!!

+   * CONSTRUCTION PHASE ONLY
+   * Initialize this CanvasLayer with the given data.  The data must
+   * have either mSurface or mGLContext initialized (but not both), as
+   * well as mSize.
+   */

Say that this can only be called once?

+   * CONSTRUCTION PHASE ONLY
+   * Notify this CanvasLayer that the rectangle given by aRect
+   * has been updated, and any work that needs to be done
+   * to bring the contents from the Surface/GLContext to the
+   * Layer in preparation for compositing should be performed.
+   */

Say that the canvas buffer contents must not be modified during the layer transaction? That gives the implementation the flexiblity to avoid copying buffer data unless/until it needs it after the end of the transaction.

+  if (aData.mSurface) {
+    mSurface = aData.mSurface;

Assert here that aData.mGLContext is null

+  mSurface = aData.mSurface;

delete

+  if (mGLContext) {
+    nsRefPtr<gfxImageSurface> isurf =
+      new gfxImageSurface(gfxIntSize(mBounds.width, mBounds.height),
+                          IsOpaqueContent() ? gfxASurface::ImageFormatRGB24 : gfxASurface::ImageFormatARGB32);
+    if (!isurf || isurf->CairoStatus() != 0) {
+      return;
+    }
+
+    mGLContext->MakeCurrent();
+    mGLContext->fReadPixels(0, 0, mBounds.width, mBounds.height,
+                            LOCAL_GL_BGRA, LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV,
+                            isurf->Data());

Shouldn't we only be reading back mUpdatedRect, or something like that? Is that future work? Probably deserves a comment at least.

I think the readback should happen during ::Paint, though (if mUpdatedRect is non-empty).

+    //gfxUtils::PermultiplyImageSurface(isurf);

What's this about? Do we need to premultiply?

+CanvasLayerOGL::Initialize(const Data& aData)
+{
+  NS_ASSERTION(mSurface == nsnull, "BasicCanvasLayer::Initialize called twice!");
+
+  mUpdatedRect.Empty();

Shouldn't need to set the rect to empty here, it starts empty

I'm sad that we need platform-specific pbuffer code here, but I know nothing about this so I have nothing else to suggest.

Like for BasicLayers, I think the CanvasLayerOGL code should defer all the real work until RenderLayer in case there are multiple Updated calls. Seems simpler that way anyway.

+      fprintf (stderr, "Error: %d %x %p\n", err, err, (void*) mGLContext->GetNativeData(GLContext::NativePBuffer)); fflush(stderr);
+const GLContextProvider::ContextFormat GLContextProvider::ContextFormat::BasicRGBA32Format(GLContextProvider::ContextFormat::BasicRGBA32);
+#define A2_(_x,_y)  do { attribs.AppendElement((CGLPixelFormatAttribute) _x); attribs.AppendElement((CGLPixelFormatAttribute) _y); } while(0)

wrap somewhere

+LibrarySymbolLoader::LoadSymbols(SymLoadStruct *firstStruct, bool tryplatform, const char *prefix)

PRBool

Looks like all the GLContext code needs to be using a* parameter names

+    mInitialized = (bool) LoadSymbols(&symbols[0], trygl, prefix);

Just change mInitialized to PRBool

Personally I suspect the A1/A2 macros aren't worth it...

Shouldn't nsHTMLCanvasFrame::GetCanvasSize return 0,0 for errors?
Attached patch updated, v3Splinter Review
Updated with review comments.  Note that this omits all the pure-file removals.
Attachment #444506 - Attachment is obsolete: true
Attachment #444923 - Flags: review?(roc)
Attachment #444923 - Flags: review?(bas.schouten)
Attachment #444506 - Flags: review?(roc)
Attachment #444506 - Flags: review?(bas.schouten)
Oh, some comments on the patch:

> +    canvasLayer->SetIsOpaqueContent(PR_FALSE);
>
> Should probably respect the canvas opaque attribute. Maybe instead of the
> canvas context exporting GetCanvasLayer, it should be GetCanvasData, returning
> a CanvasLayer::Data that the caller creates a layer for? Then the rest of the
> layer initialization can be shared across contexts.

WebGL specifies a different mechanism for opaqueness (one that we'll want to transition our 2d stuff to as well); we just don't implement it yet.  So currently webgl canvases can't be opaque, hence why this is always PR_FALSE for now.

> +    //gfxUtils::PermultiplyImageSurface(isurf);
>
> What's this about? Do we need to premultiply?

Whoops, yes; I implemented gfxUtils::PremultiplyImageSurface based on the fast premult/unpremult code we had in Canvas 2D.  I couldn't convert that code to it since it actually premults/unpremults between an image surface and a raw buffer.  It could be fixed with a bit of wrapping of the raw data in an image surface, but I didn't do that here.
Comment on attachment 444923 [details] [diff] [review]
updated, v3

+  NS_IMETHOD RenderContextsExternal (gfxContext *ctx, gfxPattern::GraphicsFilter aFilter) = 0;

Unnecessary space before (

+    nsRefPtr<gfxImageSurface> surf = new gfxImageSurface(gfxIntSize(mWidth, mHeight),
+                                                         gfxASurface::ImageFormatARGB32);
+    if (surf->CairoStatus() != 0)
+        return NS_ERROR_FAILURE;
 
-        int bwidth = mGLPbuffer->Width();
-        int bheight = mGLPbuffer->Height();
+    MakeContextCurrent();
+    gl->fReadPixels(0, 0, mWidth, mHeight,
+                    LOCAL_GL_BGRA,
+                    LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV,
+                    surf->Data());

Can you ASSERT that surf->Stride() is what you expect?

+    nsRefPtr<layers::CanvasLayer> mCanvasLayer;

You removed all uses of mCanvasLayer, but you still need to remove this declaration.

+    if (!LibrarySymbolLoader::LoadSymbols(mOGLLibrary, &pbufferSymbols[0], (LibrarySymbolLoader::PlatformLookupFunction)fGetProcAddress)) {
+        // this isn't an error, just means that pbuffers aren't supported
+        fCreatePbuffer = nsnull;
+    }
+
+    if (!LibrarySymbolLoader::LoadSymbols(mOGLLibrary, &pixFmtSymbols[0], (LibrarySymbolLoader::PlatformLookupFunction)fGetProcAddress)) {

Teletype

+    // the future we should use mUpdatedRect, thoug hwith WebGL we don't

Fix typo
Attachment #444923 - Flags: review?(roc) → review+
Small followup -- fixes y-axis flip issues when rendering WebGL using BasicLayers.  WebGL is rendered correctly when using OpenGL layers, but normal 2d canvases will be flipped upside down -- I'll fix this next, but it's going to depend on bug 565435 being fixed, since I have to fiddle with texcoords.  Might take a day or two.  I don't think it's an issue for now, since we don't have accelerated rendering for the browser window currently.
Attachment #445006 - Flags: review?(roc)
Attachment #444923 - Flags: review?(bas.schouten) → review+
Attached patch filter followupSplinter Review
and one more followup -- the image-rendering mode was being ignored.  try server run going again, this was the only reftest that failed last time.
Attachment #445806 - Flags: review?(roc)
WebGLArray.h was removed from the EXPORTS in content/canvas/public/Makefile.in, but the file itself sits still in said directory and is an #include dependency for WebGLContextNotSupported.cpp. The build breaks due to not finding the header file. I commented #include WebGLArray.h out and WebGLContextNotSupported.cpp was compiled successfully. Dunno if that's the right fix as another possibility would be to add WebGLArray.h back to the EXPORTS in the Makefile. Please give me a hint that I can open a follow-up bug with a proper fix. I'm building on OS/2 w/o WebGL dissabled (of course), thanks.
Nope, that's the right fix -- missed that while I was munging removals.  Just pushed a fix.
(In reply to comment #29)

> http://hg.mozilla.org/mozilla-central/rev/652733ce0792

Sorry, it appears that you removed to much here:
weakld: error: Unresolved symbol (UNDEF) '_kDOMClassInfo_WebGLFramebuffer_interf
aces'.
weakld: info: The symbol is referenced by:
    E:\mozbuild4\mozilla\dom\base\jsdombase_s.lib(nsDOMClassInfo.o)
and so on

 DOMCI_DATA(CanvasRenderingContextWebGL, void)
-DOMCI_DATA(WebGLBuffer, void)
-DOMCI_DATA(WebGLTexture, void)
-DOMCI_DATA(WebGLProgram, void)
-DOMCI_DATA(WebGLShader, void)
-DOMCI_DATA(WebGLFramebuffer, void)
-DOMCI_DATA(WebGLRenderbuffer, void)

These lines have to be added back
Duplicate of this bug: 565321
Depends on: 570474
You need to log in before you can comment on or make changes to this bug.