Closed Bug 1114333 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Canvas: 2D, defect, major)

34 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: 8998628+bugzilla, Assigned: christos.alewa)

Details

(Keywords: perf, testcase)

Attachments

(4 files, 2 obsolete files)

Attached file 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
Severity: normal → major
Component: Untriaged → Canvas: 2D
Product: Firefox → Core
OS: Linux → All
Hardware: x86_64 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: perf, testcase
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.
Attached file Here is the diff of the sources.. (obsolete) —
Attachment #8580590 - Flags: review?(vladimir)
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!
Attached patch Unified_diff (obsolete) — Splinter Review
Sure, here you are :)
Attachment #8580591 - Attachment is obsolete: true
Attachment #8582264 - Flags: review?(vladimir)
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
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.
Attached patch patchSplinter Review
Here's the patch generated generated against current m-c.  Jeff, can you look this over for Christos?
Attachment #8582264 - Attachment is obsolete: true
Attachment #8582264 - Flags: review?(vladimir)
Attachment #8582397 - Flags: review?(jmuizelaar)
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;
Attachment #8582397 - Flags: review?(jmuizelaar) → review+
Without accounting for Bas' comments, the original patch looks pretty good on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea033bb32cf8
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
Sorry Christos, dropped the ball on this.

The patch still applies cleanly, good try from comment 10, we can land.
Assignee: nobody → christos.alewa
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aaa0f79dfefe
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.