Speedup canvas by PremultiplyImageSurface directly into locked Texture

UNCONFIRMED
Unassigned

Status

()

UNCONFIRMED
8 years ago
5 years ago

People

(Reporter: reportbase, Unassigned)

Tracking

Trunk
Other
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

8 years ago
Created attachment 499598 [details]
Upstream patch from harmattan
Duplicate of this bug: 621235
attempt to speedup canvas gl rendering with remote fennec:
1) readPixels directly into shared memory surface
2) PremultiplyImageSurface from sharedMemory -> locked Texture
Summary: Canvas speed → Speedup canvas by PremultiplyImageSurface directly into locked Texture
Component: General → Graphics
Product: Fennec → Core
QA Contact: general → thebes
(Reporter)

Comment 5

8 years ago
Created attachment 500891 [details]
Upstream patch from harmattan

Upstream patch from harmattan
Attachment #499598 - Attachment is obsolete: true
(Reporter)

Updated

8 years ago
Depends on: 619056
Blocks: 619056
No longer depends on: 619056
(Reporter)

Updated

8 years ago
Attachment #500891 - Flags: review?(vladimir)
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.
Attachment #500891 - Flags: review?(vladimir) → review-
(Reporter)

Comment 7

8 years ago
Created attachment 503594 [details] [diff] [review]
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.
Attachment #500891 - Attachment is obsolete: true
(Reporter)

Comment 8

8 years ago
Created attachment 503595 [details] [diff] [review]
Added asserts
Attachment #503594 - Attachment is obsolete: true
Attachment #503595 - Flags: review?
(Reporter)

Comment 9

8 years ago
Created attachment 503601 [details] [diff] [review]
Added asserts and fixed reviewer requestee
Attachment #503595 - Attachment is obsolete: true
Attachment #503601 - Flags: review?(vladimir)
Attachment #503595 - Flags: review?
(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 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");
Attachment #503601 - Flags: review?(vladimir) → review-
You need to log in before you can comment on or make changes to this bug.