These two methods shouldn't require full image decoding. They are blockers for having asynchronous / deferred image decoding. frameOrientationAtIndex This is not implemented at the moment. And this information can be read from the image header without decoding the entire image. frameHasAlphaAtIndex This method is used for optimization in drawing. CG port doesn't return this reliable and use a simple jpeg->no, png/gif->yes heuristic. Having to decoding a full frame to answer this call is too heavy. This also blocks asynchronous image decoding as this is not an essential entry point to kick start image decoding. We want image decoding to start only when an image is drawn, at least for single frame images.
Created attachment 156504 [details] Patch
Looks good to me. :)
Created attachment 157324 [details] Patch
There's no new code in the second patch. It added a test for partially loaded JPEG images. This shows that this change doesn't affect rendering results when an image is fully loaded and partially loaded.
Comment on attachment 157324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157324&action=review > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:284 > + return m_frameBufferCache[index].status() != ImageFrame::FrameComplete || m_frameBufferCache[index].hasAlpha(); I find this confusing to read. It would be clearer as if (m_frameBufferCache[index].status() == ImageFrame::FrameComplete) return m_frameBufferCache[index].hasAlpha(); return true; > Source/WebCore/platform/image-decoders/ImageDecoder.h:280 > + virtual bool frameHasAlphaAtIndex(size_t); Can this be const? > LayoutTests/fast/images/jpg-image-with-background.html:11 > +<html> > +<head> > +<script> > +if (window.testRunner) > + testRunner.dumpAsText(true); > +</script> > +</head> > +<body> > +<img src="resources/test-load.jpg" style="background-color: blue;"></img> > +</body> > +</html> I'm not sure if this is worth testing. > LayoutTests/http/tests/resources/load-and-stall.php:37 > +<?php > +$name = $_GET['name']; > +$stallAt = $_GET['stallAt']; > +$stallFor = $_GET['stallFor']; > +$mimeType = $_GET['mimeType']; > + > +$file = fopen($name, "rb"); > +if (!$file) > + die("Cannot open file."); > + > +header("Content-Type: " . $mimeType); > +header("Content-Length: " . filesize($name)); > + > +if (isset($stallAt) && isset($stallFor)) { > + $stallAt = (int)$stallAt; > + $stallFor = (int)$stallFor; > + if ($stallAt > filesize($name)) > + die("Incorrect value for stallAt."); > + > + while ($written < $stallAt) { > + $write = 1024; > + if ($write > $stallAt - $written) > + $write = $stallAt - $written; > + > + echo(fread($file, $write)); > + $written += $write; > + flush(); > + ob_flush(); > + } > + sleep($stallFor); > + echo(fread($file, filesize($name) - $stallAt)); > +} else { > + echo(fread($file, filesize($name))); > +} > + > +fclose($file); > +?> Does this match the style we use for other PHP files?
Created attachment 157333 [details] Patch
> > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:284 > > + return m_frameBufferCache[index].status() != ImageFrame::FrameComplete || m_frameBufferCache[index].hasAlpha(); > > I find this confusing to read. It would be clearer as > if (m_frameBufferCache[index].status() == ImageFrame::FrameComplete) > return m_frameBufferCache[index].hasAlpha(); > return true; Done. > > Source/WebCore/platform/image-decoders/ImageDecoder.h:280 > > + virtual bool frameHasAlphaAtIndex(size_t); > > Can this be const? Done. > > LayoutTests/fast/images/jpg-image-with-background.html:11 > > I'm not sure if this is worth testing. Removed. > > LayoutTests/http/tests/resources/load-and-stall.php:37 > > Does this match the style we use for other PHP files? Matched the style of this file with reset-temp-file.php in the same directory. Indentation using 4 spaces.
Comment on attachment 157333 [details] Patch Thanks for review. I have addressed your comments in the last review.
Comment on attachment 157333 [details] Patch Rejecting attachment 157333 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: Kit/chromium/third_party/yasm/source/patched-yasm --revision 134927 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 47>At revision 134927. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/13460408
Created attachment 157378 [details] Patch for landing
Comment on attachment 157378 [details] Patch for landing Clearing flags on attachment: 157378 Committed r125154: <http://trac.webkit.org/changeset/125154>
All reviewed patches have been landed. Closing bug.
This appears to have caused http/tests/images/jpg-img-partial-load.html to regress on Mac Lion. http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r125185%20(2163)/results.html
I'm looking into this.
This test is written on chromium's DRT which uses different image decoders than CG port. I think this maybe the reason. I'll try this on lion to verify. In the mean time please mark this as timeout in TestExpectations. Thanks.
Followup in https://bugs.webkit.org/show_bug.cgi?id=93636