Partially loaded JPEGs should have alpha channel
Created attachment 126300 [details] Patch
I've tested that this patch fixes the corruption seen in http://code.google.com/p/chromium/issues/detail?id=113171.
Comment on attachment 126300 [details] Patch Will all clients of this code properly pay attention to the transition from translucent to opaque? What happens if a JPEG image ends up in its own composited layer, for example?
We don't create composited layers for images until the image is fully loaded (specifically until isLoaded() returns true, triggered by the notifyFinished() callback). So I think we're fine on that front. Any way to get test coverage for this, perhaps by using an http test configured to serve only part of the image?
Comment on attachment 126300 [details] Patch I'm not sure I agree with this. Isn't it up to image clients to only draw the loaded part of the image?
If some clients miss the transition to opaque, then they're not worse off than they are now since they have been treating the image transparent anyway -- otherwise they would have seen the corruption during the load. Testing should be possible if we can control the http response, or if there is some other way to load an image partially, say, from a data URL.
(In reply to comment #5) > I'm not sure I agree with this. Isn't it up to image clients to only draw the loaded part of the image? I agree that it would be more efficient to only draw the valid part of the image, but this seems like a fair bit of complexity for a relatively small gain. The optimization would only kick in while the image is being loaded.
Created attachment 127523 [details] Patch
This new version adds a test for rendering a partially loaded JPEG. I fear it might not be entirely deterministic due to the lack of synchronization between the image loading and rendering; a new callback or additional polling could fix that. I still plan to investigate propagating the loaded region up to the client, but please feel free to review the test in the meanwhile.
Comment on attachment 127523 [details] Patch Attachment 127523 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11540403 New failing tests: http/tests/loading/partial-jpeg.php
Created attachment 127612 [details] Patch Move resources out of the way to fix test failure.
Comment on attachment 127612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127612&action=review The code and test look good overall, but the test needs a little more work in my opinion. > LayoutTests/http/tests/loading/partial-jpeg-expected.txt:4 > +main frame - didFailLoadWithError Are these diagnostic outputs really expected to be in the result? > LayoutTests/http/tests/loading/partial-jpeg.html:16 > + setTimeout(forceDisplay, 0); Based on the presence of the "didFailLoadWithError" diagnostic output above, can you use an onError callback on the image tag in order to reliably detect when it's loaded as far as it's going to, and launch the rest of the test then? Also, I think you need to use layoutTestController.waitUntilDone() / layoutTestController.notifyDone() to further avoid race conditions in this test case.
Created attachment 127840 [details] Patch
(In reply to comment #12) > > LayoutTests/http/tests/loading/partial-jpeg-expected.txt:4 > > +main frame - didFailLoadWithError > > Are these diagnostic outputs really expected to be in the result? Nope, looks like they got enabled automatically since the test was under loading/. I've now moved the test under incremental/. > > LayoutTests/http/tests/loading/partial-jpeg.html:16 > > + setTimeout(forceDisplay, 0); > > Based on the presence of the "didFailLoadWithError" diagnostic output above, can you use an onError callback on the image tag in order to reliably detect when it's loaded as far as it's going to, and launch the rest of the test then? That's a good suggestion, but turns out the load failure is triggered by window.stop() and not by the image tag. onError never fires for the image, since the load just stalls and doesn't actually fail. > Also, I think you need to use layoutTestController.waitUntilDone() / layoutTestController.notifyDone() to further avoid race conditions in this test case. Good idea, done.
Comment on attachment 127840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127840&action=review Thanks, looks good overall. Please regenerate the ChangeLogs and I'll r+ it. BTW, please mark it cq? if you want it submitted to the commit queue. > Source/WebCore/ChangeLog:25 > + http/tests/loading/partial-jpeg.php Please regenerate the ChangeLogs with the new test names. > LayoutTests/ChangeLog:23 > + * http/tests/loading/resources/checkerboard.jpg: Added. These are out of date now.
Created attachment 130119 [details] Patch Regenerated changelogs.
Comment on attachment 130119 [details] Patch Looks good.
Comment on attachment 130119 [details] Patch Clearing flags on attachment: 130119 Committed r109779: <http://trac.webkit.org/changeset/109779>
All reviewed patches have been landed. Closing bug.
(In reply to comment #6) > If some clients miss the transition to opaque, then they're not worse off than they are now since they have been treating the image transparent anyway -- otherwise they would have seen the corruption during the load. You won't see the corruption unless an image is large and is being decoded during load. And then only with skia change r3036 so far. One has to wonder how/why skia is going around ImageSource::frameHasAlphaAtIndex() http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/ImageSource.cpp?rev=109779#L173 or wonder if the bug would reproduce for a large, opaque PNG http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp?rev=109779#L369 No test for PNG here, and the JPEG test needs fixing according the dashboard.
This change regressed WebGL color correction in some cases. Filed bug 82428 about that: Kenneth could you please review?
(In reply to comment #20) > One has to wonder how/why skia is going around ImageSource::frameHasAlphaAtIndex() > http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/ImageSource.cpp?rev=109779#L173 That is indeed a good question. I'll look into it. > or wonder if the bug would reproduce for a large, opaque PNG > http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp?rev=109779#L369 Right, looks like the bug should happen for PNGs too. > No test for PNG here, and the JPEG test needs fixing according the dashboard. Opened bug 82480 for this.
(In reply to comment #22) > (In reply to comment #20) > > One has to wonder how/why skia is going around ImageSource::frameHasAlphaAtIndex() > > http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/ImageSource.cpp?rev=109779#L173 > > That is indeed a good question. I'll look into it. Looks like ImageSkia.cpp doesn't consult currentFrameHasAlpha() and instead draws the underlying SkBitmap blindly. Seems like an optimization opportunity to me. I'll put together a patch.