93171 – Remove decoding of frames in BitmapImage::frameHasAlphaAtIndex and BitmapImage::frameOrientationAtIndex

RESOLVED FIXED 93171

Remove decoding of frames in BitmapImage::frameHasAlphaAtIndex and BitmapImage::frameOrientationAtIndex

https://bugs.webkit.org/show_bug.cgi?id=93171

Summary Remove decoding of frames in BitmapImage::frameHasAlphaAtIndex and BitmapImag...

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	Details Formatted Diff Diff
Patch (13.48 KB, patch) 2012-08-08 16:15 PDT, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (11.59 KB, patch) 2012-08-08 16:49 PDT, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch for landing (11.63 KB, patch) 2012-08-08 20:23 PDT, Hin-Chung Lam	no flags	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Images

Assignee

Hin-Chung Lam

Reported

2012-08-03 18:45 PDT

Modified

2012-08-09 19:08 PDT History

CC List

14 users Show

URL

Keywords

Depends on

90375

Blocks

93636

Dependencies

tree graph