Bug 93171

Summary: Remove decoding of frames in BitmapImage::frameHasAlphaAtIndex and BitmapImage::frameOrientationAtIndex
Product: WebKit Reporter: Hin-Chung Lam <hclam>
Component: ImagesAssignee: Hin-Chung Lam <hclam>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, dongseong.hwang, eric.carlson, eric, hclam, hyatt, nduca, nick, noel.gordon, pkasting, simon.fraser, skyul, vangelis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 90375    
Bug Blocks: 93636    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Hin-Chung Lam
Reported 2012-08-03 18:45:41 PDT
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.
Attachments
Patch (8.64 KB, patch)
2012-08-03 18:54 PDT, Hin-Chung Lam 		no flags
DetailsFormatted DiffDiff
Patch (13.48 KB, patch)
2012-08-08 16:15 PDT, Hin-Chung Lam 		no flags
DetailsFormatted DiffDiff
Patch (11.59 KB, patch)
2012-08-08 16:49 PDT, Hin-Chung Lam 		no flags
DetailsFormatted DiffDiff
Patch for landing (11.63 KB, patch)
2012-08-08 20:23 PDT, Hin-Chung Lam 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Hin-Chung Lam
Comment 1 2012-08-03 18:54:57 PDT
Created attachment 156504 [details] Patch
Dongseong Hwang
Comment 2 2012-08-04 01:16:14 PDT
Looks good to me. :)
Hin-Chung Lam
Comment 3 2012-08-08 16:15:34 PDT
Created attachment 157324 [details] Patch
Hin-Chung Lam
Comment 4 2012-08-08 16:17:35 PDT
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.
Simon Fraser (smfr)
Comment 5 2012-08-08 16:24:07 PDT
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?
Hin-Chung Lam
Comment 6 2012-08-08 16:49:35 PDT
Created attachment 157333 [details] Patch
Hin-Chung Lam
Comment 7 2012-08-08 16:54:13 PDT
> > 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.
Hin-Chung Lam
Comment 8 2012-08-08 16:56:04 PDT
Comment on attachment 157333 [details] Patch Thanks for review. I have addressed your comments in the last review.
WebKit Review Bot
Comment 9 2012-08-08 19:35:52 PDT
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
Hin-Chung Lam
Comment 10 2012-08-08 20:23:56 PDT
Created attachment 157378 [details] Patch for landing
WebKit Review Bot
Comment 11 2012-08-08 23:23:58 PDT
Comment on attachment 157378 [details] Patch for landing Clearing flags on attachment: 157378 Committed r125154: <http://trac.webkit.org/changeset/125154>
WebKit Review Bot
Comment 12 2012-08-08 23:24:03 PDT
All reviewed patches have been landed. Closing bug.
Dean Jackson
Comment 13 2012-08-09 11:40:29 PDT
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
Hin-Chung Lam
Comment 14 2012-08-09 11:41:21 PDT
I'm looking into this.
Hin-Chung Lam
Comment 15 2012-08-09 11:44:08 PDT
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.
Dean Jackson
Comment 16 2012-08-09 11:54:10 PDT
Followup in https://bugs.webkit.org/show_bug.cgi?id=93636
Note You need to log in before you can comment on or make changes to this bug.