621236 - Speedup canvas by PremultiplyImageSurface directly into locked Texture

Status: UNCONFIRMED

(Core :: Graphics, defect)

Description

[tom brinkman]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.12) Gecko/20101027 Ubuntu/10.04 (lucid) Firefox/3.6.12
Build Identifier: 

Upstream patch from harmattan

Reproducible: Always

Comment 1

[tom brinkman]

Attachment: Upstream patch from harmattan

Comment 3

[Tuukka Tolvanen (sp3000)]

I guess this patch is for bug 621235 and this bug was supposed to have http://hg.mozilla.org/users/romaxa_gmail.com/workbase_prc2/file/9f86415c6f09/bugXXXXX_canvas_speedup_maemo6.diff

Comment 4

[Oleg Romashin (:romaxa)]

attempt to speedup canvas gl rendering with remote fennec:
1) readPixels directly into shared memory surface
2) PremultiplyImageSurface from sharedMemory -> locked Texture

Comment 5

[tom brinkman]

Attachment: Upstream patch from harmattan

Upstream patch from harmattan

Comment 6

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

Comment on attachment 500891 [details]
Upstream patch from harmattan

>diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp
>--- a/gfx/layers/basic/BasicLayers.cpp
>+++ b/gfx/layers/basic/BasicLayers.cpp
>@@ -1966,18 +1966,20 @@ BasicShadowableCanvasLayer::Updated(cons
>     // Put back the previous framebuffer binding.
>     if (currentFramebuffer != mCanvasFramebuffer)
>       mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, currentFramebuffer);
> 
>     // If the underlying GLContext doesn't have a framebuffer into which
>     // premultiplied values were written, we have to do this ourselves here.
>     // Note that this is a WebGL attribute; GL itself has no knowledge of
>     // premultiplied or unpremultiplied alpha.
>+#if (MOZ_PLATFORM_MAEMO != 6)
>     if (!mGLBufferIsPremultiplied)
>       gfxUtils::PremultiplyImageSurface(mBackBuffer);
>+#endif

We already have mGLBufferIsPremultiplied -- why is that not set correctly on Maemo6?

>diff --git a/gfx/layers/opengl/CanvasLayerOGL.cpp b/gfx/layers/opengl/CanvasLayerOGL.cpp
>--- a/gfx/layers/opengl/CanvasLayerOGL.cpp
>+++ b/gfx/layers/opengl/CanvasLayerOGL.cpp

>     nsIntRegion updateRegion(nsIntRect(0, 0, sz.width, sz.height));
>+#if (MOZ_PLATFORM_MAEMO == 6)
>+    nsRefPtr<gfxContext> dest = mTexImage->BeginUpdate(updateRegion);
>+    gfxImageSurface* image = static_cast<gfxImageSurface*>(aNewFront);
>+    gfxImageSurface* destImage = static_cast<gfxImageSurface*>(dest->OriginalSurface());

Doing this cast without checking whether the surface is an image surface is dangerous, and will cause breakage if that changes.  At the very least, this needs to assert that the surface type is an image surface.

Comment 7

[tom brinkman]

Attachment: Added two asserts to previous patch

>> Doing this cast without checking whether the surface is an image surface

Agreed.  static_cast conversions are not as safe as dynamic_cast conversions, because static_cast does no run-time type check, while dynamic_cast does. A dynamic_cast to an ambiguous pointer will fail, while a static_cast returns as if nothing were wrong; this can be dangerous. Although dynamic_cast conversions are safer, dynamic_cast only works on pointers or references, and the run-time type check is an overhead.

Per your instructions, I added two asserts.

>> We already have mGLBufferIsPremultiplied -- why is that not set correctly

Oleg might have a better answer, but this is just a minor change needed to accomodate MAEMO6 related optimizations.

Comment 8

[tom brinkman]

Attachment: Added asserts

Comment 9

[tom brinkman]

Attachment: Added asserts and fixed reviewer requestee

Comment 10

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

(In reply to comment #7)
> >> We already have mGLBufferIsPremultiplied -- why is that not set correctly
> 
> Oleg might have a better answer, but this is just a minor change needed to
> accomodate MAEMO6 related optimizations.

It's not really minor -- it's ignoring a member variable that other code is expected to have set correctly.  So either other code doesn't have it set correctly, or this needs an explanation as to why it's not correct for Maemo...

Comment 11

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

Comment on attachment 503601 [details] [diff] [review]
Added asserts and fixed reviewer requestee

Ugh, wait, I didn't see this portion:


>+#if (MOZ_PLATFORM_MAEMO == 6)
>+    nsRefPtr<gfxContext> dest = mTexImage->BeginUpdate(updateRegion);
>+
>+    gfxImageSurface* image = static_cast<gfxImageSurface*>(aNewFront);
>+    NS_ASSERTION(image, "static_cast of aNewFront failed");
>+
>+    gfxImageSurface* destImage = static_cast<gfxImageSurface*>(dest->OriginalSurface());
>+    NS_ASSERTION(destImage, "static cast of dest->OriginalSurface failed");
>+
>+    gfxUtils::PremultiplyImageSurface(image, destImage);
>+    mTexImage->EndUpdate();
>+#else

This is not correct.  You can't assert the results of a static_cast; it's static, not a dyanmic cast.  We can't use dynamic_cast because we don't build with RTTI, and a static_cast will always succeed because the two types have a common class hierarchy (static_cast is evaluated at compile time -- it'll either compile successfully or it won't; only way the result could be null is the value that was being casted was null).  The correct way to write the two requested asserts is something like:

NS_ASSERTION(aNewFront->GetType() == gfxASurface::SurfaceTypeImage, "surface is wrong type");
NS_ASSERTION(dest->OriginalSurface()->GetType() == gfxASurface::SurfaceTypeImage, "surface is wrong type");