1114333 - putImageData(imageData, 0, 0, 0, 0, 1, 1) unreasonably slow

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[8998628+bugzilla]

Attachment: firefoxBug3.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141127111021

Steps to reproduce:

context.putImageData(imageData, 0, 0, 0, 0, 1, 1);


Actual results:

Running time is proportional to size of imageData, not to size of dirtyRectangle.


Expected results:

Running time should be proportional to the size of dirtyRectangle.
I am not terribly familar with firefox internals, but it looks like putImageData is iterating over the whole image regardless of the size of dirtyRectangle.
https://github.com/mozilla/gecko-dev/blob/1247620b1deaef4819482726b527f6c5a2d8bf29/dom/canvas/CanvasRenderingContext2D.cpp#L5101

Comment 1

[Christos Alewa]

Attachment: Modified source of CanvasRenderingContext2D.cpp

Hi,
with this modified source CanvasRenderingContext2D.cpp it iterates just over the requested size, not the whole image anymore, dramatically increasing the speed of firefox.
I'll upload the diff, the new source and a small Testprogam where you can define the area as you please as well.

Comment 2

[Christos Alewa]

Attachment: Here is the diff of the sources..

Comment 3

[Christos Alewa]

Attachment: Canvas-putImageData.html - Small Testprogram

Comment 4

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

Hi Christos, thanks for the patch!  Could you upload a unified diff (diff -u, or just the output from hg diff/git diff if you're working from the source tree)?  That way we'll be able to review it directly.  Thanks!

Comment 5

[Christos Alewa]

Attachment: Unified_diff

Sure, here you are :)

Comment 6

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

Great, thanks!  Hmm, are you looking at the latest Firefox source?  The current CanvasRenderingContext2D.cpp at tip looks very similar to what your patch does:  https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#5130

Comment 7

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

Oh, wait, the patch attached is just reversed (I assume you're adding the copyWidth/copyHeight loop and not the other way around, right?)  Let me flip it and find a proper reviewer.

Comment 8

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

Attachment: patch

Here's the patch generated generated against current m-c.  Jeff, can you look this over for Christos?

Comment 9

[Bas Schouten (:bas.schouten)]

Comment on attachment 8582397 [details] [diff] [review]
patch

Review of attachment 8582397 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +5116,5 @@
>    }
>  
> +  uint32_t copyWidth = dirtyRect.Width();
> +  uint32_t copyHeight = dirtyRect.Height();
> +  nsRefPtr<gfxImageSurface> imgsurf = new gfxImageSurface(gfxIntSize(copyWidth, copyHeight),

Eventually we should just create the DataSourceSurface here directly, that would be a lot better.

@@ +5125,5 @@
>    }
>  
> +  uint32_t copyX = dirtyRect.x - x;
> +  uint32_t copyY = dirtyRect.y - y;
> +  //uint8_t *src = aArray->Data();

nit: This line should probably go.

@@ +5155,5 @@
>        *dst++ = gfxUtils::sPremultiplyTable[a * 256 + g];
>        *dst++ = gfxUtils::sPremultiplyTable[a * 256 + b];
>  #endif
>      }
> +    srcLine += w * 4;

Probably to be completely correct here (although we did this wrong in the past as well apparently), we should add a dst += imgSurf->Stride() - w * 4;

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Without accounting for Bas' comments, the original patch looks pretty good on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea033bb32cf8

Comment 11

[Christos Alewa]

Hi everyone,
does anybody know if the workaround for this slow behaviour is about to be implemented?
As far as I am concerned with the alteration of the code the putImageData is significantly faster, that is why I'm asking.
Greetings,
Christos

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Sorry Christos, dropped the ball on this.

The patch still applies cleanly, good try from comment 10, we can land.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa0f79dfefe

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa0f79dfefe

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aaa0f79dfefe