[Chromium] Issue 58536: Fix Layout Test canvas/philip/tests/2d.imageData.put.alpha.html with --accelerated-2d-canvas.
Created attachment 81189 [details] Patch
Could you review my patch?
This is a bit outside my area of expertise.
Do you know who is the right person? (In reply to comment #3) > This is a bit outside my area of expertise.
You can try running "webkit-patch suggest-reviewers" on your patch to see which reviewers it suggests.
abarth@quadzen:~/svn/webkit3$ ./Tools/Scripts/webkit-patch suggest-reviewers Dan Bernstein David Hyatt Dirk Schulze Nikolas Zimmermann Oliver Hunt Simon Fraser James Robinson
Added a couple of people whom Adam recommended. Could you review my patch?
Is there anybody who can review my patch?
Comment on attachment 81189 [details] Patch I think this fix should go in ImageBufferSkia.cpp rather than in the common code. Conceptually, ImageBuffer::putUnmultipliedImageData is supposed to be unaffected by the CompositeOperator.
Thanks, Kenneth!! I also uploaded, https://bugs.webkit.org/show_bug.cgi?id=53857 If you have time, could you review it too? (In reply to comment #9) > (From update of attachment 81189 [details]) > I think this fix should go in ImageBufferSkia.cpp rather than in the common code. Conceptually, ImageBuffer::putUnmultipliedImageData is supposed to be unaffected by the CompositeOperator.
(In reply to comment #10) > Thanks, Kenneth!! > > I also uploaded, > https://bugs.webkit.org/show_bug.cgi?id=53857 > > If you have time, could you review it too? Reviewed. Looking forward to revised versions of these two patches to be able to r+ them. Please be sure to set the cq bit to cq? if you want these landed via the commit queue.
Created attachment 81578 [details] Patch
Comment on attachment 81578 [details] Patch Ok, I move the logic to ImageBufferSkia.cpp. Please review.
Comment on attachment 81578 [details] Patch Why should changing the composite operator have any effect on how putImageData works? If putImageData() is wrong, fix that don't patch putUnmultipliedImageData.
As I wrote in the change log and the comment, syncing between software and hardware doesn't work well if the composite operator is wrong. As you know, if there is no hardware usage, it looks working fine. But logically putImageData() is wrong because of wrong operator inside. That is my intention for the first patch. What do you think, Kenneth? If my intention is right, as James suggested, should we use the first patch? (In reply to comment #14) > (From update of attachment 81578 [details]) > Why should changing the composite operator have any effect on how putImageData works? If putImageData() is wrong, fix that don't patch putUnmultipliedImageData.
After thinking again last night, I flip to the second patch. Because putUnmultipliedImageData() shouldn't change the result according to the operation as Kenneth says. As you see, putImageData<Unmultiplied>() always overwrite the whole pixels, so the function purpose is "overwriting the destination", it means setCompositeOperation(CompositeDestinationIn) in putUnmultipliedImageData(). What do you think, James? My and Kenneth's thoughts are wrong? Thanks, (In reply to comment #15) > As I wrote in the change log and the comment, syncing between software and hardware doesn't work well if the composite operator is wrong. > As you know, if there is no hardware usage, it looks working fine. But logically putImageData() is wrong because of wrong operator inside. That is my intention for the first patch. > > What do you think, Kenneth? > > If my intention is right, as James suggested, should we use the first patch? > > (In reply to comment #14) > > (From update of attachment 81578 [details] [details]) > > Why should changing the composite operator have any effect on how putImageData works? If putImageData() is wrong, fix that don't patch putUnmultipliedImageData.
Looking at this more, I think the bug is that ImageBuffer::putUnmultipliedImageData() (ImageBufferSkia.cpp) is calling PlatformContextSkia::prepareForSoftwareDraw() where it should be calling syncSoftwareCanvas(). (See copyImage() higher in the file.) putPremultipliedImageData() looks like it has a similar bug.
Kenneth, You are right, I didn't notice the function, syncSoftwareCanvas();-( Ok, I will replace prepareForSoftwareDraw() to syncSoftwareCanvas() in both putPremultipliedImageda() and putUnmultipliedImageData(). (In reply to comment #17) > Looking at this more, I think the bug is that ImageBuffer::putUnmultipliedImageData() (ImageBufferSkia.cpp) is calling PlatformContextSkia::prepareForSoftwareDraw() where it should be calling syncSoftwareCanvas(). (See copyImage() higher in the file.) putPremultipliedImageData() looks like it has a similar bug.
Created attachment 81732 [details] Patch
Comment on attachment 81732 [details] Patch Please review again.
Comment on attachment 81732 [details] Patch Looks good to me.
Comment on attachment 81732 [details] Patch Clearing flags on attachment: 81732 Committed r78103: <http://trac.webkit.org/changeset/78103>
All reviewed patches have been landed. Closing bug.
(In reply to comment #23) > All reviewed patches have been landed. Closing bug. Thanks very much for fixing this!