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

Description Hin-Chung Lam 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.
Comment 1 Hin-Chung Lam 2012-08-03 18:54:57 PDT
Created attachment 156504 [details]
Patch
Comment 2 Dongseong Hwang 2012-08-04 01:16:14 PDT
Looks good to me. :)
Comment 3 Hin-Chung Lam 2012-08-08 16:15:34 PDT
Created attachment 157324 [details]
Patch
Comment 4 Hin-Chung Lam 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.
Comment 5 Simon Fraser (smfr) 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?
Comment 6 Hin-Chung Lam 2012-08-08 16:49:35 PDT
Created attachment 157333 [details]
Patch
Comment 7 Hin-Chung Lam 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.
Comment 8 Hin-Chung Lam 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.
Comment 9 WebKit Review Bot 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
Comment 10 Hin-Chung Lam 2012-08-08 20:23:56 PDT
Created attachment 157378 [details]
Patch for landing
Comment 11 WebKit Review Bot 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>
Comment 12 WebKit Review Bot 2012-08-08 23:24:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Dean Jackson 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
Comment 14 Hin-Chung Lam 2012-08-09 11:41:21 PDT
I'm looking into this.
Comment 15 Hin-Chung Lam 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.
Comment 16 Dean Jackson 2012-08-09 11:54:10 PDT
Followup in https://bugs.webkit.org/show_bug.cgi?id=93636