Closed Bug 1114333 Opened 5 years ago Closed 5 years ago
Image Data(image Data, 0, 0, 0, 0, 1, 1) unreasonably slow
806 bytes, text/html
169.09 KB, text/x-c++src
1.26 KB, text/html
3.51 KB, patch
|Details | Diff | Splinter Review|
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
Component: Untriaged → Canvas: 2D
Product: Firefox → Core
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.
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!
Sure, here you are :)
Attachment #8582264 - Attachment is patch: true
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.
Here's the patch generated generated against current m-c. Jeff, can you look this over for Christos?
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
You need to log in before you can comment on or make changes to this bug.