Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1407393002:
ImageBitmap: Change two enum uses. (Closed)
|
Created:
5 years, 2 months ago by jbroman Modified:
5 years, 2 months ago CC:
blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, vmpstr+blinkwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImageBitmap: Change two enum uses.
An earlier CL (https://codereview.chromium.org/1382883002/) made two changes
which seem to have unintended effects on behaviour.
1. The ImageBitmap constructor asserts that its data is opaque (whereas
previously it took the default NonOpaque opacity mode), but if this is so
it should have a comment justifying it.
2. UnacceleratedImageBufferSurface is clearings the SkSurface if it was
told _not_ to initialize image pixels, which seems like an inversion of the
intended condition.
BUG=543515, 544691
Committed: https://crrev.com/1998d1cc5f2985cdeac94d3e669b0e7901a77bbb
Cr-Commit-Position: refs/heads/master@{#354880}
Patch Set 1 #
Total comments: 7
Messages
Total messages: 18 (3 generated)
jbroman@chromium.org changed reviewers: + junov@chromium.org, xidachen@chromium.org
This looks like a couple of simple mistakes. What do you think? I tried to add a layout test to cover the opaque case, but it does something else wrong and I haven't put time in to debug it. This is probably a trivial enough fix to land without, but for posterity here it was (https://gist.github.com/jeremyroman/9f9cbd38b1bd9b0482ee).
https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:101: OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, DoNotInitializeImagePixels); This part is tricky. It was set to be Opaque on purpose. Because in the constructor of "UnacceleratedImageBufferSurface" class, the following line: m_surface = adoptRef(SkSurface::NewRaster(info, Opaque == opacityMode ? 0 : &disableLCDProps)); takes long time when it is NonOpaque, but takes almost no time when it is set to Opaque. My opinion on this implementation is that the buffer will be filled in by the source data later, so it doesn't matter whether it is opaque or non-opaque at the buffer allocation step. However, this could be wrong. https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp (right): https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:49: if (initializationMode == InitializeImagePixels) { Correct. Thank you for catching this. I believe this regression is also likely caused by this mistake. https://code.google.com/p/chromium/issues/detail?id=543515&q=xidachen%40chrom... I have verified that canvas.drawImage() eventually calls this method, so this change should fix the above regression.
https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:101: OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, DoNotInitializeImagePixels); On 2015/10/18 at 14:08:48, xidachen wrote: > This part is tricky. It was set to be Opaque on purpose. Because in the constructor of "UnacceleratedImageBufferSurface" class, the following line: > > m_surface = adoptRef(SkSurface::NewRaster(info, Opaque == opacityMode ? 0 : &disableLCDProps)); > > takes long time when it is NonOpaque, but takes almost no time when it is set to Opaque. My opinion on this implementation is that the buffer will be filled in by the source data later, so it doesn't matter whether it is opaque or non-opaque at the buffer allocation step. However, this could be wrong. I'd like Justin's opinion, but it seems to me that this causes the image to have kOpaque_AlphaType in its SkImageInfo, which shouldn't apply to images that aren't opaque.
I think it may need a bit more time to debug this issue. https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:101: OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, DoNotInitializeImagePixels); Hmmm, I just found something very strange. With the change that you have in the "UnacceleratedImageBufferSurface", it won't pass your layout test even if I change this line to be: OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, InitializeImagePixels);
https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:101: OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, DoNotInitializeImagePixels); On 2015/10/18 at 15:44:52, xidachen wrote: > Hmmm, I just found something very strange. With the change that you have in the "UnacceleratedImageBufferSurface", it won't pass your layout test even if I change this line to be: > > OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, InitializeImagePixels); Yeah, something fishy is going on (or I'm misunderstanding the ImageData API). I spent about 30 min on Friday trying to figure out what was going on and why I got very very wrong values, but I haven't dug any deeper.
We can spend more time on the ImageData API, but I think we may need to correct the mistake I made in the "UnacceleratedImageBuffer" first, I am concerned that other people who use it will be misled by my mistake.
Description was changed from ========== ImageBitmap: Change two enum uses. An earlier CL (https://codereview.chromium.org/1382883002/) made two changes which seem to have unintended effects on behaviour. 1. The ImageBitmap constructor asserts that its data is opaque (whereas previously it took the default NonOpaque opacity mode), but if this is so it should have a comment justifying it. 2. UnacceleratedImageBufferSurface is clearings the SkSurface if it was told _not_ to initialize image pixels, which seems like an inversion of the intended condition. ========== to ========== ImageBitmap: Change two enum uses. An earlier CL (https://codereview.chromium.org/1382883002/) made two changes which seem to have unintended effects on behaviour. 1. The ImageBitmap constructor asserts that its data is opaque (whereas previously it took the default NonOpaque opacity mode), but if this is so it should have a comment justifying it. 2. UnacceleratedImageBufferSurface is clearings the SkSurface if it was told _not_ to initialize image pixels, which seems like an inversion of the intended condition. BUG=543515,544691 ==========
Added references to bugs xidachen believes to be related.
lgtm Thanks for fixing it.
lgtm https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:101: OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, DoNotInitializeImagePixels); I agree that the image buffer should be marked as non-opaque. This will result in the newImageSnapshot that is taken below to be marked as non-opaque, which is conservatively accurate. https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:106: buffer->putByteArray(Premultiplied, data->data()->data(), data->size(), srcRect, IntPoint(std::min(0, -cropRect.x()), std::min(0, -cropRect.y()))); This operation is an over-write. If it covers buffer entirely, we should not need to intialize the pixels, and I think that is supposed to always be the case.
On 2015/10/18 14:08:48, xidachen wrote: > This part is tricky. It was set to be Opaque on purpose. Because in the > constructor of "UnacceleratedImageBufferSurface" class, the following line: > > m_surface = adoptRef(SkSurface::NewRaster(info, Opaque == opacityMode ? 0 : > &disableLCDProps)); > That is inconsequential because we will never draw text to this buffer. We just push image data onto it
On 2015/10/18 15:44:14, jbroman wrote: > I'd like Justin's opinion, but it seems to me that this causes the image to have > kOpaque_AlphaType in its SkImageInfo, which shouldn't apply to images that > aren't opaque. Exactly.
On 2015/10/18 15:48:00, jbroman wrote: > https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): > > https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/ImageBitmap.cpp:101: OwnPtr<ImageBuffer> > buffer = ImageBuffer::create(data->size(), NonOpaque, > DoNotInitializeImagePixels); > On 2015/10/18 at 15:44:52, xidachen wrote: > > Hmmm, I just found something very strange. With the change that you have in > the "UnacceleratedImageBufferSurface", it won't pass your layout test even if I > change this line to be: > > > > OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, > InitializeImagePixels); > > Yeah, something fishy is going on (or I'm misunderstanding the ImageData API). I > spent about 30 min on Friday trying to figure out what was going on and why I > got very very wrong values, but I haven't dug any deeper. What layout test are we talking about?
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407393002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407393002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/1998d1cc5f2985cdeac94d3e669b0e7901a77bbb Cr-Commit-Position: refs/heads/master@{#354880} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
