Bug 53757 – [Chromium] Issue 58536: Fix Layout Test canvas/philip/tests/2d.imageData.put.alpha.html with --accelerated-2d-canvas.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Naoki Takano]

[Chromium] Issue 58536: Fix Layout Test canvas/philip/tests/2d.imageData.put.alpha.html with --accelerated-2d-canvas.

[Naoki Takano]

Created attachment 81189 [details]
Patch

[Naoki Takano]

Could you review my patch?

[Adam Barth]

This is a bit outside my area of expertise.

[Naoki Takano]

Do you know who is the right person?

(In reply to comment #3)
> This is a bit outside my area of expertise.

[Adam Barth]

You can try running "webkit-patch suggest-reviewers" on your patch to see which reviewers it suggests.

[Adam Barth]

abarth@quadzen:~/svn/webkit3$ ./Tools/Scripts/webkit-patch suggest-reviewers
Dan Bernstein
David Hyatt
Dirk Schulze
Nikolas Zimmermann
Oliver Hunt
Simon Fraser
James Robinson

[Naoki Takano]

Added a couple of people whom Adam recommended.

Could you review my patch?

[Naoki Takano]

Is there anybody who can review my patch?

[Kenneth Russell]

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.

[Naoki Takano]

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.

[Kenneth Russell]

(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.

[Naoki Takano]

Created attachment 81578 [details]
Patch

[Naoki Takano]

Comment on attachment 81578 [details]
Patch

Ok, I move the logic to ImageBufferSkia.cpp. Please review.

[James Robinson]

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.

[Naoki Takano]

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.

[Naoki Takano]

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.

[Kenneth Russell]

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.

[Naoki Takano]

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.

[Naoki Takano]

Created attachment 81732 [details]
Patch

[Naoki Takano]

Comment on attachment 81732 [details]
Patch

Please review again.

[Kenneth Russell]

Comment on attachment 81732 [details]
Patch

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 81732 [details]
Patch

Clearing flags on attachment: 81732

Committed r78103: <http://trac.webkit.org/changeset/78103>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen White]

(In reply to comment #23)
> All reviewed patches have been landed.  Closing bug.

Thanks very much for fixing this!