Bug 78239 - Partially loaded JPEGs should have alpha channel
Summary: Partially loaded JPEGs should have alpha channel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sami Kyostila
URL:
Keywords:
Depends on:
Blocks: 82428 82480 95707 96064 98487
  Show dependency treegraph
 
Reported: 2012-02-09 07:34 PST by Sami Kyostila
Modified: 2012-10-07 20:58 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.05 KB, patch)
2012-02-09 07:40 PST, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (8.31 KB, patch)
2012-02-16 22:13 PST, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (8.38 KB, patch)
2012-02-17 10:47 PST, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (8.32 KB, patch)
2012-02-20 12:02 PST, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (8.28 KB, patch)
2012-03-05 06:52 PST, Sami Kyostila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sami Kyostila 2012-02-09 07:34:32 PST
Partially loaded JPEGs should have alpha channel
Comment 1 Sami Kyostila 2012-02-09 07:40:05 PST
Created attachment 126300 [details]
Patch
Comment 2 Sami Kyostila 2012-02-09 07:42:54 PST
I've tested that this patch fixes the corruption seen in http://code.google.com/p/chromium/issues/detail?id=113171.
Comment 3 Kenneth Russell 2012-02-09 10:55:56 PST
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?
Comment 4 James Robinson 2012-02-09 11:01:36 PST
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 5 Simon Fraser (smfr) 2012-02-09 11:06:56 PST
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?
Comment 6 Sami Kyostila 2012-02-09 11:07:46 PST
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.
Comment 7 Sami Kyostila 2012-02-09 11:10:40 PST
(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.
Comment 8 Sami Kyostila 2012-02-16 22:13:24 PST
Created attachment 127523 [details]
Patch
Comment 9 Sami Kyostila 2012-02-16 22:22:17 PST
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 10 WebKit Review Bot 2012-02-17 01:35:37 PST
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
Comment 11 Sami Kyostila 2012-02-17 10:47:42 PST
Created attachment 127612 [details]
Patch

Move resources out of the way to fix test failure.
Comment 12 Kenneth Russell 2012-02-19 11:20:56 PST
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.
Comment 13 Sami Kyostila 2012-02-20 12:02:40 PST
Created attachment 127840 [details]
Patch
Comment 14 Sami Kyostila 2012-02-20 12:08:50 PST
(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 15 Kenneth Russell 2012-02-21 09:32:47 PST
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.
Comment 16 Sami Kyostila 2012-03-05 06:52:14 PST
Created attachment 130119 [details]
Patch

Regenerated changelogs.
Comment 17 Kenneth Russell 2012-03-05 10:08:51 PST
Comment on attachment 130119 [details]
Patch

Looks good.
Comment 18 WebKit Review Bot 2012-03-05 11:55:13 PST
Comment on attachment 130119 [details]
Patch

Clearing flags on attachment: 130119

Committed r109779: <http://trac.webkit.org/changeset/109779>
Comment 19 WebKit Review Bot 2012-03-05 11:55:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 noel gordon 2012-03-28 01:12:43 PDT
(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.
Comment 21 noel gordon 2012-03-28 01:16:52 PDT
This change regressed WebGL color correction in some cases.  Filed bug 82428 about that: Kenneth could you please review?
Comment 22 Sami Kyostila 2012-03-28 10:32:57 PDT
(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.
Comment 23 Sami Kyostila 2012-03-28 12:04:11 PDT
(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.